-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix permissions application in File.copy
#15520
Fix permissions application in File.copy
#15520
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds weird to set the permissions first, but I guess that if the copy fails, then the proper permissions would still have been set on the destination file 🤔
And it avoids the race condition that it could be possible to access the file with the default file creation permissions, while we're writing to it. |
we're pulling up a test env to test this |
this does not solve our problem so thinking a
Obviously this should still be merged as the old version would copy the file and then fail |
Okay so the problem in your case seems to be not the |
Applying the final permissions already to a newly created file makes the extra call to
chmod
unnecessary.We still need to detect whether the permissions are correct (this is the case when creating a new file or when an existing file already has the intended permissions). Otherwise we need to adjust the permissions. In that case this patch introduce an addional syscall (accessing
d.info
befored.chmod
). There is no other way to determine whetherFile.open
created a new file or opened an existing one.This change ensures correct behaviour by avoiding unnecessary file mode changes which can be impossible in some environments (see #15518).
The code comment claiming that we need to only change permissions after writing the data is based on a false assumption. Changing the permissions on disk does not have any effect on existing file descriptors. The OS only checks permissions when opening a file.
Closes #15510