Skip to content

fix(mv): don't destroy the destination when a cross-device move fails#13334

Open
abendrothj wants to merge 1 commit into
uutils:mainfrom
abendrothj:fix/mv-special-file-dest-loss
Open

fix(mv): don't destroy the destination when a cross-device move fails#13334
abendrothj wants to merge 1 commit into
uutils:mainfrom
abendrothj:fix/mv-special-file-dest-loss

Conversation

@abendrothj

Copy link
Copy Markdown
Contributor

Fixes #13145 (reported via security advisory GHSA-xw28-j282-p74c).

Problem

When mv falls back to copy+remove for a cross-filesystem (EXDEV) move, it removes an existing destination before it knows the source can be recreated there. For a special file like a socket this always fails — sockets fell through to the regular-file copy path, which cannot open them — so the destination was deleted and the move failed:

$ echo important > a
$ mv /path/on/tmpfs/sock a
mv: Permission denied (os error 13)
$ cat a
cat: a: No such file or directory      # 'a' was deleted

GNU mv recreates the socket at the destination instead. The same destroy-before-create ordering also affected fifos (mkfifo after removing the destination) and regular files (destination removed before the source was even opened, so an unreadable source cost the destination).

Fix

  • Special files are now recreated, like GNU (copy_special.c): fifos via mkfifo, sockets and device nodes via mknod, preserving ownership and permissions (the fifo path previously hardcoded mode 0666).
  • The destination can no longer be destroyed by a failure: the node is created under a random temporary name (same CuXXXXXX-from-/dev/urandom scheme as the existing create_symlink_replace, extracted into a shared helper) and then atomically renamed over the destination. If creation fails (e.g. mknod of a device node as non-root), the error propagates with the destination untouched. mknod/mkfifo fail with EEXIST on a planted symlink rather than following it, so the unlink-window symlink concerns from mv copy TOCTOU Race #10015 don't reappear here.
  • Sockets and device nodes inside directories moved across devices are recreated the same way; previously they were fed to fs::copy, failing the whole directory move.
  • Regular files: the source is now opened before the destination is touched, so a source that cannot be opened for reading (e.g. mode 000) no longer costs the destination.

Not addressed here (happy to follow up): the two-argument error path still prints without the cannot move 'src' to 'dest' context mentioned in the issue; wrapping it touches the stderr format of several unrelated error kinds, so it seemed better as a separate change.

Testing

New integration tests (Linux, /dev/shm pattern like the existing cross-device tests):

  • test_mv_cross_device_socket_replaces_dest — the exact GHSA repro: now succeeds, destination becomes the socket (was: destination deleted, move failed).
  • test_mv_cross_device_fifo_replaces_dest_and_preserves_mode — fifo replaces an existing destination and keeps its 0604 mode (was: hardcoded 0666).
  • test_mv_dir_with_socket_across_partitions — directory containing a socket survives the move (was: whole move failed).
  • test_mv_cross_device_unreadable_source_preserves_dest — failed move of an unreadable source leaves the destination intact (skipped as root).

Full mv suite passes on Linux (130 tests, run as an unprivileged user in a container) and macOS (105 tests). Also manually verified on macOS across a RAM-disk filesystem boundary, including the failure-preservation case: mknod(S_IFSOCK) needs root there, so the socket move fails cleanly and the destination survives — before this change it was deleted.

The cross-device (EXDEV) fallback removed an existing destination before
it knew the source could be recreated there, so a failed move destroyed
the destination:

- A socket or device node fell through to the regular-file copy path,
  which removed the destination and then failed to open the source,
  losing the destination entirely.
- A fifo was recreated only after the destination was removed, so a
  failed mkfifo also lost the destination. The fifo was also recreated
  with a hardcoded 0666 mode instead of the source's.
- A regular source that could not be opened for reading (e.g. mode 000)
  cost the destination as well, because the destination was removed
  before the source was opened.

Special files (fifos, sockets, and device nodes) are now recreated with
mkfifo/mknod like GNU mv does, preserving ownership and permissions. The
node is created under a random temporary name and renamed over the
destination, so a failure to create it can never destroy an existing
destination. Sockets and device nodes inside directories moved across
devices are recreated the same way instead of failing the whole move.
For regular files, the source is now opened before the destination is
touched.

Reported via security advisory GHSA-xw28-j282-p74c.

Fixes uutils#13145
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/rm/isatty (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/tty-eof (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/cut/bounded-memory is now being skipped but was previously passing.
Congrats! The gnu test tests/unexpand/bounded-memory is now passing!
Note: The gnu test tests/env/env-signal-handler was skipped on 'main' but is now failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mv: moving a special file across filesystems deletes the destination on failure

1 participant