mv: use 'cannot overwrite TARGET: Directory not empty' for non-empty dir target - #12124
Draft
sylvestre wants to merge 6 commits into
Draft
mv: use 'cannot overwrite TARGET: Directory not empty' for non-empty dir target#12124sylvestre wants to merge 6 commits into
sylvestre wants to merge 6 commits into
Conversation
|
GNU testsuite comparison: |
This comment was marked as resolved.
This comment was marked as resolved.
sylvestre
force-pushed
the
fix/mv-cannot-overwrite-not-empty
branch
from
May 5, 2026 18:42
c3fa3d3 to
c1023eb
Compare
65 tasks
Contributor
Author
|
i will rebase it |
Add `fsxattr::copy_xattrs_fd` that operates on two open file descriptors via the `xattr::FileExt` trait. Path-based xattr copies re-resolve the source and destination path on every list / get / set syscall, so a concurrent renamer could redirect individual operations to different inodes mid-copy and produce a non-atomic snapshot — a correctness bug that becomes security-relevant when privileged processes move files carrying security xattrs like SELinux labels or file capabilities. Refactor mv's cross-device regular-file fallback (`rename_file_fallback`) to open source and destination itself, do the content copy via `io::copy`, and invoke the new fd-based xattr helper while both fds are still live. Both inodes are pinned for the whole copy + xattr window. Non-Unix builds keep the previous `fs::copy` path. Both opens go through `uucore::safe_copy` with `nofollow=true`, so the path-swap-to-symlink TOCTOU that cp uutils#10017 closes for cp's regular-file path is closed here for mv too — symmetrically on the source (so a planted symlink can't redirect the read) and on the destination (so a planted symlink can't redirect the truncate and write to a victim file the caller has permission to write). The path-based `copy_xattrs` is kept for callers that cannot pin fds (notably directory traversal). Extend `util/check-safe-traversal.sh` with a cross-device mv strace check that fails if any path-based listxattr/getxattr/setxattr lands on the source or destination paths. The check skips gracefully on systems where the tmpfs used for the cross-device probe doesn't support user xattrs.
`rename_file_fallback` (the cross-device / EXDEV fallback) unlinked the destination before calling `fs::copy`, creating a TOCTOU window unique to uutils: between the unlink and the subsequent open an attacker with write access to the destination directory could recreate the path as a symlink to a privileged file, causing the copy to write through the symlink. Match GNU mv's pattern: rely on `fs::copy`'s `O_WRONLY|O_CREAT|O_TRUNC` to truncate an existing regular file in place, and do not unlink first. The separate symlink-destination unlink is kept here; issue uutils#10010 / uutils#10014 address atomic symlink replacement more thoroughly. Updates `test_mv_cross_device_permission_denied` to target a non-existent destination (where the create itself requires dir write), since mv now correctly succeeds on truncating an existing writable file in a read-only dir — matching GNU.
Commit 17b8b1e (uutils#10014) made `copy_xattrs` and `copy_xattrs_fd` silently absorb `ENOTSUP`/`EOPNOTSUPP`. mv only ever discards their return value (`let _ = ...`), so its behavior was unchanged — but the two functions are also called by cp, where `handle_preserve` propagates the error so `cp --preserve=xattr` (required-preserve) can exit 1 to match GNU cp when the destination filesystem rejects xattrs. Verified against GNU cp 9.10 with `strace -e inject=lsetxattr,fsetxattr: error=EOPNOTSUPP`: cp src dst → exit 0 (GNU and uutils agree) cp -a src dst → exit 0 (suppressed by handle_preserve) cp --preserve=xattr src dst → exit 1 "setting attributes for 'X': Operation not supported" Pre-uutils#10014 uutils cp matched all three; post-uutils#10014 it silently exited 0 on the `--preserve=xattr` case, diverging from GNU. Restore the original strict semantics on `copy_xattrs` and `copy_xattrs_fd`, and add `copy_xattrs_ignore_unsupported` / `copy_xattrs_fd_ignore_unsupported` wrappers for the best-effort callers. mv's three call sites switch to the new wrappers — the behavior is identical to the previous `let _ =` discard, but the helper name now documents the intent so a future maintainer doesn't accidentally start surfacing the error.
sylvestre
force-pushed
the
fix/mv-cannot-overwrite-not-empty
branch
from
July 4, 2026 16:32
c1023eb to
8ea0c47
Compare
The rebase resolution in 28b11b9 dropped the pre-copy unlink of an existing cross-device destination, reasoning that fs::copy's O_WRONLY|O_CREAT|O_TRUNC truncating in place "matches GNU mv". It doesn't: GNU's copy.c explicitly unlinks any pre-existing destination before a cross-device copy so that the operation fails the same way a same-device rename() would when the destination directory forbids removing entries ("inter-device move failed ... unable to remove target"). Truncating in place instead only requires write permission on the destination *file*, silently bypassing that check and causing tests/mv/part-fail.sh (GNU testsuite) to regress: mv succeeded when it should have failed with Permission denied. Restore the unlink in rename_file_fallback. This does not reopen the symlink TOCTOU window uutils#10015 was about: the subsequent open of `to` already uses O_NOFOLLOW (via create_dest_restrictive, added after uutils#10015 as part of the fd-based xattr copy work), so an attacker recreating `to` as a symlink between the unlink and that open gets ELOOP rather than a followed write. Report the unlink failure via mv's own "inter-device move failed" diagnostic directly (show! + the empty-message "already reported" sentinel) instead of letting the generic "cannot move SRC to DEST" context wrap it a second time, which previously produced a garbled, doubly-wrapped message. Updates test_mv_cross_device_truncate_in_readonly_dir, which encoded the incorrect assumption from 28b11b9, to expect the correct failure instead. tests/cp/sparse-to-pipe.sh was also flagged as regressed by CI, but cp.rs and uucore's fsxattr.rs are byte-identical to origin/main on this branch, and the test passes consistently (5/5 runs) both before and after this change; it could not have been caused by this branch's diff and is most likely an unrelated CI flake.
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
2 participants
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.
No description provided.