Repository navigation
mv: recreate special files when moving them across devices - #13334
abendrothj wants to merge 6 commits into
Conversation
|
GNU testsuite comparison: |
da75e2c to
eaf69db
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes mv cross-device (EXDEV) fallback behavior to prevent data loss when moving special files (e.g., sockets/fifos/device nodes) and when the source cannot be opened. It updates the mv implementation to recreate special files and to avoid destroying an existing destination unless replacement is guaranteed.
Changes:
- Recreate FIFOs/sockets/device nodes during EXDEV fallback (instead of attempting content copy) and preserve metadata.
- Replace destinations via temp-name creation + atomic rename to avoid leaving the destination missing on failure.
- Add Linux integration tests covering socket/fifo replacement, directory-with-socket moves, and unreadable source preservation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/by-util/test_mv.rs | Adds regression/integration tests for cross-device special-file moves and destination-preservation behavior. |
| src/uu/mv/src/mv.rs | Implements special-file recreation and safer destination replacement logic in EXDEV fallback paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let parent = to | ||
| .parent() | ||
| .filter(|p| !p.as_os_str().is_empty()) | ||
| .unwrap_or_else(|| Path::new(".")); | ||
|
|
||
| let mut urandom = fs::File::open("/dev/urandom")?; | ||
|
|
||
| for _ in 0..32 { | ||
| let tmp_bytes = random_temp_name(&mut urandom)?; | ||
| let tmp = parent.join(OsStr::from_bytes(&tmp_bytes)); | ||
|
|
||
| match copy_special_file(from, metadata, &tmp) { | ||
| Ok(()) => { | ||
| if let Err(e) = fs::rename(&tmp, to) { | ||
| let _ = fs::remove_file(&tmp); |
There was a problem hiding this comment.
Done in 88fd411, though not with O_NOFOLLOW on the parent: it's opened once with DirFd::open_anchor, which follows symlinks like a normal lookup (see the comment below). The node is created in a private staging directory under it and renamed over the destination relative to that descriptor, and cleanup goes through the same descriptor.
| // file that was distributed with this source code. | ||
| // | ||
| // spell-checker:ignore mydir hardlinked tmpfs notty unwriteable myfolder SRCDATA DSTDATA REALDATA | ||
| // spell-checker:ignore mydir hardlinked tmpfs notty unwriteable GHSA |
There was a problem hiding this comment.
The ignore list keeps myfolder, SRCDATA, DSTDATA and REALDATA now (and adds Nofile), so nothing used later was dropped.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/uu/mv/src/mv.rs:1623
open_destination_parent(from)opens the source’s parent directory withSymlinkBehavior::NoFollow. That makes cross-device moves fail (before touching the destination) when the source path is under a symlinked directory (e.g.mv link/file destwherelink -> realdir). Previously the source was removed viafs::remove_file(from), which follows symlinks in parent components as normal path resolution does.
Consider opening the source parent with SymlinkBehavior::Follow (while keeping NoFollow for the destination side) so normal mv semantics continue to work for sources located under symlinked directories.
#[cfg(all(unix, not(target_os = "redox")))]
let (src_parent_fd, src_basename) = open_destination_parent(from)
.map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
src/uu/mv/src/mv.rs:1121
rename_special_fallbackusesopen_destination_parent(from)for the source cleanup path. Sinceopen_destination_parentintentionally opens parents withSymlinkBehavior::NoFollow, this makes cross-device moves of special files fail if the source lives under a symlinked directory (even though opening/reading the source itself follows symlinks in parent components).
Recommendation: keep NoFollow for the destination parent (security), but open the source parent with SymlinkBehavior::Follow so source paths under symlinked directories continue to work as they did previously.
This issue also appears on line 1621 of the same file.
let (dir_fd, basename) = open_destination_parent(to)?;
let (src_parent_fd, src_basename) = open_destination_parent(from)?;
let basename_cstr = CString::new(basename.as_bytes())
Merging this PR will degrade performance by 12.3%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
|
this is a lot of cfg for redox |
|
Yes, I think the Redox-specific implementations should move into
The extraction will move the actual Redox behavior while leaving only small dispatch points and shared helpers, such as This keeps the change focused without introducing a broader platform abstraction than necessary. |
|
Infrastructure failure with precommit.ci |
f6ec44e to
44dd7d7
Compare
44dd7d7 to
c05f50e
Compare
|
Redox code is out of Two other things from the rebase onto main. The branch's local The failing musl job isn't from this PR: it's |
cb7be93 to
35ac41f
Compare
| let basename = to | ||
| .file_name() | ||
| .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "invalid destination path"))?; | ||
| let dir_fd = DirFd::open(parent, SymlinkBehavior::NoFollow)?; |
There was a problem hiding this comment.
with NoFollow, mv sock /dev/shm/link/dest fails when link is a symlink to a dir, no?
replace_link in uucore follows the parent on purpose. please add a test for this case
There was a problem hiding this comment.
Yes, it did. The parent is followed now, through DirFd::open_anchor(parent) like replace_link. test_mv_cross_device_special_file_into_symlinked_parent covers it, replacing a socket and creating a FIFO through a symlinked directory.
| /// Open the source's parent directory following symlinks as normal path | ||
| /// resolution does, while returning a pinned directory fd for source removal. | ||
| #[cfg(all(unix, not(target_os = "redox")))] | ||
| fn open_source_parent(from: &Path) -> io::Result<(DirFd, OsString)> { |
There was a problem hiding this comment.
almost the same function as open_destination_parent, could be dedup, no?
There was a problem hiding this comment.
Both are gone. The one remaining parent open is DirFd::open_anchor in uucore, so mv has no wrapper of its own.
| /// Recreate the source special file (fifo, socket, or device node) at `to`, | ||
| /// preserving its ownership and permissions. | ||
| #[cfg(all(unix, not(target_os = "redox")))] | ||
| fn copy_special_file(_from: &Path, metadata: &fs::Metadata, to: &Path) -> io::Result<()> { |
There was a problem hiding this comment.
_from is unused here, do we need it in the signature?
There was a problem hiding this comment.
Removed, it's copy_special_file(to, metadata) now.
|
this is very complex, I think it would benefit to be smaller |
| } | ||
| Err(io::Error::new( | ||
| io::ErrorKind::AlreadyExists, | ||
| "could not allocate a unique temp name in destination directory", |
There was a problem hiding this comment.
please use translate!() (same string is duplicated in redox.rs)
There was a problem hiding this comment.
The temp-name exhaustion moved into uucore's create_temp_at, which uses translate!("error-no-unique-temp-name") (en-US and fr-FR), and redox.rs is gone, so the string is in one place.
35ac41f to
88fd411
Compare
abendrothj
changed the title
|
I split it up. The shared pieces are separate PRs: About redox.rs: I went the other way from what I said earlier. The module is gone, and aix, hurd and redox get one inline |
Open a directory readable, and on EACCES retry with O_PATH or O_SEARCH, which anchor *at calls without read access. The targets with either flag are named by the has_o_path and has_o_search cfg aliases. If the retry fails, its own error is returned.
Move the temporary name and renameat logic of replace_link into replace_entry_at, which creates the entry through a callback relative to an open directory, so other callers can replace entries the same way. The parent is now opened with DirFd::open_anchor, so ln -sf replacing a link in a directory with write and search but no read permission works, as it does with GNU, instead of failing with EACCES.
Create fifos, sockets and device nodes relative to an open directory. nix and rustix do not provide mknodat on Apple targets, so this calls libc directly.
A cross-device move of a socket or device node went through the regular file copy, which removed the destination and then failed to open the source. A fifo also removed the destination before creating the new one, and lost its mode. Fifos, sockets and device nodes are now created with their mode and ownership in a private directory next to the destination and renamed over it, so the destination is kept if the node cannot be created, and the ownership and mode cannot land on another file linked over the destination name meanwhile. As for regular files, setuid and setgid are dropped when the ownership cannot be kept. Fixes uutils#13145
The private directory that a special file is created in was made under a temporary umask of 077, so that the owner could create the node inside whatever the umask. Change its mode to 0700 by name right after creating it instead, before opening it: the check after the open still rejects a directory moved there in between, and mv no longer changes the process-wide umask or needs uucore's mode feature.
ccd87db to
7c744c8
Compare
|
one more commit: the staging directory gets its 0700 from a chmod right after the mkdir, instead of a temporary umask of 077, so mv no longer changes the process umask and this no longer needs #15121. |
A cross-device
mvof a socket or device node went through the regular file copy, which removed the destination and then failed to open the source. A FIFO also removed the destination before the new one was created, and lost its mode.FIFOs, sockets and device nodes are now created with their mode and ownership in a private directory next to the destination, then renamed over it relative to the parent's descriptor. If the node can't be created, the destination is left alone. The parent is opened with
DirFd::open_anchor, so symlinked and write/search-only parents work. As with regular files, setuid and setgid are dropped when ownership can't be kept. AddsDirFd::mknod_at.On aix, hurd and redox there's a simpler inline fallback that removes the destination first, without the staging.
Special files inside a directory moved across devices are in #15123.
Based on #15120 and #15122; until they merge, the diff here shows their commits too.
Closes #13145