From a84e49e08500eb7c28e588b0a9ab0b425de4cddf Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 30 Aug 2026 10:29:46 +0200 Subject: [PATCH 1/3] cp: open the source once per copy Every regular-file copy on Linux opened the source three times: once in check_for_data, once in check_sparse_detection, then again in the copy itself. Open it once and thread the descriptor through the probe and the copy, so the strategy and the bytes come from the same fd. before: open("src", O_RDONLY) x3 + open("dst", O_WRONLY|O_CREAT) x1 after: open("src", O_RDONLY) x1 + open("dst", O_WRONLY|O_CREAT) x1 Copying 3000 small files, 50 runs: 199.9ms +/- 14.9 -> 171.7ms +/- 23.6. The ratio's error bars overlap; the stable signal is system time, 177.3ms -> 146.9ms, with user time unchanged at ~18ms. --- src/uu/cp/src/platform/linux.rs | 232 +++++++++++++++----------------- tests/by-util/test_cp.rs | 30 ++++- 2 files changed, 136 insertions(+), 126 deletions(-) diff --git a/src/uu/cp/src/platform/linux.rs b/src/uu/cp/src/platform/linux.rs index 98afa6d95a8..fc32a4be5da 100644 --- a/src/uu/cp/src/platform/linux.rs +++ b/src/uu/cp/src/platform/linux.rs @@ -22,26 +22,15 @@ use crate::{ is_stream, }; -// Replacement for `std::fs::copy` that uses the safe-copy primitives but -// only applies `O_NOFOLLOW` to the *source* open. The destination is -// followed if it is a pre-existing symlink, matching GNU cp -d/-P which -// only forbid dereferencing on the source side. -fn fs_copy(source: P, dest: Q, source_nofollow: bool, context: &str) -> CopyResult<()> -where - P: AsRef, - Q: AsRef, -{ - let mut src = open_source(source, source_nofollow) - .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - let mut dst = create_dest_restrictive(&dest, false).map_err(|e| { +// Create the destination. It is followed when it is a pre-existing symlink, +// matching GNU cp -d/-P which only forbid dereferencing on the source side. +fn create_dest(dest: &Path) -> CopyResult { + create_dest_restrictive(dest, false).map_err(|e| { CpError::IoErrContext( e, - translate!("cp-error-cannot-create-regular-file", "path" => dest.as_ref().quote()), + translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()), ) - })?; - buf_copy::copy_fast(&mut src, &mut dst) - .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - Ok(()) + }) } /// The fallback behavior for [`clone`] on failed system call. @@ -69,87 +58,89 @@ enum CloneFallback { enum CopyMethod { /// Do a sparse copy SparseCopy, - /// Use [`std::fs::copy`]. + /// Copy the bytes with [`buf_copy::copy_fast`]. FSCopy, /// Default (can either be [`CopyMethod::SparseCopy`] or [`CopyMethod::FSCopy`]) Default, - /// Use [`sparse_copy_without_hole`] + /// Use [`sparse_copy_without_hole_fd`] SparseCopyWithoutHole, } /// Use the Linux `ioctl_ficlone` API to do a copy-on-write clone. /// /// `fallback` controls what to do if the system call fails. -fn clone

( - source: P, - dest: P, +fn clone( + src_file: &mut File, + dest: &Path, fallback: CloneFallback, - nofollow: bool, context: &str, -) -> CopyResult<()> -where - P: AsRef, -{ +) -> CopyResult<()> { // Only needed to decide whether a failed --reflink=always clone should // clean up the dest, so skip the lstat for the other fallbacks. - let dest_existed = - matches!(fallback, CloneFallback::Error) && dest.as_ref().symlink_metadata().is_ok(); - let mut src_file = - open_source(&source, nofollow).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - let mut dst_file = create_dest_restrictive(&dest, false).map_err(|e| { - CpError::IoErrContext( - e, - translate!("cp-error-cannot-create-regular-file", "path" => dest.as_ref().quote()), - ) - })?; - if let Err(err) = ioctl_ficlone(&dst_file, &src_file) { + let dest_existed = matches!(fallback, CloneFallback::Error) && dest.symlink_metadata().is_ok(); + let mut dst_file = create_dest(dest)?; + if let Err(err) = ioctl_ficlone(&dst_file, &*src_file) { // Reuse the already-open descriptors: the dest was just created with // a restrictive mode that the umask may have stripped of write bits, // so re-opening it by path can fail with EACCES (LP: #2164777). return match fallback { CloneFallback::Error => { // GNU cp removes a dest it created itself, but keeps a - // pre-existing (now truncated) one. - if !dest_existed { - let _ = std::fs::remove_file(&dest); + // pre-existing (now truncated) one. Only unlink while the + // path still names the inode we created: it may have been + // replaced since, and removing blindly would drop an + // unrelated file. + if !dest_existed && path_still_refers_to(dest, &dst_file) { + let _ = std::fs::remove_file(dest); } Err(CpError::IoErrContext(err.into(), context.to_owned())) } - CloneFallback::FSCopy => buf_copy::copy_fast(&mut src_file, &mut dst_file) + CloneFallback::FSCopy => buf_copy::copy_fast(src_file, &mut dst_file) .map_err(|e| CpError::IoErrContext(e, context.to_owned())), - CloneFallback::SparseCopy => sparse_copy_fd(&mut src_file, &dst_file, context), + CloneFallback::SparseCopy => sparse_copy_fd(src_file, &dst_file, context), CloneFallback::SparseCopyWithoutHole => { - sparse_copy_without_hole_fd(&src_file, &dst_file, context) + sparse_copy_without_hole_fd(src_file, &dst_file, context) } }; } Ok(()) } +/// Whether `path` still resolves to the inode behind `file`. +fn path_still_refers_to(path: &Path, file: &File) -> bool { + let (Ok(current), Ok(opened)) = (path.symlink_metadata(), file.metadata()) else { + return false; + }; + current.dev() == opened.dev() && current.ino() == opened.ino() +} + /// Checks whether a file contains any non null bytes i.e. any byte != 0x0 /// This function returns a tuple of (bool, u64, u64) signifying a tuple of (whether a file has /// data, its size, no of blocks it has allocated in disk) -fn check_for_data(source: &Path, nofollow: bool) -> io::Result<(bool, u64, u64)> { - let mut src_file = open_source(source, nofollow)?; +fn check_for_data(src_file: &mut File) -> io::Result<(bool, u64, u64)> { let metadata = src_file.metadata()?; let size = metadata.size(); let blocks = metadata.blocks(); // checks edge case of virtual files in /proc which have a size of zero but contains data - if size == 0 { + let (has_data, blocks) = if size == 0 { let mut buf: Vec = vec![0; metadata.blksize() as usize]; // Directly use metadata.blksize() - let _ = src_file.read(&mut buf)?; - return Ok((buf.iter().any(|&x| x != 0x0), size, 0)); - } - let has_data = seek(src_file, SeekFrom::Data(0)).is_ok(); + let read = src_file.read(&mut buf)?; + (buf[..read].iter().any(|&x| x != 0x0), 0) + } else { + (seek(&*src_file, SeekFrom::Data(0)).is_ok(), blocks) + }; + + // The probe moved the descriptor; the copy that follows reads it + // sequentially, so hand it back positioned at the start. + seek(&*src_file, SeekFrom::Start(0))?; Ok((has_data, size, blocks)) } /// Checks whether a file is sparse i.e. it contains holes, uses the crude heuristic blocks < size / 512 /// Reference:`` -fn check_sparse_detection(source: &Path, nofollow: bool) -> io::Result { - let src_file = open_source(source, nofollow)?; +fn check_sparse_detection(src_file: &File) -> io::Result { let metadata = src_file.metadata()?; let size = metadata.size(); let blocks = metadata.blocks(); @@ -157,23 +148,8 @@ fn check_sparse_detection(source: &Path, nofollow: bool) -> io::Result { Ok(blocks < size / 512) } -/// Optimized [`sparse_copy`] doesn't create holes for large sequences of zeros in non `sparse_files` +/// Optimized [`sparse_copy_fd`] doesn't create holes for large sequences of zeros in non `sparse_files` /// Used when `--sparse=auto` -fn sparse_copy_without_hole

(source: P, dest: P, nofollow: bool, context: &str) -> CopyResult<()> -where - P: AsRef, -{ - let src_file = - open_source(&source, nofollow).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - let dst_file = create_dest_restrictive(&dest, false).map_err(|e| { - CpError::IoErrContext( - e, - translate!("cp-error-cannot-create-regular-file", "path" => dest.as_ref().quote()), - ) - })?; - sparse_copy_without_hole_fd(&src_file, &dst_file, context) -} - fn sparse_copy_without_hole_fd(src_file: &File, dst_file: &File, context: &str) -> CopyResult<()> { let ctx_err = |e: io::Error| CpError::IoErrContext(e, context.to_owned()); @@ -212,21 +188,6 @@ fn sparse_copy_without_hole_fd(src_file: &File, dst_file: &File, context: &str) } /// Perform a sparse copy from one file to another. /// Creates a holes for large sequences of zeros in `non_sparse_files`, used for `--sparse=always` -fn sparse_copy

(source: P, dest: P, nofollow: bool, context: &str) -> CopyResult<()> -where - P: AsRef, -{ - let mut src_file = - open_source(&source, nofollow).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - let dst_file = create_dest_restrictive(&dest, false).map_err(|e| { - CpError::IoErrContext( - e, - translate!("cp-error-cannot-create-regular-file", "path" => dest.as_ref().quote()), - ) - })?; - sparse_copy_fd(&mut src_file, &dst_file, context) -} - fn sparse_copy_fd(src_file: &mut File, dst_file: &File, context: &str) -> CopyResult<()> { let ctx_err = |e: io::Error| CpError::IoErrContext(e, context.to_owned()); @@ -321,6 +282,12 @@ where } /// Copies `source` to `dest` using copy-on-write if possible. +/// +/// The source is opened once and the descriptor is threaded through both the +/// sparseness probe and the copy itself. Probing and copying used to open the +/// path separately, costing three opens per copy and letting the strategy be +/// decided from one file while the bytes came from whatever the path named +/// later (#13185). pub(crate) fn copy_on_write( source: &Path, dest: &Path, @@ -344,16 +311,20 @@ pub(crate) fn copy_on_write( copy_debug.offload = OffloadReflinkDebug::Avoided; copy_stream(source, dest, nofollow, context) } else { + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; let mut copy_method = CopyMethod::Default; - let result = handle_reflink_never_sparse_always(source, dest, nofollow); + let result = handle_reflink_never_sparse_always(&mut src_file, dest); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; } + let mut dst_file = create_dest(dest)?; match copy_method { - CopyMethod::FSCopy => fs_copy(source, dest, nofollow, context), - _ => sparse_copy(source, dest, nofollow, context), + CopyMethod::FSCopy => buf_copy::copy_fast(&mut src_file, &mut dst_file) + .map_err(|e| CpError::IoErrContext(e, context.to_owned())), + _ => sparse_copy_fd(&mut src_file, &dst_file, context), } } } @@ -364,11 +335,15 @@ pub(crate) fn copy_on_write( copy_debug.offload = OffloadReflinkDebug::Avoided; copy_stream(source, dest, nofollow, context) } else { - let result = handle_reflink_never_sparse_never(source, nofollow); + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + let result = handle_reflink_never_sparse_never(&mut src_file); if let Ok(debug) = result { copy_debug = debug; } - fs_copy(source, dest, nofollow, context) + let mut dst_file = create_dest(dest)?; + buf_copy::copy_fast(&mut src_file, &mut dst_file) + .map_err(|e| CpError::IoErrContext(e, context.to_owned())) } } (ReflinkMode::Never, SparseMode::Auto) => { @@ -378,18 +353,22 @@ pub(crate) fn copy_on_write( copy_debug.offload = OffloadReflinkDebug::Avoided; copy_stream(source, dest, nofollow, context) } else { + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; let mut copy_method = CopyMethod::Default; - let result = handle_reflink_never_sparse_auto(source, dest, nofollow); + let result = handle_reflink_never_sparse_auto(&mut src_file, dest); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; } + let mut dst_file = create_dest(dest)?; match copy_method { CopyMethod::SparseCopyWithoutHole => { - sparse_copy_without_hole(source, dest, nofollow, context) + sparse_copy_without_hole_fd(&src_file, &dst_file, context) } - _ => fs_copy(source, dest, nofollow, context), + _ => buf_copy::copy_fast(&mut src_file, &mut dst_file) + .map_err(|e| CpError::IoErrContext(e, context.to_owned())), } } } @@ -400,8 +379,10 @@ pub(crate) fn copy_on_write( copy_debug.offload = OffloadReflinkDebug::Avoided; copy_stream(source, dest, nofollow, context) } else { + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; let mut copy_method = CopyMethod::Default; - let result = handle_reflink_auto_sparse_always(source, dest, nofollow); + let result = handle_reflink_auto_sparse_always(&mut src_file, dest); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; @@ -409,9 +390,9 @@ pub(crate) fn copy_on_write( match copy_method { CopyMethod::FSCopy => { - clone(source, dest, CloneFallback::FSCopy, nofollow, context) + clone(&mut src_file, dest, CloneFallback::FSCopy, context) } - _ => clone(source, dest, CloneFallback::SparseCopy, nofollow, context), + _ => clone(&mut src_file, dest, CloneFallback::SparseCopy, context), } } } @@ -422,12 +403,14 @@ pub(crate) fn copy_on_write( copy_debug.offload = OffloadReflinkDebug::Avoided; copy_stream(source, dest, nofollow, context) } else { - let result = handle_reflink_auto_sparse_never(source, nofollow); + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + let result = handle_reflink_auto_sparse_never(&mut src_file); if let Ok(debug) = result { copy_debug = debug; } - clone(source, dest, CloneFallback::FSCopy, nofollow, context) + clone(&mut src_file, dest, CloneFallback::FSCopy, context) } } (ReflinkMode::Auto, SparseMode::Auto) => { @@ -435,8 +418,10 @@ pub(crate) fn copy_on_write( copy_debug.offload = OffloadReflinkDebug::Unsupported; copy_stream(source, dest, nofollow, context) } else { + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; let mut copy_method = CopyMethod::Default; - let result = handle_reflink_auto_sparse_auto(source, dest, nofollow); + let result = handle_reflink_auto_sparse_auto(&mut src_file, dest); if let Ok((debug, method)) = result { copy_debug = debug; copy_method = method; @@ -444,13 +429,12 @@ pub(crate) fn copy_on_write( match copy_method { CopyMethod::SparseCopyWithoutHole => clone( - source, + &mut src_file, dest, CloneFallback::SparseCopyWithoutHole, - nofollow, context, ), - _ => clone(source, dest, CloneFallback::FSCopy, nofollow, context), + _ => clone(&mut src_file, dest, CloneFallback::FSCopy, context), } } } @@ -459,7 +443,9 @@ pub(crate) fn copy_on_write( copy_debug.sparse_detection = SparseDebug::No; copy_debug.reflink = OffloadReflinkDebug::Yes; - clone(source, dest, CloneFallback::Error, nofollow, context) + let mut src_file = open_source(source, nofollow) + .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + clone(&mut src_file, dest, CloneFallback::Error, context) } (ReflinkMode::Always, _) => { return Err(translate!("cp-error-reflink-always-sparse-auto").into()); @@ -472,9 +458,8 @@ pub(crate) fn copy_on_write( /// Handles debug results when flags are "--reflink=auto" and "--sparse=always" and specifies what /// type of copy should be used fn handle_reflink_auto_sparse_always( - source: &Path, + src_file: &mut File, dest: &Path, - nofollow: bool, ) -> io::Result<(CopyDebug, CopyMethod)> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -482,8 +467,8 @@ fn handle_reflink_auto_sparse_always( sparse_detection: SparseDebug::Zeros, }; let mut copy_method = CopyMethod::Default; - let (data_flag, size, blocks) = check_for_data(source, nofollow)?; - let sparse_flag = check_sparse_detection(source, nofollow)?; + let (data_flag, size, blocks) = check_for_data(src_file)?; + let sparse_flag = check_sparse_detection(src_file)?; if data_flag || size < 512 { copy_debug.offload = OffloadReflinkDebug::Avoided; @@ -508,14 +493,14 @@ fn handle_reflink_auto_sparse_always( /// Handles debug results when flags are "--reflink=auto" and "--sparse=auto" and specifies what /// type of copy should be used -fn handle_reflink_never_sparse_never(source: &Path, nofollow: bool) -> io::Result { +fn handle_reflink_never_sparse_never(src_file: &mut File) -> io::Result { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, reflink: OffloadReflinkDebug::No, sparse_detection: SparseDebug::No, }; - let (data_flag, size, _blocks) = check_for_data(source, nofollow)?; - let sparse_flag = check_sparse_detection(source, nofollow)?; + let (data_flag, size, _blocks) = check_for_data(src_file)?; + let sparse_flag = check_sparse_detection(src_file)?; if sparse_flag { copy_debug.sparse_detection = SparseDebug::SeekHole; @@ -528,16 +513,16 @@ fn handle_reflink_never_sparse_never(source: &Path, nofollow: bool) -> io::Resul } /// Handles debug results when flags are "--reflink=auto" and "--sparse=never", files will be copied -/// through cloning them with fallback switching to [`std::fs::copy`] -fn handle_reflink_auto_sparse_never(source: &Path, nofollow: bool) -> io::Result { +/// through cloning them with fallback switching to [`buf_copy::copy_fast`] +fn handle_reflink_auto_sparse_never(src_file: &mut File) -> io::Result { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, reflink: OffloadReflinkDebug::No, sparse_detection: SparseDebug::No, }; - let (data_flag, size, _blocks) = check_for_data(source, nofollow)?; - let sparse_flag = check_sparse_detection(source, nofollow)?; + let (data_flag, size, _blocks) = check_for_data(src_file)?; + let sparse_flag = check_sparse_detection(src_file)?; if sparse_flag { copy_debug.sparse_detection = SparseDebug::SeekHole; @@ -552,9 +537,8 @@ fn handle_reflink_auto_sparse_never(source: &Path, nofollow: bool) -> io::Result /// Handles debug results when flags are "--reflink=auto" and "--sparse=auto" and specifies what /// type of copy should be used fn handle_reflink_auto_sparse_auto( - source: &Path, + src_file: &mut File, dest: &Path, - nofollow: bool, ) -> io::Result<(CopyDebug, CopyMethod)> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -563,8 +547,8 @@ fn handle_reflink_auto_sparse_auto( }; let mut copy_method = CopyMethod::Default; - let (data_flag, size, blocks) = check_for_data(source, nofollow)?; - let sparse_flag = check_sparse_detection(source, nofollow)?; + let (data_flag, size, blocks) = check_for_data(src_file)?; + let sparse_flag = check_sparse_detection(src_file)?; if (data_flag && size != 0) || (size > 0 && size < 512) { copy_debug.offload = OffloadReflinkDebug::Yes; @@ -596,9 +580,8 @@ fn handle_reflink_auto_sparse_auto( /// Handles debug results when flags are "--reflink=never" and "--sparse=auto" and specifies what /// type of copy should be used fn handle_reflink_never_sparse_auto( - source: &Path, + src_file: &mut File, dest: &Path, - nofollow: bool, ) -> io::Result<(CopyDebug, CopyMethod)> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -606,8 +589,8 @@ fn handle_reflink_never_sparse_auto( sparse_detection: SparseDebug::No, }; - let (data_flag, size, blocks) = check_for_data(source, nofollow)?; - let sparse_flag = check_sparse_detection(source, nofollow)?; + let (data_flag, size, blocks) = check_for_data(src_file)?; + let sparse_flag = check_sparse_detection(src_file)?; let mut copy_method = CopyMethod::Default; if data_flag || size < 512 { @@ -633,9 +616,8 @@ fn handle_reflink_never_sparse_auto( /// Handles debug results when flags are "--reflink=never" and "--sparse=always" and specifies what /// type of copy should be used fn handle_reflink_never_sparse_always( - source: &Path, + src_file: &mut File, dest: &Path, - nofollow: bool, ) -> io::Result<(CopyDebug, CopyMethod)> { let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, @@ -644,8 +626,8 @@ fn handle_reflink_never_sparse_always( }; let mut copy_method = CopyMethod::SparseCopy; - let (data_flag, size, blocks) = check_for_data(source, nofollow)?; - let sparse_flag = check_sparse_detection(source, nofollow)?; + let (data_flag, size, blocks) = check_for_data(src_file)?; + let sparse_flag = check_sparse_detection(src_file)?; if data_flag || size < 512 { copy_debug.offload = OffloadReflinkDebug::Avoided; diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index bd81c5f76f2..f216f52a3be 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -4,7 +4,7 @@ // file that was distributed with this source code. // spell-checker:ignore (flags) reflink (fs) tmpfs (linux) filefrag rlimit Rlim NOFILE clob btrfs neve ROOTDIR USERDIR outfile subvolume uufs xattrs ELOOP -// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel prwx doesnotexist reftests subdirs mksocket srwx dstlink +// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel prwx doesnotexist reftests subdirs mksocket srwx dstlink mcstransd #[cfg(unix)] use rstest::rstest; use uucore::display::Quotable; @@ -5611,6 +5611,34 @@ fn test_cp_debug_sparse_never_zero_sized_virtual_file() { .stdout_contains("copy offload: avoided, reflink: no, sparse detection: no"); } +// A file that stats as zero-sized but still returns data (/proc entries) +// is probed for content by reading from it. The copy reuses that same +// descriptor, so it has to start over at offset 0 -- otherwise everything +// the probe consumed is dropped and the destination ends up empty. The +// --debug tests above only look at the reported strategy, not the bytes. +#[test] +#[cfg(target_os = "linux")] +fn test_cp_zero_sized_virtual_file_contents() { + let expected = std::fs::read_to_string("/proc/version").unwrap(); + assert!(!expected.is_empty()); + + for extra in [ + &[][..], + &["--sparse=never"], + &["--sparse=always"], + &["--sparse=auto"], + &["--reflink=never"], + &["--reflink=never", "--sparse=always"], + ] { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(extra) + .args(&["/proc/version", "copied"]) + .succeeds() + .no_output(); + assert_eq!(at.read("copied"), expected, "with {extra:?}"); + } +} + #[test] #[cfg(target_os = "linux")] fn test_cp_debug_default_zero_sized_virtual_file() { From 2ae8396d12cf20b9913ecbe4cea28afb9ad432c5 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 30 Aug 2026 13:38:08 +0200 Subject: [PATCH 2/3] cp: check the single source open at the syscall level New util/check-open-once.sh straces cp and asserts the source path is opened exactly once, across the default, --sparse=always and --sparse=never paths. Verified it fails on the previous code with 'opened the source 3 times', so it guards the invariant rather than restating it. Kept out of check-toctou.sh: no exploit is known for the old window -- every open used the same credentials and flags -- and the security scripts should stay about attacker-exploitable splits. The new header says so and points at the two that do. Traces open(2) as well as openat(2): rustix issues the former on Linux, so an openat-only filter matches nothing and passes vacuously. --- .github/workflows/CICD.yml | 17 +++++++++++ util/check-open-once.sh | 58 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+) create mode 100755 util/check-open-once.sh diff --git a/.github/workflows/CICD.yml b/.github/workflows/CICD.yml index e5d905a4a46..c0ca5f40741 100644 --- a/.github/workflows/CICD.yml +++ b/.github/workflows/CICD.yml @@ -1089,6 +1089,23 @@ jobs: - name: Run libc interposition verification run: ./util/check-libc-interposition.sh + test_open_once: + name: Redundant Open Check + runs-on: ubuntu-latest + needs: [ min_version, deps ] + + steps: + - uses: actions/checkout@v7.0.1 + with: + persist-credentials: false + - uses: Swatinem/rust-cache@v2 + - name: Install strace + run: sudo apt-get update && sudo apt-get install -y strace + - name: Build utilities for redundant open checks + run: cargo build --profile=release-small -p uu_cp + - name: Run redundant open verification + run: ./util/check-open-once.sh + test_toctou: name: TOCTOU Security Check runs-on: ubuntu-latest diff --git a/util/check-open-once.sh b/util/check-open-once.sh new file mode 100755 index 00000000000..12b81c6f86e --- /dev/null +++ b/util/check-open-once.sh @@ -0,0 +1,58 @@ +#!/bin/bash +# +# spell-checker:ignore strace reflink FDCWD +# +# Redundant-open verification. +# +# These strace-based checks assert that a utility resolves an operand path +# once and works off the resulting descriptor, rather than re-opening the +# same path for each stage of its work. Repeated opens cost syscalls and +# let two stages disagree about which file they are acting on. +# +# This is a correctness and efficiency check, not a security one: the +# companion scripts check-toctou.sh and check-safe-traversal.sh cover the +# cases where a split across two path-based syscalls is actually +# attacker-exploitable. +# + +set -e + +echo "=== Redundant Open Verification ===" + +# shellcheck disable=SC2034 # read by check-common.sh once sourced +CHECK_UTILS="cp" +. "$(dirname "${BASH_SOURCE[0]}")/check-common.sh" + +cd "$TEMP_DIR" + +# cp used to open the source three times per copy: once in check_for_data, +# once in check_sparse_detection, then again in the copy itself. The +# sparseness probe and the copy now share one descriptor, so the strategy +# and the bytes come from the same file (#13185). +if cp_cmd=$(util_cmd cp); then + echo "" + echo "Testing cp (single source open)..." + # rustix issues open(2) directly on Linux, so match both spellings -- + # an openat-only filter would silently match nothing and pass. + for mode in "" "--reflink=never --sparse=always" "--reflink=never --sparse=never"; do + rm -f cp_probe_dst + printf 'source bytes for the probe\n' > cp_probe_src.txt + # shellcheck disable=SC2086 # $mode is a deliberate word-split flag list + strace -f -e trace=open,openat -o strace_cp_open.log \ + $cp_cmd $mode cp_probe_src.txt cp_probe_dst 2>/dev/null || true + + if [ ! -s strace_cp_open.log ]; then + fail_immediately "strace produced no output for cp" + fi + opens=$(grep -cE '(open|openat)\(.*"cp_probe_src\.txt"' strace_cp_open.log || true) + if [ "$opens" -ne 1 ]; then + cat strace_cp_open.log + fail_immediately "cp ${mode:-(default)} opened the source $opens times, expected exactly 1 - the sparseness probe and the copy must share one descriptor (#13185)" + fi + echo "✓ cp ${mode:-(default)} opens the source exactly once" + done + rm -f cp_probe_src.txt cp_probe_dst +fi + +echo "" +echo "✓ Redundant open verification completed" From 9ddecff84172fa13a912ca6f6dfce8460a4a9161 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 30 Aug 2026 13:48:38 +0200 Subject: [PATCH 3/3] mv: check the single source open at the syscall level mv already holds the invariant: it has its own cross-device fallback rather than cp's copy machinery, so it never had the three-open problem. These pin it in place, since the pending fd-anchored rework of the EXDEV directory path (GHSA-pp2g) touches exactly this code. Three assertions: a same-filesystem move must open the source zero times (it is a rename, not a copy), and the cross-device file and directory fallbacks must open each source exactly once. Each was verified to fail by injecting a redundant File::open at the matching call site. The directory case matches any path ending in the source name opened O_RDONLY, so it keeps working once the traversal opens by bare name under a directory fd. Cross-device needs a second filesystem; /dev/shm supplies it, and the checks skip loudly when it shares a device with TEMP_DIR rather than passing vacuously. --- .github/workflows/CICD.yml | 2 +- util/check-open-once.sh | 74 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/.github/workflows/CICD.yml b/.github/workflows/CICD.yml index c0ca5f40741..b6e7b782c9f 100644 --- a/.github/workflows/CICD.yml +++ b/.github/workflows/CICD.yml @@ -1102,7 +1102,7 @@ jobs: - name: Install strace run: sudo apt-get update && sudo apt-get install -y strace - name: Build utilities for redundant open checks - run: cargo build --profile=release-small -p uu_cp + run: cargo build --profile=release-small -p uu_cp -p uu_mv - name: Run redundant open verification run: ./util/check-open-once.sh diff --git a/util/check-open-once.sh b/util/check-open-once.sh index 12b81c6f86e..f9b19086bda 100755 --- a/util/check-open-once.sh +++ b/util/check-open-once.sh @@ -1,6 +1,6 @@ #!/bin/bash # -# spell-checker:ignore strace reflink FDCWD +# spell-checker:ignore strace reflink FDCWD EXDEV tmpfs xdev dstdir RDONLY # # Redundant-open verification. # @@ -20,9 +20,19 @@ set -e echo "=== Redundant Open Verification ===" # shellcheck disable=SC2034 # read by check-common.sh once sourced -CHECK_UTILS="cp" +CHECK_UTILS="cp mv" . "$(dirname "${BASH_SOURCE[0]}")/check-common.sh" +# check-common.sh installs `trap cleanup EXIT`; extend it so the scratch +# directory on the second filesystem goes away with it. +SHM_DIR="" +cleanup() { + rm -rf "$TEMP_DIR" + if [ -n "$SHM_DIR" ]; then + rm -rf "$SHM_DIR" + fi +} + cd "$TEMP_DIR" # cp used to open the source three times per copy: once in check_for_data, @@ -54,5 +64,65 @@ if cp_cmd=$(util_cmd cp); then rm -f cp_probe_src.txt cp_probe_dst fi +# mv must not open an operand it is only going to rename, and its +# cross-device fallback -- a copy, since rename(2) cannot cross filesystems -- +# must open the source once. mv has its own fallback rather than cp's +# (mv.rs, open_source + create_dest_restrictive + copy_fast), so this pins +# an invariant that a future move onto shared copy machinery could break. +if mv_cmd=$(util_cmd mv); then + echo "" + echo "Testing mv (single source open)..." + + printf 'moved by rename\n' > mv_probe_src.txt + strace -f -e trace=open,openat -o strace_mv_rename.log \ + $mv_cmd mv_probe_src.txt mv_probe_dst 2>/dev/null || true + if [ ! -s strace_mv_rename.log ]; then + fail_immediately "strace produced no output for mv" + fi + opens=$(grep -cE '(open|openat)\(.*"mv_probe_src\.txt"' strace_mv_rename.log || true) + if [ "$opens" -ne 0 ]; then + cat strace_mv_rename.log + fail_immediately "mv opened the source $opens times for a same-filesystem move, expected 0 - it must rename, not copy" + fi + echo "✓ mv (same filesystem) never opens the source" + rm -f mv_probe_dst + + # /dev/shm is tmpfs on Linux, so it is a different filesystem from + # $TEMP_DIR and rename(2) across the two returns EXDEV. Skip rather than + # fail where that does not hold, so the check stays honest about what it + # actually exercised. + if [ -d /dev/shm ] && [ "$(stat -c %d /dev/shm)" != "$(stat -c %d "$TEMP_DIR")" ]; then + SHM_DIR=$(mktemp -d /dev/shm/uutils-open-once.XXXXXX) + + printf 'moved across filesystems\n' > mv_probe_xdev.txt + strace -f -e trace=open,openat -o strace_mv_xdev.log \ + $mv_cmd mv_probe_xdev.txt "$SHM_DIR/dst" 2>/dev/null || true + opens=$(grep -cE '(open|openat)\(.*"mv_probe_xdev\.txt"' strace_mv_xdev.log || true) + if [ "$opens" -ne 1 ]; then + cat strace_mv_xdev.log + fail_immediately "mv opened the source $opens times for a cross-device move, expected exactly 1" + fi + echo "✓ mv (cross-device file) opens the source exactly once" + + mkdir -p mv_probe_dir + printf 'inner payload\n' > mv_probe_dir/inner.txt + strace -f -e trace=open,openat -o strace_mv_xdev_dir.log \ + $mv_cmd mv_probe_dir "$SHM_DIR/dstdir" 2>/dev/null || true + # Match any path ending in inner.txt opened O_RDONLY: that is the + # source read, whether it is spelled relative to the cwd or, once the + # traversal is fd-anchored, as a bare name under a directory fd. The + # O_RDONLY rules out the destination, which is opened for writing. + opens=$(grep -cE '(open|openat)\([^)]*"[^"]*inner\.txt"[^)]*O_RDONLY' strace_mv_xdev_dir.log || true) + if [ "$opens" -ne 1 ]; then + cat strace_mv_xdev_dir.log + fail_immediately "mv opened a file inside a cross-device directory move $opens times, expected exactly 1" + fi + echo "✓ mv (cross-device directory) opens each file exactly once" + else + echo "- skipped mv cross-device checks: no second filesystem available" + fi + rm -rf mv_probe_src.txt mv_probe_xdev.txt mv_probe_dir +fi + echo "" echo "✓ Redundant open verification completed"