Repository navigation
mv: chown the open destination file in a cross-device move - #15351
ozenyunus35 wants to merge 3 commits into
Conversation
The cross-device file copy keeps the source and destination open so a path swap cannot redirect a step to another inode, but ownership was still set by path (lchown). Set it on the open destination descriptor with fchown, as GNU does, so the comment above the block now holds. Part of uutils#14599
| // as themselves that used to run as someone else. GNU strips the bits | ||
| // in that case and so do we. | ||
| let ownership_preserved = preserve_ownership(from, to).unwrap_or(false); | ||
| let ownership_preserved = preserve_ownership_fd(&src_meta, &dst_file); |
There was a problem hiding this comment.
could you please add a test with a setgid file chgrp'ed to a supplementary group? it would cover the fchown path keeping the bit
There was a problem hiding this comment.
Added test_mv_cross_device_preserves_setgid_with_supplementary_group: it chgrps a 2755 file to a supplementary group, moves it to /dev/shm and checks both the gid and the setgid bit.
|
|
||
| /// [`preserve_ownership`] for a regular file the cross-device copy just | ||
| /// created: the owner is set on the open destination descriptor (`fchown`, as | ||
| /// GNU does), not by path, so it lands on the inode that was written. |
There was a problem hiding this comment.
please drop the "as GNU does", not needed here
| /// destination, and the mode is applied after it. | ||
| #[test] | ||
| #[cfg(target_os = "linux")] | ||
| fn test_mv_cross_device_preserves_setgid_with_supplementary_group() { |
There was a problem hiding this comment.
almost the same as test_mv_cross_device_preserves_ownership below, could you just set 0o2755 in that one and check the mode there? :)
There was a problem hiding this comment.
Done, I dropped the separate test and test_mv_cross_device_preserves_ownership now sets 0o2755 and checks the mode after the move.
|
Closing this, as I can't follow up on it further. Thanks for the reviews. |
2 participants
Part of #14599
The cross-device file copy keeps
src_file/dst_fileopen so a concurrent path swap can't redirect a step to another inode, butpreserve_ownership(from, to)still re-stat'ed both paths and changed the owner by path (lchown). The regular-file copy now usespreserve_ownership_fd, which compares the source'sstat(taken from the open source fd) with the open destination and callsrustix::fs::fchownondst_file, like GNU'sfchown(4, …). It returns whether the ids match afterwards, so the setuid/setgid stripping is unchanged. With that, the comment above the block holds as written.Not done here: creating the destination with
O_EXCL. The destination can exist when it's being replaced, so I'd rather handle that separately with its own tests. Happy to follow up.The mv tests pass on macOS (117). The Linux-only
inter_partition_copyingtests (tmpfs) couldn't run on my machine, so I'm relying on CI for those.I used an AI assistant to help draft parts of the code and tests; I reviewed the changes and ran the tests myself.