Skip to content

fix(cp): don't delete dest before same-file check with --remove-destination#13158

Open
koopatroopa787 wants to merge 1 commit into
uutils:mainfrom
koopatroopa787:fix-cp-remove-destination-same-file
Open

fix(cp): don't delete dest before same-file check with --remove-destination#13158
koopatroopa787 wants to merge 1 commit into
uutils:mainfrom
koopatroopa787:fix-cp-remove-destination-same-file

Conversation

@koopatroopa787

Copy link
Copy Markdown
Contributor

Fixes #13144 (GHSA-9p9p-xh9v-6fvx).

Bug

$ echo hello > a
$ cp --remove-destination a ./a
cp: cannot stat 'a': No such file or directory
$ cat a
cat: a: No such file or directory      # file is gone

cp --remove-destination a ./a deletes the file before discovering that
source and destination refer to the same data, destroying it. GNU cp
refuses with cp: 'a' and './a' are the same file and leaves the file
intact.

Root cause

In copy_file, the block that removes dest ahead of time to let
--remove-destination recreate hard-linked destinations used source != dest (a raw Path comparison) as the guard against deleting the source
itself:

if are_hardlinks_to_same_file(source, dest)
    && source != dest
    && matches!(options.overwrite, OverwriteMode::Clobber(ClobberMode::RemoveDestination))
{
    fs::remove_file(dest)?;
}

"a" and "./a" are different Path values but the exact same
directory entry, so the guard doesn't catch this case and the file gets
unlinked before the later same-file check (in handle_existing_dest)
ever runs.

Fix

Compare canonicalized paths instead of the raw Paths. This still allows
deletion for the legitimate case this block exists for — two distinct
directory entries that are hard-linked to each other (e.g. cp --remove-destination file hardlink, covered by
test_remove_destination_with_destination_being_a_hardlink_to_source) —
but skips it when source and dest are actually the same entry, letting
the existing "same file" error fire instead.

Testing

Added test_remove_destination_same_path_does_not_destroy_file
(cross-platform) reproducing the exact GHSA repro. Ran the full cp test
suite (159 tests) with no regressions, plus cargo clippy -D warnings
and cargo fmt --check.

…nation

cp --remove-destination a ./a unlinked the file before detecting that
source and destination were the same path, destroying the only copy of
the data. The "source != dest" guard only compared the raw Path values,
so differently-spelled references to the same directory entry slipped
through.

Compare canonicalized paths instead, so the deletion still happens for
the legitimate case of two distinct directory entries hard-linked to
the same file (which must keep working for --remove-destination), but
is skipped when source and dest are actually the same entry.

Fixes GHSA-9p9p-xh9v-6fvx
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/cp/sparse-to-pipe. tests/cp/sparse-to-pipe is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/seq/seq-epipe is now being skipped but was previously passing.
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.

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.

cp --remove-destination deletes the file when source and dest are the same

1 participant