diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 046ac6a873..7350981c50 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -87,6 +87,10 @@ pub enum CpError { #[error("{}", translate!("cp-error-not-all-files-copied"))] NotAllFilesCopied, + /// Xattr copying already reported each failure; only the exit code is needed. + #[error("")] + XattrErrorsReported, + /// Simple [`walkdir::Error`] wrapper #[error("{0}")] WalkDirErr(#[from] walkdir::Error), @@ -1417,16 +1421,10 @@ fn is_enotsup_error(error: &CpError) -> bool { /// When handling errors, we don't always want to show them to the user. This function handles that. fn show_error_if_needed(error: &CpError) { match error { - // When using --no-clobber, we don't want to show - // an error message - #[expect(clippy::match_same_arms)] // needs comment - CpError::NotAllFilesCopied => { - // Need to return an error code - } - CpError::Skipped(_) => { - // touch a b && echo "n"|cp -i a b && echo $? - // should return an error - } + // NotAllFilesCopied: Need to return an error code + // Skipped: touch a b && echo "n"|cp -i a b && echo $? should return an error + // XattrErrorsReported: each failing attribute was already reported on stderr by `copy_xattrs*` + CpError::NotAllFilesCopied | CpError::Skipped(_) | CpError::XattrErrorsReported => {} // Format IoErrContext using strip_errno to remove "(os error N)" suffix // for GNU-compatible output CpError::IoErrContext(io_err, context) | CpError::SelinuxContextIoErr(io_err, context) => { @@ -1857,12 +1855,16 @@ fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyRe fs::set_permissions(dest, revert_perms)?; } - // If copying xattrs failed, propagate that error now with context. + // `copy_xattrs*` already reported each failure; add context only when xattrs are unsupported. copy_xattrs_result.map_err(|e| { - CpError::IoErrContext( - e, - translate!("cp-error-setting-attributes", "path" => dest.quote()), - ) + if uucore::fsxattr::is_xattr_unsupported(&e) { + CpError::IoErrContext( + e, + translate!("cp-error-setting-attributes", "path" => dest.quote()), + ) + } else { + CpError::XattrErrorsReported + } })?; Ok(()) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 3536c54a73..a45f167672 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -1146,12 +1146,8 @@ fn rename_dir_fallback( display_manager, ); - // Apply xattrs using a file descriptor to avoid TOCTOU races, ignoring - // ENOTSUP/EOPNOTSUPP (filesystem without xattr support, which is expected - // for cross-device moves). - // - // The fd is opened read-only: a directory cannot be opened for writing, and - // fsetxattr checks write permission on the inode, not the open mode. + // Apply xattrs using a read-only fd. Per-attribute failures are reported + // on stderr by apply_xattrs_fd_* and do not fail the move. #[cfg(any( target_os = "freebsd", target_os = "hurd", @@ -1162,7 +1158,7 @@ fn rename_dir_fallback( { use std::fs::File; let dest = File::open(to)?; - fsxattr::apply_xattrs_fd_ignore_unsupported(&dest, xattrs)?; + let _ = fsxattr::apply_xattrs_fd_ignore_unsupported(&dest, xattrs); } result?; diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index b180cf9a65..89adb8c7ce 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -7,6 +7,9 @@ //! Set of functions to manage xattr on files and dirs +use crate::display::Quotable; +use crate::error::strip_errno; +use crate::show_error; use itertools::Itertools; use rustc_hash::FxHashMap; use std::ffi::{OsStr, OsString}; @@ -17,7 +20,7 @@ use std::path::Path; /// True if the error is `ENOTSUP` / `EOPNOTSUPP` (same errno on Linux, /// distinct on the BSDs). #[cfg(unix)] -fn is_xattr_unsupported(err: &std::io::Error) -> bool { +pub fn is_xattr_unsupported(err: &std::io::Error) -> bool { matches!( err.raw_os_error(), Some(e) if e == libc::ENOTSUP || e == libc::EOPNOTSUPP @@ -25,20 +28,51 @@ fn is_xattr_unsupported(err: &std::io::Error) -> bool { } #[cfg(not(unix))] -fn is_xattr_unsupported(_err: &std::io::Error) -> bool { +pub fn is_xattr_unsupported(_err: &std::io::Error) -> bool { false } +/// Report a per-attribute failure on stderr (except unsupported fs errnos) +/// and record it to return after continuing the copy loop. +fn record_xattr_failure( + attr_name: &OsStr, + reading: bool, + err: std::io::Error, + pending_error: &mut Option, +) { + if !is_xattr_unsupported(&err) { + let action = if reading { + "cannot read attribute" + } else { + "setting attribute" + }; + show_error!("{action} {}: {}", attr_name.quote(), strip_errno(&err)); + } + if pending_error.is_none() { + *pending_error = Some(err); + } +} + /// Copies extended attributes (xattrs) from one path to another. -/// All errors propagate, including `ENOTSUP` / `EOPNOTSUPP`; for +/// +/// A failed attribute is reported on stderr and does not stop the other +/// attributes from being copied; the first such failure is propagated at +/// the end. `ENOTSUP` / `EOPNOTSUPP` are recorded but not reported; for /// best-effort callers see [`copy_xattrs_ignore_unsupported`]. pub fn copy_xattrs>(source: P, dest: P) -> std::io::Result<()> { + let mut pending_error = None; for attr_name in xattr::list(&source)? { - if let Some(value) = xattr::get(&source, &attr_name)? { - xattr::set(&dest, &attr_name, &value)?; + match xattr::get(&source, &attr_name) { + Ok(Some(value)) => { + if let Err(err) = xattr::set(&dest, &attr_name, &value) { + record_xattr_failure(&attr_name, false, err, &mut pending_error); + } + } + Ok(None) => {} + Err(err) => record_xattr_failure(&attr_name, true, err, &mut pending_error), } } - Ok(()) + pending_error.map_or(Ok(()), Err) } /// Like [`copy_xattrs`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())` @@ -53,15 +87,25 @@ pub fn copy_xattrs_ignore_unsupported>(source: P, dest: P) -> std /// Copies xattrs between two open file descriptors. Pins both inodes so /// list/get/set calls cannot be redirected by a concurrent renamer, unlike /// the path-based [`copy_xattrs`]. +/// +/// Failures are handled like in [`copy_xattrs`]: each one is reported and +/// the remaining attributes are still copied. #[cfg(unix)] pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> std::io::Result<()> { use xattr::FileExt; + let mut pending_error = None; for attr_name in source.list_xattr()? { - if let Some(value) = source.get_xattr(&attr_name)? { - dest.set_xattr(&attr_name, &value)?; + match source.get_xattr(&attr_name) { + Ok(Some(value)) => { + if let Err(err) = dest.set_xattr(&attr_name, &value) { + record_xattr_failure(&attr_name, false, err, &mut pending_error); + } + } + Ok(None) => {} + Err(err) => record_xattr_failure(&attr_name, true, err, &mut pending_error), } } - Ok(()) + pending_error.map_or(Ok(()), Err) } /// Like [`copy_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`. @@ -77,16 +121,27 @@ pub fn copy_xattrs_fd_ignore_unsupported( } /// Like `copy_xattrs`, but skips the security.selinux attribute. +/// +/// Failures are handled like in [`copy_xattrs`]: each one is reported and +/// the remaining attributes are still copied. #[cfg(unix)] pub fn copy_xattrs_skip_selinux>(source: P, dest: P) -> std::io::Result<()> { + let mut pending_error = None; for attr_name in xattr::list(&source)? { - if attr_name.as_bytes() != b"security.selinux" - && let Some(value) = xattr::get(&source, &attr_name)? - { - xattr::set(&dest, &attr_name, &value)?; + if attr_name.as_bytes() == b"security.selinux" { + continue; + } + match xattr::get(&source, &attr_name) { + Ok(Some(value)) => { + if let Err(err) = xattr::set(&dest, &attr_name, &value) { + record_xattr_failure(&attr_name, false, err, &mut pending_error); + } + } + Ok(None) => {} + Err(err) => record_xattr_failure(&attr_name, true, err, &mut pending_error), } } - Ok(()) + pending_error.map_or(Ok(()), Err) } /// Copies only the POSIX ACL xattrs (`system.posix_acl_access` and @@ -156,6 +211,9 @@ pub fn retrieve_xattrs_fd(source: &std::fs::File) -> std::io::Result>( dest: P, xattrs: FxHashMap>, ) -> std::io::Result<()> { + let mut pending_error = None; for (attr, value) in xattrs { - xattr::set(&dest, &attr, &value)?; + if let Err(err) = xattr::set(&dest, &attr, &value) { + record_xattr_failure(&attr, false, err, &mut pending_error); + } } - Ok(()) + pending_error.map_or(Ok(()), Err) } /// Applies extended attributes (xattrs) to a given file using a file descriptor. /// -/// This version avoids TOCTOU races by operating on an open file descriptor -/// rather than a path, ensuring all operations target the same inode. +/// Failures are handled like in [`copy_xattrs`]: each one is reported and +/// the remaining attributes are still applied. /// /// # Arguments /// @@ -193,10 +254,13 @@ pub fn apply_xattrs_fd( xattrs: FxHashMap>, ) -> std::io::Result<()> { use xattr::FileExt; + let mut pending_error = None; for (attr, value) in xattrs { - dest.set_xattr(&attr, &value)?; + if let Err(err) = dest.set_xattr(&attr, &value) { + record_xattr_failure(&attr, false, err, &mut pending_error); + } } - Ok(()) + pending_error.map_or(Ok(()), Err) } /// Like [`apply_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`. diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 54188b30da..2a66d2e168 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -9486,35 +9486,32 @@ fn test_cp_xattr_enotsup_handling() { #[cfg(target_os = "linux")] fn test_cp_xattr_failure_keeps_dest_contents() { use std::process::Command; + use uutests::util::tmpfs_to_target_failing_xattr_value; let scene = TestScenario::new(util_name!()); - // tmpfs accepts large user-xattr values while ext4 and friends cap them - // near the block size, so copying such a source out of tmpfs makes - // --preserve=xattr fail only after the file data has been written. - // The fixtures dir may itself be on tmpfs, so put the destination in - // target/tmp, which lives on the build filesystem. - let big_value = "y".repeat(9_100); let pid = std::process::id(); - let source = format!("/dev/shm/cp_keep_dest_{pid}"); let dest_dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(format!("cp_keep_dest_{pid}")); - if std_fs::write(&source, "kept content").is_err() || std_fs::create_dir(&dest_dir).is_err() { - return; // skip: no usable /dev/shm or target/tmp + if std_fs::create_dir(&dest_dir).is_err() { + return; // skip: no usable target/tmp + } + let Some(big_value) = tmpfs_to_target_failing_xattr_value(&dest_dir) else { + std_fs::remove_dir_all(&dest_dir).ok(); + return; // skip: this filesystem combination cannot produce the failure + }; + + let source = format!("/dev/shm/cp_keep_dest_{pid}"); + if std_fs::write(&source, "kept content").is_err() { + std_fs::remove_dir_all(&dest_dir).ok(); + return; } - let source_accepts = Command::new("setfattr") + if !Command::new("setfattr") .args(["-n", "user.huge", "-v", &big_value, &source]) .status() - .is_ok_and(|s| s.success()); - let probe = dest_dir.join("probe"); - std_fs::write(&probe, "x").unwrap(); - let dest_rejects = !Command::new("setfattr") - .args(["-n", "user.huge", "-v", &big_value]) - .arg(&probe) - .status() - .is_ok_and(|s| s.success()); - if !source_accepts || !dest_rejects { + .is_ok_and(|s| s.success()) + { std_fs::remove_file(&source).ok(); std_fs::remove_dir_all(&dest_dir).ok(); - return; // skip: this filesystem combination cannot produce the failure + return; } let out = dest_dir.join("out"); @@ -9524,7 +9521,7 @@ fn test_cp_xattr_failure_keeps_dest_contents() { .arg(&source) .arg(&out) .fails() - .stderr_contains("setting attributes"); + .stderr_contains("setting attribute 'user.huge'"); assert_eq!(std_fs::read_to_string(&out).unwrap(), "kept content"); // A read-only source propagates its mode to the destination; the failure @@ -9533,11 +9530,12 @@ fn test_cp_xattr_failure_keeps_dest_contents() { let out_ro = dest_dir.join("out_ro"); scene .ucmd() + .umask(0o022) .arg("--preserve=xattr") .arg(&source) .arg(&out_ro) .fails() - .stderr_contains("setting attributes"); + .stderr_contains("setting attribute 'user.huge'"); assert_eq!(std_fs::read_to_string(&out_ro).unwrap(), "kept content"); assert_eq!( std_fs::metadata(&out_ro).unwrap().mode() & 0o777, diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 2a33c8d9c2..e2b3096d36 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -3172,8 +3172,39 @@ fn test_mv_xattr_enotsup_silent() { } /// Cross-device mv of a directory must preserve the directory's own xattrs. -/// The fd-based xattr path has to open the destination read-only: a directory -/// cannot be opened for writing, so a write-mode open would silently drop them. +#[cfg(target_os = "linux")] +fn assert_xattr_value(path: &Path, name: &str, expected: &[u8]) { + use std::process::Command; + let out = Command::new("getfattr") + .args(["-n", name, "--only-values", "--absolute-names"]) + .arg(path) + .output() + .expect("getfattr failed"); + assert!( + out.status.success(), + "xattr '{name}' was lost on {}: {}", + path.display(), + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(out.stdout, expected); +} + +#[cfg(target_os = "linux")] +fn assert_no_xattr(path: &Path, name: &str) { + use std::process::Command; + let out = Command::new("getfattr") + .args(["-n", name, "--only-values", "--absolute-names"]) + .arg(path) + .output() + .expect("getfattr failed"); + assert!( + !out.status.success(), + "xattr '{name}' should not be present on {}", + path.display() + ); +} + +/// Cross-device mv of a directory must preserve the directory's own xattrs. #[test] #[cfg(target_os = "linux")] fn test_mv_cross_device_dir_xattr_preserved() { @@ -3212,21 +3243,134 @@ fn test_mv_cross_device_dir_xattr_preserved() { .succeeds() .no_stderr(); - let out = Command::new("getfattr") - .args([ - "-n", - "user.dirattr", - "--only-values", - dst_path.to_str().unwrap(), - ]) - .output() - .expect("failed to run getfattr on the moved directory"); + assert_xattr_value(dst_path.as_path(), "user.dirattr", b"dirvalue"); +} + +/// A failed xattr on a cross-device move must not stop the remaining +/// attributes from being copied to the destination. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_xattr_partial_failure_keeps_remaining() { + use std::path::PathBuf; + use std::process::Command; + use uutests::util::tmpfs_to_target_failing_xattr_value; + + let pid = std::process::id(); + let source_dir = Path::new("/dev/shm").join(format!("mv_xattr_partial_{pid}")); + let dest_dir = + PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(format!("mv_xattr_partial_{pid}")); + if std::fs::create_dir(&source_dir).is_err() || std::fs::create_dir(&dest_dir).is_err() { + return; // skip: no usable /dev/shm or target/tmp + } + let Some(big_value) = tmpfs_to_target_failing_xattr_value(&dest_dir) else { + std::fs::remove_dir_all(&source_dir).ok(); + std::fs::remove_dir_all(&dest_dir).ok(); + return; // skip: this filesystem combination cannot produce the failure + }; + + // Set small attributes around the failing big attribute so that + // regardless of filesystem listing order (alphabetical, insertion, + // or reverse-insertion), at least one surviving attribute is + // processed after the failing one. + let source = source_dir.join("src"); + std::fs::write(&source, "data").unwrap(); + Command::new("setfattr") + .args(["-n", "user.a_small", "-v", "12345678"]) + .arg(&source) + .status() + .unwrap(); + Command::new("setfattr") + .args(["-n", "user.m_big", "-v", &big_value]) + .arg(&source) + .status() + .unwrap(); + Command::new("setfattr") + .args(["-n", "user.z_small", "-v", "87654321"]) + .arg(&source) + .status() + .unwrap(); + + let dest = dest_dir.join("dst"); + let scene = TestScenario::new(util_name!()); + scene + .ucmd() + .arg(&source) + .arg(&dest) + .succeeds() + .stderr_contains("setting attribute 'user.m_big'"); assert!( - out.status.success(), - "directory xattr was not preserved across devices: {}", - String::from_utf8_lossy(&out.stderr) + !source.exists(), + "the source must be removed even when an xattr fails" + ); + + assert_xattr_value(&dest, "user.a_small", b"12345678"); + assert_xattr_value(&dest, "user.z_small", b"87654321"); + assert_no_xattr(&dest, "user.m_big"); + + std::fs::remove_dir_all(&source_dir).ok(); + std::fs::remove_dir_all(&dest_dir).ok(); +} + +/// Partial xattr failure on a cross-device directory move still completes +/// and preserves surviving attributes. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_dir_xattr_partial_failure_completes() { + use std::path::PathBuf; + use std::process::Command; + use uutests::util::tmpfs_to_target_failing_xattr_value; + + let pid = std::process::id(); + let source_dir = Path::new("/dev/shm").join(format!("mv_dir_xattr_partial_{pid}")); + let dest_dir = + PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(format!("mv_dir_xattr_partial_{pid}")); + if std::fs::create_dir(&source_dir).is_err() || std::fs::create_dir(&dest_dir).is_err() { + return; // skip: no usable /dev/shm or target/tmp + } + let Some(big_value) = tmpfs_to_target_failing_xattr_value(&dest_dir) else { + std::fs::remove_dir_all(&source_dir).ok(); + std::fs::remove_dir_all(&dest_dir).ok(); + return; // skip: this filesystem combination cannot produce the failure + }; + + std::fs::write(source_dir.join("f.txt"), "content").unwrap(); + Command::new("setfattr") + .args(["-n", "user.a_small", "-v", "12345678"]) + .arg(&source_dir) + .status() + .unwrap(); + Command::new("setfattr") + .args(["-n", "user.m_big", "-v", &big_value]) + .arg(&source_dir) + .status() + .unwrap(); + Command::new("setfattr") + .args(["-n", "user.z_small", "-v", "87654321"]) + .arg(&source_dir) + .status() + .unwrap(); + + let dest = dest_dir.join("dst_dir"); + let scene = TestScenario::new(util_name!()); + scene + .ucmd() + .arg(&source_dir) + .arg(&dest) + .succeeds() + .stderr_contains("setting attribute 'user.m_big'"); + assert!( + !source_dir.exists(), + "the source directory must be removed even when an xattr fails" ); - assert_eq!(out.stdout, b"dirvalue"); + assert!( + dest.join("f.txt").exists(), + "directory contents must survive" + ); + + assert_xattr_value(&dest, "user.a_small", b"12345678"); + assert_xattr_value(&dest, "user.z_small", b"87654321"); + + std::fs::remove_dir_all(&dest_dir).ok(); } /// Cross-device mv of a symlink onto an existing file must replace the diff --git a/tests/uutests/src/lib/util.rs b/tests/uutests/src/lib/util.rs index bc04d45957..59e08a4e99 100644 --- a/tests/uutests/src/lib/util.rs +++ b/tests/uutests/src/lib/util.rs @@ -1019,6 +1019,42 @@ pub fn compare_xattrs>(path1: P, path2: P) -> bool { get_sorted_xattrs(path1) == get_sorted_xattrs(path2) } +/// Size and content of an extended attribute value that `/dev/shm` (tmpfs) accepts +/// while the given destination directory rejects it. +/// +/// This produces a mismatch where an attribute can be set on a tmpfs source but +/// fails to copy onto the destination filesystem. Returns `None` when this +/// machine's filesystem combination cannot produce that failure, in which case +/// the test should be skipped. +#[cfg(target_os = "linux")] +pub fn tmpfs_to_target_failing_xattr_value>(dest_dir: P) -> Option { + use std::process::Command; + + for size in [9_100, 40_000] { + let value = "y".repeat(size); + let source_probe = Path::new("/dev/shm").join(format!("xattr_probe_{size}")); + let dest_probe = dest_dir.as_ref().join(format!("probe_{size}")); + fs::write(&source_probe, "x").ok(); + fs::write(&dest_probe, "x").ok(); + let source_accepts = Command::new("setfattr") + .args(["-n", "user.huge", "-v", &value]) + .arg(&source_probe) + .status() + .is_ok_and(|s| s.success()); + let dest_rejects = !Command::new("setfattr") + .args(["-n", "user.huge", "-v", &value]) + .arg(&dest_probe) + .status() + .is_ok_and(|s| s.success()); + remove_file(&source_probe).ok(); + remove_file(&dest_probe).ok(); + if source_accepts && dest_rejects { + return Some(value); + } + } + None +} + /// Object-oriented path struct that represents and operates on /// paths relative to the directory it was constructed for. #[derive(Clone)]