Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle closing files that have been writen to #7199

Merged
merged 3 commits into from Mar 23, 2023

Conversation

samcoe
Copy link
Member

@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
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Mar 19, 2023
// HasEnvToken returns true when a token has been specified in an
// environment variable, else returns false.
Copy link
Member 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
Member

@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
Member 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)

mislav
mislav approved these changes Mar 22, 2023
Copy link
Member

@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.

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Mar 22, 2023
@samcoe samcoe enabled auto-merge (squash) March 23, 2023 01:05
@samcoe samcoe merged commit 39805fa into trunk Mar 23, 2023
10 checks passed
@samcoe samcoe deleted the fix-writable-file-handle branch March 23, 2023 01:17
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
The GitHub CLI
  
Pending Release 🥚
Development

Successfully merging this pull request may close these issues.

None yet

2 participants