Fix permissions application in File.copy#15520
Fix permissions application in File.copy#15520straight-shoota merged 2 commits intocrystal-lang:masterfrom
File.copy#15520Conversation
ysbaddaden
left a comment
There was a problem hiding this comment.
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
chmodunnecessary.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.infobefored.chmod). There is no other way to determine whetherFile.opencreated 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