diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 41872be659..3bc6a76691 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -2157,11 +2157,22 @@ fn delete_dest_if_needed_and_allowed( OverwriteMode::Clobber(cl) | OverwriteMode::Interactive(cl) => { match cl { ClobberMode::Force => { - // TODO - // Using `readonly` here to check if `dest` needs to be deleted is not correct: - // "On Unix-based platforms this checks if any of the owner, group or others write permission bits are set. It does not check if the current user is in the file's assigned group. It also does not check ACLs. Therefore the return value of this function cannot be relied upon to predict whether attempts to read or write the file will actually succeed." - // This results in some copy operations failing, because this necessary deletion is being skipped. - is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly() + if is_symlink_loop(dest) { + true + } else { + let dest_metadata = fs::metadata(dest)?; + if dest_metadata.is_file() { + // Determine whether `dest` needs to be removed before + // copying by trying to open it for writing. + OpenOptions::new().write(true).open(dest).is_err() + } else { + // For non-regular destinations (FIFOs, sockets, + // devices) an open-for-write probe can block + // indefinitely (e.g. opening a FIFO with no reader) + // Fall back to the permission-bit check instead of actually opening. + dest_metadata.permissions().readonly() + } + } } ClobberMode::RemoveDestination => true, ClobberMode::Standard => { diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 2342d4c667..23bd0716c1 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -801,13 +801,21 @@ fn test_cp_arg_interactive_verbose_clobber() { #[test] #[cfg(unix)] fn test_cp_f_i_verbose_non_writeable_destination_y() { + use rustix::process::geteuid; + + // A privileged process can write to a 000-mode file regardless of its + // permission bits, so -f never needs to remove and recreate it + if geteuid().is_root() { + return; + } + let (at, mut ucmd) = at_and_ucmd!(); at.touch("a"); at.touch("b"); // Non-writeable file - at.set_mode("b", 0o0000); + rustix::fs::chmod(at.plus("b"), rustix::fs::Mode::from_bits_truncate(0o000)).unwrap(); ucmd.args(&["-f", "-i", "--verbose", "a", "b"]) .pipe_in("y") @@ -833,6 +841,35 @@ fn test_cp_f_i_verbose_non_writeable_destination_empty() { .stderr_only("cp: replace 'b', overriding mode 0000 (---------)? "); } +#[test] +#[cfg(unix)] +fn test_cp_f_preserves_dest_mode_when_writable_by_privilege() { + use rustix::process::geteuid; + + // A privileged process can write to a file regardless of its permission + // bits, so `cp -f` must not unlink and recreate such a destination: GNU + // cp only removes the destination when it genuinely cannot be opened for + // writing. Removing it needlessly loses the destination's original mode + // (e.g. resets `000` to the umask-derived default). + if !geteuid().is_root() { + return; + } + + let (at, mut ucmd) = at_and_ucmd!(); + + at.write("a", "s"); + at.write("b", "d"); + rustix::fs::chmod(at.plus("b"), rustix::fs::Mode::from_bits_truncate(0o000)).unwrap(); + + ucmd.args(&["-f", "a", "b"]).succeeds(); + + assert_eq!(at.read("b"), "s"); + assert_eq!( + rustix::fs::stat(at.plus("b")).unwrap().st_mode & 0o777, + 0o000 + ); +} + #[test] #[cfg(target_os = "linux")] fn test_cp_arg_link() {