Skip to content

Properly handle closing files that have been writen to#7199

Merged
samcoe merged 3 commits intotrunkfrom
fix-writable-file-handle
Mar 23, 2023
Merged

Properly handle closing files that have been writen to#7199
samcoe merged 3 commits intotrunkfrom
fix-writable-file-handle

Conversation

@samcoe
Copy link
Contributor

@samcoe samcoe commented Mar 19, 2023

Addresses CodeQL warnings about properly handling the return value when closing a file that has been written to.

Fixes https://github.com/cli/cli/security/code-scanning/116
Fixes https://github.com/cli/cli/security/code-scanning/115
Fixes https://github.com/cli/cli/security/code-scanning/114
Fixes https://github.com/cli/cli/security/code-scanning/113

@samcoe samcoe requested a review from a team as a code owner March 19, 2023 23:34
@samcoe samcoe self-assigned this Mar 19, 2023
@samcoe samcoe requested review from mislav and removed request for a team March 19, 2023 23:34
Comment on lines +143 to +144
// HasEnvToken returns true when a token has been specified in an
// environment variable, else returns false.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change that I wanted to make to improve this comment a bit.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we care enough about bubbling errors that result from file.Close() up to the user. I'd rather say that we'd want to ignore them, especially because gh is such a short-lived process. That way we could avoid restructuring whole functions with named return values.

Have you considered

defer func() {
  _ = file.Close()
}()

@samcoe samcoe requested a review from mislav March 22, 2023 21:46
@samcoe
Copy link
Contributor Author

samcoe commented Mar 22, 2023

@mislav This is the same as the go-gh PR regarding the errors potentially being write errors, do you still feel that we could ignore them?

cc cli/go-gh#113 (comment)

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right about legitimate filesystem errors potentially being masked. I was just skittish about this requiring significant code changes at first.

@samcoe samcoe force-pushed the fix-writable-file-handle branch from abbf23a to 8695f8f Compare March 23, 2023 01:03
@samcoe samcoe enabled auto-merge (squash) March 23, 2023 01:05
@samcoe samcoe merged commit 39805fa into trunk Mar 23, 2023
@samcoe samcoe deleted the fix-writable-file-handle branch March 23, 2023 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants