mv,cp: fix xattr TOCTOU by using file descriptor-based operations - #10545
Merged
Conversation
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
from
January 28, 2026 22:26
50618a3 to
2598f34
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
2 times, most recently
from
February 21, 2026 21:03
6dda6b4 to
3f06bad
Compare
Merging this PR will improve performance by 3.75%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
from
February 21, 2026 21:34
3f06bad to
9707371
Compare
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
from
February 21, 2026 21:50
9707371 to
c857f11
Compare
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
from
February 24, 2026 09:52
c857f11 to
6e40932
Compare
|
GNU testsuite comparison: |
sylvestre
marked this pull request as ready for review
February 26, 2026 20:36
sylvestre
force-pushed
the
issue-10014
branch
2 times, most recently
from
March 16, 2026 21:59
cf75720 to
1514ccd
Compare
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
2 times, most recently
from
March 17, 2026 10:45
35596ca to
5fa39b8
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
3 times, most recently
from
March 18, 2026 22:16
867b6a0 to
09664a9
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
issue-10014
branch
3 times, most recently
from
March 23, 2026 06:39
0211168 to
7f35960
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR hardens mv/cp xattr preservation against TOCTOU races by switching to file-descriptor-based xattr operations, and adds regression tests around directory xattrs on cross-device moves.
Changes:
- Add fd-based xattr retrieve/apply helpers in
uucore::fsxattr(plus unit test). - Update
mvdirectory fallback path to retrieve/apply xattrs via open file descriptors. - Update
cpto use fd-based xattr copying for regular files and add an integration test for cross-device dir xattrs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/by-util/test_mv.rs | Adds Linux integration test ensuring cross-device directory xattrs are preserved. |
| src/uucore/src/lib/features/fsxattr.rs | Introduces *_fd xattr APIs and unit test coverage. |
| src/uu/mv/src/mv.rs | Switches directory fallback xattr handling to fd-based operations and ignores unsupported FS errors. |
| src/uu/cp/src/cp.rs | Uses fd-based xattr copying for regular files to reduce TOCTOU exposure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "directory xattr was not preserved across devices: {}", | ||
| String::from_utf8_lossy(&out.stderr) | ||
| ); | ||
| assert_eq!(out.stdout, b"dirvalue"); |
Comment on lines
+3089
to
+3091
| let other_fs_tempdir = | ||
| TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); | ||
| let dst_path = other_fs_tempdir.path().join("dst_dir"); |
Comment on lines
+458
to
+465
| #[test] | ||
| fn test_apply_and_retrieve_xattrs_fd() { | ||
| use std::fs::OpenOptions; | ||
|
|
||
| let temp_dir = tempdir().unwrap(); | ||
| let file_path = temp_dir.path().join("test_file.txt"); | ||
|
|
||
| File::create(&file_path).unwrap(); |
| fsxattr::apply_xattrs(to, xattrs)?; | ||
| { | ||
| use std::fs::File; | ||
| fsxattr::apply_xattrs_fd_ignore_unsupported(&File::open(to)?, xattrs)?; |
Comment on lines
+143
to
+145
| /// A result containing a HashMap of attribute names and values, or an error. | ||
| #[cfg(unix)] | ||
| pub fn retrieve_xattrs_fd(source: &std::fs::File) -> std::io::Result<FxHashMap<OsString, Vec<u8>>> { |
|
Binary size comparison: |
|
GNU testsuite comparison: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/by-util/test_mv.rs:3091
- This test will hard-fail on environments where /dev/shm is missing/unwritable, and it also doesn't verify that the move is actually cross-device (so it may silently test the rename fast-path instead of the copy+delete fallback). Consider skipping gracefully if /dev/shm can't be used, and skip if source and destination are on the same device.
let other_fs_tempdir =
TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm");
let dst_path = other_fs_tempdir.path().join("dst_dir");
src/uucore/src/lib/features/fsxattr.rs:474
- This unit test currently
unwrap()s xattr operations, which will fail on filesystems/environments where user xattrs are unsupported/disabled. Other xattr tests in this module already skip in that situation; this one should too. Also, since the tested APIs are#[cfg(unix)], the test should be gated similarly.
// Apply using file descriptor
let file = OpenOptions::new().write(true).open(&file_path).unwrap();
apply_xattrs_fd(&file, test_xattrs).unwrap();
tests/by-util/test_mv.rs:3114
getfattr --only-valuestypically includes a trailing newline, so comparingout.stdouttob"dirvalue"is likely to be flaky. Also, this test currently panics ifgetfattrisn't available. Consider skipping gracefully on spawn failure and trimming stdout before comparing.
.output()
.expect("failed to run getfattr on the moved directory");
assert!(
out.status.success(),
"directory xattr was not preserved across devices: {}",
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/uucore/src/lib/features/fsxattr.rs:475
- The new fd-based xattr test can fail on filesystems that don't support user xattrs (common in some CI/container setups). Other xattr tests in this module skip when xattr operations return ENOTSUP/EOPNOTSUPP; this one should do the same to avoid flaky failures unrelated to the logic being tested.
// Apply using file descriptor
let file = OpenOptions::new().write(true).open(&file_path).unwrap();
apply_xattrs_fd(&file, test_xattrs).unwrap();
drop(file);
tests/by-util/test_mv.rs:3061
- This comment is misleading: attempting to open a directory for writing typically fails (e.g., EISDIR), rather than "silently" dropping xattrs. Rewording avoids confusion about what failure mode is being guarded against.
/// Cross-device mv of a directory must preserve the directory's own xattrs.
/// The fd-based xattr path has to open the destination read-only: a directory
/// cannot be opened for writing, so a write-mode open would silently drop them.
Path-based xattr calls in cp's copy_extended_attrs and mv's rename_dir_fallback can be redirected to a different inode by a concurrent renamer between the list and the set. Pin the inodes by doing the list/get/set through open file descriptors instead. The destination fd is opened read-only: a directory cannot be opened for writing, and fsetxattr checks write permission on the inode rather than the open mode, so read-only works for both files and directories. Closes: uutils#10014
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (5)
tests/by-util/test_mv.rs:3092
- The test intends to exercise the cross-device (EXDEV) fallback, but it doesn’t verify that the fixtures directory and
/dev/shmare actually on different devices. If they’re the same filesystem in a given environment, this test can pass without covering the fallback path. Consider skipping when the device IDs match.
let other_fs_tempdir =
TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm");
let dst_path = other_fs_tempdir.path().join("dst_dir");
src/uucore/src/lib/features/fsxattr.rs:180
apply_xattrs_fdworks on directory file descriptors too (it’s used that way in mv’s directory fallback), but the doc comment describes only “file”. Aligning the documentation with actual usage helps prevent incorrect assumptions by future callers.
/// Applies extended attributes (xattrs) to a given file using a file descriptor.
///
/// This version avoids TOCTOU races by operating on an open file descriptor
/// rather than a path, ensuring all operations target the same inode.
///
tests/by-util/test_mv.rs:3108
- This test skips when
setfattrcannot be executed, but it will panic ifgetfattris missing or cannot be executed. HandleCommand::new("getfattr").output()errors the same way and skip the test, so CI environments withoutgetfattrdon’t fail spuriously.
let out = Command::new("getfattr")
.args([
"-n",
"user.dirattr",
"--only-values",
dst_path.to_str().unwrap(),
])
.output()
.expect("failed to run getfattr on the moved directory");
src/uucore/src/lib/features/fsxattr.rs:145
retrieve_xattrs_fdis used for directories as well (e.g. mv cross-device directory fallback), but the doc comment currently says it retrieves xattrs for a “file”. Updating the docs avoids misleading callers about supported handle types.
This issue also appears on line 176 of the same file.
/// Retrieves the extended attributes (xattrs) of a given file using a file descriptor.
///
/// This version avoids TOCTOU races by operating on an open file descriptor
/// rather than a path, ensuring all operations target the same inode.
///
src/uu/cp/src/cp.rs:1742
- The comment says
copy_extended_attrs“uses file descriptor-based operations”, but the implementation only does so for regular files (metadata.is_file()); directories and other types still use path-based xattr operations. Clarifying the comment avoids overstating the TOCTOU mitigation coverage.
///
/// Uses file descriptor-based operations to avoid TOCTOU races during xattr copying.
#[cfg(all(unix, not(target_os = "android")))]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #10014