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

Fix permissions application in File.copy #15520

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Feb 25, 2025

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 before d.chmod). There is no other way to determine whether File.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

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:system labels Feb 25, 2025
@straight-shoota straight-shoota self-assigned this Feb 25, 2025
Copy link
Contributor

@ysbaddaden ysbaddaden left a 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 🤔

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 25, 2025

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.

@straight-shoota straight-shoota added this to the 1.16.0 milestone Feb 25, 2025
@stakach
Copy link
Contributor

stakach commented Feb 25, 2025

we're pulling up a test env to test this

@stakach
Copy link
Contributor

stakach commented Feb 26, 2025

this does not solve our problem so thinking a copy_data function and a preserve: true param to FileUtils#mv would additionally need to be made. The volume we are copying to is effectively a mounted windows share I think

Error changing permissions: '/app/www/1740530256731_1428/scripts/local_auth.js': Operation not permitted (File::Error)
  from /usr/share/crystal/src/crystal/system/unix/file.cr:127:7 in 'system_chmod'

Obviously this should still be merged as the old version would copy the file and then fail
versus failing fast in this version

@straight-shoota
Copy link
Member Author

Okay so the problem in your case seems to be not the chmod itself but that the file system doesn't support the intended permissions and rejects them.

@straight-shoota straight-shoota merged commit 565988b into crystal-lang:master Feb 27, 2025
34 checks passed
@straight-shoota straight-shoota deleted the fix/file.copy-permissions branch February 27, 2025 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants