mv: use 'cannot overwrite TARGET: Directory not empty' for non-empty dir target#12124
Draft
sylvestre wants to merge 7 commits intouutils:mainfrom
Draft
mv: use 'cannot overwrite TARGET: Directory not empty' for non-empty dir target#12124sylvestre wants to merge 7 commits intouutils:mainfrom
sylvestre wants to merge 7 commits intouutils:mainfrom
Conversation
|
GNU testsuite comparison: |
This comment was marked as resolved.
This comment was marked as resolved.
uutils#10010) When moving a symlink across filesystems onto an existing destination, uutils failed with "File exists" because the EXDEV fallback called `unix::fs::symlink(target, to)` without handling the case where `to` already exists. GNU mv replaces the destination. In `rename_symlink_fallback`, catch `AlreadyExists` and fall through to a create-then-rename helper: create the symlink at a unique temp name in the destination's parent directory, then `fs::rename` it into place. `rename(2)` is atomic on Linux, so a concurrent observer never sees the destination path missing, and an attacker-controlled symlink race at the destination cannot redirect the subsequent operations.
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.
`Mode::from_bits_truncate(DEST_INITIAL_MODE)` failed to build on macOS because `mode_t` is `u16` there but `u32` on Linux. Compose the mode from `Mode::RUSR | Mode::WUSR` so the call is portable by construction and self-documents the intent (read+write owner = 0o600).
sylvestre
force-pushed
the
fix/mv-cannot-overwrite-not-empty
branch
from
c3fa3d3 to
c1023eb
Compare
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.