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

G307 is not a valid rule for most OSes #856

Closed
moriyoshi opened this issue Aug 21, 2022 · 2 comments
Closed

G307 is not a valid rule for most OSes #856

moriyoshi opened this issue Aug 21, 2022 · 2 comments

Comments

@moriyoshi
Copy link

moriyoshi commented Aug 21, 2022

Related:

According to the above, the rationale behind this rule seems to be addressed in this blog post [1], but this post still fails to justify why one should report the error returned by os.File.Close() to the callee.

The blog post detailed the things mostly correctly, but it missed the point the OS does not guarantee the buffer behind the file object associated to the file descriptor to be synchronized to the disk no matter if close raises an error or not. At least POSIX does not specify such semantics [2] except that the file descriptor may have got invalidated after a failed call to close. If you need to ensure the buffer to be flushed, you'll always need to call a Go API call equivalent to fsync or fdatasync separately albeit the metadata of the file object still won't be synchronized as described in [3]. Hence it cannot be so simple as to avoid ignoring the error, at least in terms of persistence to the device in the context of which the blog was written about.

In short, you should virtually have nothing to do on a close failure, but reporting the error to the user is just a nicer thing to do, and IMO it's totally out of the scope where gosec should be concerned.

[1] https://www.joeshaw.org/dont-defer-close-on-writable-files/
[2] https://pubs.opengroup.org/onlinepubs/009604499/functions/close.html
[3] https://man7.org/linux/man-pages/man2/fsync.2.html

@ccojocar
Copy link
Member

ccojocar commented Aug 22, 2022

Thanks for raising this. gosec tires to do its best effort to advice the user. Maybe in this case is not a hight security risk, but it is still a useful warning.

@moriyoshi
Copy link
Author

Closing without any discussion would sound good to me if you proposed enough reason for it. At least this warning has nothing to do with security thing.

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

No branches or pull requests

2 participants