diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index 971e99afcdb..89d1970c6a3 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -123,6 +123,7 @@ ENOENT ENOSPC ENOSYS ENOTEMPTY +ENXIO EOPNOTSUPP EPERM EPIPE diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 9075cb95fc3..85a5971dbd1 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -2212,13 +2212,14 @@ fn is_forbidden_to_copy_to_same_file( } /// Back up, remove, or leave intact the destination file, depending on the options. +/// Returns whether `dest` was removed or moved away. fn handle_existing_dest( source: &Path, dest: &Path, options: &Options, source_in_command_line: bool, copied_files: &HashMap, -) -> CopyResult<()> { +) -> CopyResult { // Disallow copying a file to itself, unless `--force` and // `--backup` are both specified. if is_forbidden_to_copy_to_same_file(source, dest, options, source_in_command_line) { @@ -2250,7 +2251,7 @@ fn handle_existing_dest( backup_dest(dest, &backup_path, is_dest_removed)?; } if !is_dest_removed { - delete_dest_if_needed_and_allowed( + is_dest_removed = delete_dest_if_needed_and_allowed( source, dest, options, @@ -2259,21 +2260,21 @@ fn handle_existing_dest( )?; } - Ok(()) + Ok(is_dest_removed) } /// Checks if: /// * `dest` needs to be deleted before the copy operation can proceed /// * the provided options allow this deletion /// -/// If so, deletes `dest`. +/// If so, deletes `dest` and returns `true`. fn delete_dest_if_needed_and_allowed( source: &Path, dest: &Path, options: &Options, source_in_command_line: bool, copied_files: &HashMap, -) -> CopyResult<()> { +) -> CopyResult { let delete_dest = match options.overwrite { OverwriteMode::Clobber(cl) | OverwriteMode::Interactive(cl) => { match cl { @@ -2315,10 +2316,9 @@ fn delete_dest_if_needed_and_allowed( }; if delete_dest { - delete_path(dest, options) - } else { - Ok(()) + delete_path(dest, options)?; } + Ok(delete_dest) } fn delete_path(path: &Path, options: &Options) -> CopyResult<()> { @@ -2527,6 +2527,7 @@ fn handle_copy_mode( symlinked_files: &mut HashSet, source_in_command_line: bool, created_parent_dirs: &mut HashSet, + create_exclusive: bool, ) -> CopyResult { match options.copy_mode { CopyMode::Link => { @@ -2569,6 +2570,8 @@ fn handle_copy_mode( symlinked_files, source_in_command_line, created_parent_dirs, + #[cfg(unix)] + create_exclusive, )?; } CopyMode::SymLink => { @@ -2593,6 +2596,8 @@ fn handle_copy_mode( symlinked_files, source_in_command_line, created_parent_dirs, + #[cfg(unix)] + create_exclusive, )?; } UpdateMode::None => { @@ -2627,6 +2632,8 @@ fn handle_copy_mode( symlinked_files, source_in_command_line, created_parent_dirs, + #[cfg(unix)] + create_exclusive, )?; } } @@ -2640,6 +2647,8 @@ fn handle_copy_mode( symlinked_files, source_in_command_line, created_parent_dirs, + #[cfg(unix)] + create_exclusive, )?; } } @@ -2668,16 +2677,9 @@ fn handle_copy_mode( .write(true) .truncate(false) .create(true) + .create_new(create_exclusive) .open(dest) - .map_err(|e| { - CpError::IoErrContext( - e, - translate!( - "cp-error-cannot-create-regular-file", - "path" => dest.quote() - ), - ) - })?; + .map_err(|e| create_dest_error(e, dest, create_exclusive))?; } } @@ -2751,6 +2753,7 @@ fn copy_file( .as_ref() .is_some_and(|md| md.file_type().is_symlink()); let dest_target_exists = dest.try_exists().unwrap_or(false); + let mut dest_removed = false; // Fail if dest is a dangling symlink or a symlink this program created previously if dest_is_symlink { if FileInformation::from_path(dest, false).is_ok_and(|info| symlinked_files.contains(&info)) @@ -2790,6 +2793,7 @@ fn copy_file( && options.backup == BackupMode::None { fs::remove_file(dest)?; + dest_removed = true; } } @@ -2803,6 +2807,7 @@ fn copy_file( && !paths_are_same_entry(source, dest) { fs::remove_file(dest)?; + dest_removed = true; } if initial_dest_metadata.is_some() @@ -2830,7 +2835,8 @@ fn copy_file( } } } - handle_existing_dest(source, dest, options, source_in_command_line, copied_files)?; + dest_removed |= + handle_existing_dest(source, dest, options, source_in_command_line, copied_files)?; if are_hardlinks_to_same_file(source, dest) { if options.copy_mode == CopyMode::Copy { return Ok(()); @@ -2908,6 +2914,9 @@ fn copy_file( }; let dest_metadata = dest.symlink_metadata().ok(); + // Create `dest` exclusively unless an existing file is overwritten in place, + // so nothing that appears at that name meanwhile is opened, as in GNU cp. + let create_exclusive = initial_dest_metadata.is_none() || dest_removed; let dest_permissions = calculate_dest_permissions( dest_metadata.as_ref(), @@ -2928,6 +2937,7 @@ fn copy_file( symlinked_files, source_in_command_line, created_parent_dirs, + create_exclusive, )?; if options.verbose && performed_action != PerformedAction::Skipped { @@ -3041,6 +3051,40 @@ fn is_stream(metadata: &Metadata) -> bool { } } +/// How a Unix backend opens the source and creates the destination. +#[cfg(unix)] +pub(crate) struct CopyFlags { + /// The source is a FIFO or a device, read as a stream. + pub(crate) source_is_stream: bool, + /// Open the source with `O_NOFOLLOW`. + pub(crate) nofollow: bool, + /// Nothing was at `dest` before the copy, or cp removed it: create the + /// file anew and fail if something appears at `dest` in the meantime. + pub(crate) exclusive: bool, +} + +/// Create the regular file `dest`, anew if `exclusive`. Otherwise an existing +/// `dest` is truncated and followed if it is a symlink, as GNU cp -d/-P does. +#[cfg(unix)] +pub(crate) fn create_dest(dest: &Path, exclusive: bool) -> CopyResult { + uucore::safe_copy::create_dest_restrictive(dest, exclusive) + .map_err(|e| create_dest_error(e, dest, exclusive)) +} + +/// Error for a failed create of `dest`. Like GNU cp, a symlink found by an +/// exclusive create is reported as one that is not written through. +fn create_dest_error(err: io::Error, dest: &Path, exclusive: bool) -> CpError { + if exclusive && err.kind() == io::ErrorKind::AlreadyExists && dest.is_symlink() { + return CpError::Error( + translate!("cp-error-not-writing-dangling-symlink", "dest" => dest.quote()), + ); + } + CpError::IoErrContext( + err, + translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()), + ) +} + #[cfg(unix)] fn handle_no_preserve_mode(options: &Options, org_mode: u32) -> u32 { let (is_preserve_mode, is_explicit_no_preserve_mode) = options.preserve_mode(); @@ -3100,6 +3144,7 @@ fn copy_helper( symlinked_files: &mut HashSet, #[cfg_attr(not(unix), allow(unused_variables))] source_in_command_line: bool, created_parent_dirs: &mut HashSet, + #[cfg(unix)] create_exclusive: bool, ) -> CopyResult<()> { if options.parents { let parent = dest.parent().unwrap_or(dest); @@ -3144,9 +3189,11 @@ fn copy_helper( options.sparse_mode, context, #[cfg(unix)] - is_stream(source_metadata), - #[cfg(unix)] - nofollow, + CopyFlags { + source_is_stream: is_stream(source_metadata), + nofollow, + exclusive: create_exclusive, + }, )?; if !options.attributes_only && options.debug { diff --git a/src/uu/cp/src/platform/linux.rs b/src/uu/cp/src/platform/linux.rs index d14b067e915..c8f19143b6f 100644 --- a/src/uu/cp/src/platform/linux.rs +++ b/src/uu/cp/src/platform/linux.rs @@ -14,26 +14,14 @@ use std::os::unix::fs::MetadataExt; use std::path::Path; use uucore::buf_copy; -use uucore::display::Quotable; -use uucore::safe_copy::{create_dest_restrictive, open_source}; +use uucore::safe_copy::open_source; use uucore::translate; use crate::{ - CopyDebug, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, SparseMode, - is_stream, + CopyDebug, CopyFlags, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, + SparseMode, create_dest, is_stream, }; -// 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.quote()), - ) - }) -} - /// The fallback behavior for [`clone`] on failed system call. /// /// Every fallback reuses the descriptors already opened for the clone @@ -73,13 +61,14 @@ enum CopyMethod { fn clone( src_file: &mut File, dest: &Path, + exclusive: bool, fallback: CloneFallback, context: &str, ) -> 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.symlink_metadata().is_ok(); - let mut dst_file = create_dest(dest)?; + let mut dst_file = create_dest(dest, exclusive)?; 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, @@ -232,7 +221,13 @@ fn check_dest_is_fifo(dest: &Path) -> bool { } /// Copy the contents of a stream from `source` to `dest`. -fn copy_stream

(source: P, dest: P, nofollow: bool, context: &str) -> CopyResult<()> +fn copy_stream

( + source: P, + dest: P, + exclusive: bool, + nofollow: bool, + context: &str, +) -> CopyResult<()> where P: AsRef, { @@ -260,12 +255,7 @@ where // the dest does not momentarily sit with broader perms. The `0o622 & // !umask` form previously used here could still allow group/other write // under a permissive umask. See #10011. - 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()), - ) - })?; + let mut dst_file = create_dest(dest.as_ref(), exclusive)?; let ctx_err = |e: io::Error| CpError::IoErrContext(e, context.to_owned()); @@ -295,9 +285,13 @@ pub(crate) fn copy_on_write( reflink_mode: ReflinkMode, sparse_mode: SparseMode, context: &str, - source_is_stream: bool, - nofollow: bool, + flags: CopyFlags, ) -> CopyResult { + let CopyFlags { + source_is_stream, + nofollow, + exclusive, + } = flags; let mut copy_debug = CopyDebug { offload: OffloadReflinkDebug::Unknown, reflink: OffloadReflinkDebug::Unsupported, @@ -310,7 +304,7 @@ pub(crate) fn copy_on_write( copy_debug.reflink = OffloadReflinkDebug::No; if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest, nofollow, context) + copy_stream(source, dest, exclusive, nofollow, context) } else { let mut src_file = open_source(source, nofollow) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; @@ -321,7 +315,7 @@ pub(crate) fn copy_on_write( copy_method = method; } - let mut dst_file = create_dest(dest)?; + let mut dst_file = create_dest(dest, exclusive)?; match copy_method { CopyMethod::FSCopy => buf_copy::copy_fast(&mut src_file, &mut dst_file) .map_err(|e| CpError::IoErrContext(e, context.to_owned())), @@ -334,7 +328,7 @@ pub(crate) fn copy_on_write( if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest, nofollow, context) + copy_stream(source, dest, exclusive, nofollow, context) } else { let mut src_file = open_source(source, nofollow) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; @@ -342,7 +336,7 @@ pub(crate) fn copy_on_write( if let Ok(debug) = result { copy_debug = debug; } - let mut dst_file = create_dest(dest)?; + let mut dst_file = create_dest(dest, exclusive)?; buf_copy::copy_fast(&mut src_file, &mut dst_file) .map_err(|e| CpError::IoErrContext(e, context.to_owned())) } @@ -352,7 +346,7 @@ pub(crate) fn copy_on_write( if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest, nofollow, context) + copy_stream(source, dest, exclusive, nofollow, context) } else { let mut src_file = open_source(source, nofollow) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; @@ -363,7 +357,7 @@ pub(crate) fn copy_on_write( copy_method = method; } - let mut dst_file = create_dest(dest)?; + let mut dst_file = create_dest(dest, exclusive)?; match copy_method { CopyMethod::SparseCopyWithoutHole => { sparse_copy_without_hole_fd(&src_file, &dst_file, context) @@ -378,7 +372,7 @@ pub(crate) fn copy_on_write( // SparseMode::Always if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest, nofollow, context) + copy_stream(source, dest, exclusive, nofollow, context) } else { let mut src_file = open_source(source, nofollow) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; @@ -390,10 +384,20 @@ pub(crate) fn copy_on_write( } match copy_method { - CopyMethod::FSCopy => { - clone(&mut src_file, dest, CloneFallback::FSCopy, context) - } - _ => clone(&mut src_file, dest, CloneFallback::SparseCopy, context), + CopyMethod::FSCopy => clone( + &mut src_file, + dest, + exclusive, + CloneFallback::FSCopy, + context, + ), + _ => clone( + &mut src_file, + dest, + exclusive, + CloneFallback::SparseCopy, + context, + ), } } } @@ -402,7 +406,7 @@ pub(crate) fn copy_on_write( copy_debug.reflink = OffloadReflinkDebug::No; if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Avoided; - copy_stream(source, dest, nofollow, context) + copy_stream(source, dest, exclusive, nofollow, context) } else { let mut src_file = open_source(source, nofollow) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; @@ -411,13 +415,19 @@ pub(crate) fn copy_on_write( copy_debug = debug; } - clone(&mut src_file, dest, CloneFallback::FSCopy, context) + clone( + &mut src_file, + dest, + exclusive, + CloneFallback::FSCopy, + context, + ) } } (ReflinkMode::Auto, SparseMode::Auto) => { if source_is_stream { copy_debug.offload = OffloadReflinkDebug::Unsupported; - copy_stream(source, dest, nofollow, context) + copy_stream(source, dest, exclusive, nofollow, context) } else { let mut src_file = open_source(source, nofollow) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; @@ -432,10 +442,17 @@ pub(crate) fn copy_on_write( CopyMethod::SparseCopyWithoutHole => clone( &mut src_file, dest, + exclusive, CloneFallback::SparseCopyWithoutHole, context, ), - _ => clone(&mut src_file, dest, CloneFallback::FSCopy, context), + _ => clone( + &mut src_file, + dest, + exclusive, + CloneFallback::FSCopy, + context, + ), } } } @@ -446,7 +463,13 @@ pub(crate) fn copy_on_write( 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) + clone( + &mut src_file, + dest, + exclusive, + CloneFallback::Error, + context, + ) } (ReflinkMode::Always, _) => { return Err(translate!("cp-error-reflink-always-sparse-auto").into()); diff --git a/src/uu/cp/src/platform/macos.rs b/src/uu/cp/src/platform/macos.rs index b338c2bffbc..091bbc453e9 100644 --- a/src/uu/cp/src/platform/macos.rs +++ b/src/uu/cp/src/platform/macos.rs @@ -6,21 +6,18 @@ // spell-checker:ignore reflink use std::ffi::CString; -use std::fs::{self, File, OpenOptions}; +use std::fs::{self, File}; use std::os::unix::ffi::OsStrExt; -use std::os::unix::fs::OpenOptionsExt; use std::path::Path; use uucore::buf_copy; use uucore::display::Quotable; -use uucore::safe_copy::{create_dest_restrictive, open_source}; +use uucore::safe_copy::open_source; use uucore::translate; -use uucore::mode::get_umask; - use crate::{ - CopyDebug, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, SparseMode, - is_stream, + CopyDebug, CopyFlags, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, + SparseMode, create_dest, is_stream, }; /// Copies `source` to `dest` using copy-on-write if possible. @@ -30,9 +27,13 @@ pub(crate) fn copy_on_write( reflink_mode: ReflinkMode, sparse_mode: SparseMode, context: &str, - source_is_stream: bool, - nofollow: bool, + flags: CopyFlags, ) -> CopyResult { + let CopyFlags { + source_is_stream, + nofollow, + mut exclusive, + } = flags; if sparse_mode != SparseMode::Auto { return Err(translate!("cp-error-sparse-not-supported").into()); } @@ -70,6 +71,9 @@ pub(crate) fn copy_on_write( ) -> core::ffi::c_int = std::mem::transmute(raw_pfn); error = pfn(src.as_ptr(), dst.as_ptr(), 0); if std::io::Error::last_os_error().kind() == std::io::ErrorKind::AlreadyExists + // Something appeared at a `dest` that was free: leave it alone + // and let the exclusive create below report it. + && !exclusive // Only remove the `dest` if the `source` and `dest` are not the same && source != dest { @@ -92,7 +96,9 @@ pub(crate) fn copy_on_write( // TODO: rewrite this to better match linux behavior // linux first opens the source file and destination file then uses the file // descriptors to do the clone. - let _ = fs::remove_file(dest); + if fs::remove_file(dest).is_ok() { + exclusive = true; + } error = pfn(src.as_ptr(), dst.as_ptr(), 0); } } @@ -110,18 +116,7 @@ pub(crate) fn copy_on_write( if source_is_stream { let mut src_file = File::open(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - let mode = 0o622 & !get_umask(); - let mut dst_file = OpenOptions::new() - .create(true) - .write(true) - .mode(mode) - .open(dest) - .map_err(|e| { - CpError::IoErrContext( - e, - translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()), - ) - })?; + let mut dst_file = create_dest(dest, exclusive)?; let dest_is_stream = is_stream( &dst_file @@ -141,12 +136,7 @@ pub(crate) fn copy_on_write( } else { 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.quote()), - ) - })?; + let mut dst_file = create_dest(dest, exclusive)?; std::io::copy(&mut src_file, &mut dst_file) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; } diff --git a/src/uu/cp/src/platform/other_unix.rs b/src/uu/cp/src/platform/other_unix.rs index 754fa872e41..2026edfb878 100644 --- a/src/uu/cp/src/platform/other_unix.rs +++ b/src/uu/cp/src/platform/other_unix.rs @@ -8,13 +8,12 @@ use std::path::Path; use uucore::buf_copy; -use uucore::display::Quotable; -use uucore::safe_copy::{create_dest_restrictive, open_source}; +use uucore::safe_copy::open_source; use uucore::translate; use crate::{ - CopyDebug, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, SparseMode, - is_stream, + CopyDebug, CopyFlags, CopyResult, CpError, OffloadReflinkDebug, ReflinkMode, SparseDebug, + SparseMode, create_dest, is_stream, }; /// Copies `source` to `dest` for systems without copy-on-write @@ -24,9 +23,13 @@ pub(crate) fn copy_on_write( reflink_mode: ReflinkMode, sparse_mode: SparseMode, context: &str, - source_is_stream: bool, - nofollow: bool, + flags: CopyFlags, ) -> CopyResult { + let CopyFlags { + source_is_stream, + nofollow, + exclusive, + } = flags; if reflink_mode != ReflinkMode::Never { return Err(translate!("cp-error-reflink-not-supported") .to_string() @@ -46,12 +49,7 @@ pub(crate) fn copy_on_write( if source_is_stream { 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.quote()), - ) - })?; + let mut dst_file = create_dest(dest, exclusive)?; let dest_is_stream = is_stream(&dst_file.metadata()?); if !dest_is_stream { @@ -72,12 +70,7 @@ pub(crate) fn copy_on_write( // dest is followed, matching GNU cp. 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.quote()), - ) - })?; + let mut dst_file = create_dest(dest, exclusive)?; std::io::copy(&mut src_file, &mut dst_file) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 68cc9c21887..8da9a0e0853 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -1431,9 +1431,8 @@ fn rename_file_fallback( } } - // Open src/dst with O_NOFOLLOW and keep the fds alive across copy, - // chown, xattr, and chmod so a concurrent path-swap can't redirect any - // step to a different inode. + // Open src with O_NOFOLLOW, create dst exclusively, and keep both fds for copy, + // chown, xattr and chmod, so each step acts on these files even if the paths change. #[cfg(unix)] { use std::fs::Permissions; @@ -1446,7 +1445,9 @@ fn rename_file_fallback( .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))? .mode() & 0o7777; - let mut dst_file = create_dest_restrictive(to, /* nofollow */ true) + // `to` was removed above (or was absent), so anything found there now + // was not created by mv; fail instead of opening it, as GNU mv does. + let mut dst_file = create_dest_restrictive(to, /* exclusive */ true) .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; uucore::buf_copy::copy_fast(&mut &src_file, &mut dst_file) .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; diff --git a/src/uucore/src/lib/features/safe_copy.rs b/src/uucore/src/lib/features/safe_copy.rs index d8cbc36b824..01a9500fca2 100644 --- a/src/uucore/src/lib/features/safe_copy.rs +++ b/src/uucore/src/lib/features/safe_copy.rs @@ -17,9 +17,8 @@ //! * [`create_dest_restrictive`] creates the destination with mode //! `0o600` instead of the umask-derived 0o644, so another user in a //! shared directory cannot open the file before the caller narrows the -//! final permissions via `set_permissions` (issue #10011). The same -//! `nofollow` flag refuses to truncate through a symlink that may have -//! been swapped in at the destination path. +//! final permissions via `set_permissions` (issue #10011), and with +//! `exclusive` fails if anything is already at the path. use std::fs::File; use std::io; @@ -38,6 +37,10 @@ const DEST_FLAGS: OFlags = OFlags::WRONLY .union(OFlags::CREATE) .union(OFlags::TRUNC) .union(OFlags::CLOEXEC); +const DEST_EXCLUSIVE_FLAGS: OFlags = OFlags::WRONLY + .union(OFlags::CREATE) + .union(OFlags::EXCL) + .union(OFlags::CLOEXEC); /// Open `path` for reading, optionally with `O_NOFOLLOW`. /// @@ -64,17 +67,14 @@ pub fn open_source>(path: P, nofollow: bool) -> io::Result /// the source's permissions should do so via `set_permissions` *after* /// the content copy completes. /// -/// With `nofollow = true`, an existing symlink at `path` causes `ELOOP` -/// rather than truncating the symlink's target. Pass `true` whenever the -/// caller has not just unlinked `path` itself: without it, an attacker -/// who plants `path` as a symlink between the caller's check and this -/// open can redirect the truncate (and the subsequent write) to any file -/// the caller has permission to write. -pub fn create_dest_restrictive>(path: P, nofollow: bool) -> io::Result { - let mut flags = DEST_FLAGS; - if nofollow { - flags |= OFlags::NOFOLLOW; - } +/// With `exclusive`, anything at `path`, even a dangling symlink, fails with +/// `EEXIST`. Without it, an existing `path` is truncated and followed. +pub fn create_dest_restrictive>(path: P, exclusive: bool) -> io::Result { + let flags = if exclusive { + DEST_EXCLUSIVE_FLAGS + } else { + DEST_FLAGS + }; let fd: OwnedFd = open(path.as_ref(), flags, Mode::RUSR.union(Mode::WUSR))?; Ok(File::from(fd)) } @@ -166,22 +166,42 @@ mod tests { } #[test] - fn create_dest_rejects_symlink_with_nofollow() { - // Without nofollow on the dest, an attacker swapping `dst` to a - // symlink between check and open would silently truncate the - // symlink's target. With nofollow=true the open returns ELOOP and - // the would-be victim file is left untouched. + fn create_dest_exclusive_rejects_existing_symlink() { + // The exclusive create fails with EEXIST on a symlink, dangling or not, + // and leaves its target untouched. let dir = tempdir().unwrap(); - let victim = dir.path().join("victim"); + let target = dir.path().join("target"); let dst = dir.path().join("dst"); - std::fs::write(&victim, b"do not truncate me").unwrap(); - symlink(&victim, &dst).unwrap(); + let dangling = dir.path().join("dangling"); + std::fs::write(&target, b"do not truncate me").unwrap(); + symlink(&target, &dst).unwrap(); + symlink(dir.path().join("missing"), &dangling).unwrap(); + + for path in [&dst, &dangling] { + let err = create_dest_restrictive(path, true).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::AlreadyExists); + } + assert_eq!(std::fs::read(&target).unwrap(), b"do not truncate me"); + assert!(!dir.path().join("missing").exists()); + } - let err = create_dest_restrictive(&dst, true).unwrap_err(); - assert_eq!( - err.raw_os_error(), - Some(rustix::io::Errno::LOOP.raw_os_error()) - ); - assert_eq!(std::fs::read(&victim).unwrap(), b"do not truncate me"); + #[test] + fn create_dest_exclusive_rejects_existing_file() { + let dir = tempdir().unwrap(); + let path = dir.path().join("existing"); + std::fs::write(&path, b"keep").unwrap(); + + let err = create_dest_restrictive(&path, true).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::AlreadyExists); + assert_eq!(std::fs::read(&path).unwrap(), b"keep"); + } + + #[test] + fn create_dest_exclusive_uses_restrictive_initial_mode() { + let dir = tempdir().unwrap(); + let path = dir.path().join("new"); + let f = create_dest_restrictive(&path, true).unwrap(); + let mode = f.metadata().unwrap().mode() & 0o777; + assert_eq!(mode, DEST_INITIAL_MODE); } } diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index d3474eeb413..0f1c61fc379 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1369,6 +1369,67 @@ fn test_cp_arg_remove_destination() { assert_eq!(at.read(TEST_HELLO_WORLD_DEST), "Hello, World!\n"); } +/// Like GNU cp, cp must not open what appears at `dest` after removing it. +/// The FIFO source keeps cp waiting between the removal and the creation. +#[cfg(unix)] +#[rstest] +#[case::symlink_remove_destination(&["--remove-destination"], Some("other"))] +#[case::dangling_remove_destination(&["--remove-destination"], Some("missing"))] +#[case::file_remove_destination(&["--remove-destination"], None)] +#[case::symlink_force_readonly(&["-f"], Some("other"))] +#[case::file_force_readonly(&["-f"], None)] +#[case::symlink_simple_backup(&["--backup=simple"], Some("other"))] +#[case::file_numbered_backup(&["--backup=numbered"], None)] +#[cfg_attr(wasi_runner, ignore = "WASI: no FIFO/mkfifo support")] +fn test_cp_does_not_open_dest_recreated_after_removal( + #[case] args: &[&str], + #[case] symlink_target: Option<&str>, +) { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkfifo("src"); + at.write("other", "keep"); + if args.iter().any(|arg| arg.starts_with("--backup=")) { + at.write("original", "old"); + at.symlink_file("original", "dest"); + } else { + at.write("dest", "old"); + at.set_mode("dest", 0o444); + } + + let child = scene.ucmd().args(args).args(&["src", "dest"]).run_no_wait(); + let start = std::time::Instant::now(); + while at.plus("dest").symlink_metadata().is_ok() { + assert!( + start.elapsed() < Duration::from_secs(60), + "timed out: cp did not remove dest" + ); + sleep(Duration::from_millis(10)); + } + if let Some(target) = symlink_target { + at.symlink_file(target, "dest"); + } else { + at.write("dest", "new"); + } + // Opening the writer releases cp's open of the source. cp may reject the + // destination and close its end before the write, which then fails. + let _ = std::fs::write(at.plus("src"), "data"); + + let expected = if symlink_target.is_some() { + "cp: not writing through dangling symlink 'dest'\n" + } else { + "cp: cannot create regular file 'dest': File exists\n" + }; + child.wait().unwrap().failure().stderr_only(expected); + assert_eq!(at.read("other"), "keep"); + assert!(!at.file_exists("missing")); + if symlink_target.is_some() { + assert!(at.is_symlink("dest")); + } else { + assert_eq!(at.read("dest"), "new"); + } +} + #[test] fn test_cp_arg_backup() { let (at, mut ucmd) = at_and_ucmd!(); diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index b2bf30e8732..b71bc2b93ad 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -2892,13 +2892,8 @@ fn test_mv_cross_device_permission_denied() { .expect("Unable to restore directory permissions"); } -/// Regression for #10015. The cross-device fallback unlinks the destination -/// and then creates a new file at the same path. An attacker who can write -/// to the destination directory could race to plant a symlink in that -/// window; without `O_NOFOLLOW` on the create, the copy would write through -/// the symlink. The deterministic stand-in here pre-plants the symlink: -/// `safe_copy::create_dest_restrictive` must refuse it with `ELOOP` rather -/// than truncate `victim`. +/// Regression for #10015: a cross-device move replaces a symlink at the +/// destination instead of writing through it. #[test] #[cfg(target_os = "linux")] fn test_mv_cross_device_refuses_planted_symlink_dest() { diff --git a/util/check-safe-traversal.sh b/util/check-safe-traversal.sh index c52deafb148..aa9dda6c574 100755 --- a/util/check-safe-traversal.sh +++ b/util/check-safe-traversal.sh @@ -362,11 +362,11 @@ if have_util mv && [ -d /dev/shm ]; then fi fi -# mv cross-device must open the destination with O_NOFOLLOW (issue #10015). -# The EXDEV fallback unlinks the dest then opens with O_CREAT|O_TRUNC; an -# attacker racing in a planted symlink would otherwise let the copy write -# through to the symlink's target. rustix may emit either open(2) or -# openat(2) depending on the path, so the check accepts both. +# mv cross-device must not open a symlink at the destination (issue #10015). +# The EXDEV fallback unlinks the dest then creates it; an attacker racing in +# a planted symlink would otherwise let the copy write through to the +# symlink's target; O_EXCL or O_NOFOLLOW prevents that. rustix may emit +# either open(2) or openat(2) depending on the path, so the check accepts both. if have_util mv && [ -d /dev/shm ]; then temp_fs_id=$(stat -f -c %i "$TEMP_DIR" 2>/dev/null || echo "") shm_fs_id=$(stat -f -c %i /dev/shm 2>/dev/null || echo "") @@ -385,17 +385,17 @@ if have_util mv && [ -d /dev/shm ]; then nofollow_dst_base=$(basename "$nofollow_dst") # Lines that open the dest with O_CREAT — the dest may appear as a # full path (open) or basename (openat). Every such open must carry - # O_NOFOLLOW or the planted-symlink writethrough is possible. + # O_EXCL or O_NOFOLLOW or the planted-symlink writethrough is possible. dest_creates=$(grep -E "open(at)?2?\(.*\"[^\"]*${nofollow_dst_base}\".*O_CREAT" strace_mv_nofollow.log || true) if [ -z "$dest_creates" ]; then cat strace_mv_nofollow.log fail_immediately "mv cross-device did not open dest with O_CREAT — strace check broken (issue #10015)" fi - if echo "$dest_creates" | grep -vq "O_NOFOLLOW"; then + if echo "$dest_creates" | grep -Evq "O_EXCL|O_NOFOLLOW"; then cat strace_mv_nofollow.log - fail_immediately "mv cross-device must open dest with O_NOFOLLOW (issue #10015)" + fail_immediately "mv cross-device must open dest with O_EXCL or O_NOFOLLOW (issue #10015)" fi - echo "OK: mv cross-device opens dest with O_NOFOLLOW" + echo "OK: mv cross-device opens dest with O_EXCL or O_NOFOLLOW" rm -f "$nofollow_dst" fi fi