diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 321dc3c8b5..1d1467f4cf 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), @@ -1423,9 +1427,12 @@ fn show_error_if_needed(error: &CpError) { CpError::NotAllFilesCopied => { // Need to return an error code } - CpError::Skipped(_) => { + CpError::Skipped(_) | CpError::XattrErrorsReported => { // 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*`; only the exit code + // matters now. } // Format IoErrContext using strip_errno to remove "(os error N)" suffix // for GNU-compatible output @@ -1857,12 +1864,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..72a07430ad 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -1146,12 +1146,14 @@ 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). + // Apply xattrs using a file descriptor to avoid TOCTOU races. // // 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. +// + // Per-attribute failures are reported by `apply_xattrs_fd_*` on stderr and + // must not fail the move. The source was already fully copied, so GNU mv + // completes the move and still exits 0. #[cfg(any( target_os = "freebsd", target_os = "hurd", @@ -1162,7 +1164,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 b3762ced37..7ecfd06b1b 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -6,6 +6,9 @@ // spell-checker:ignore getxattr posix_acl_default posix_acl_access ENOTSUP EOPNOTSUPP renamer //! 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}; @@ -16,7 +19,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 @@ -24,20 +27,55 @@ 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 and record it so the copy loop +/// can keep working on the remaining attributes and still fail afterwards. +/// +/// `ENOTSUP` / `EOPNOTSUPP` mean the filesystem simply has no xattr support +/// and are recorded but not reported: best-effort callers map them to `Ok` +/// through the `*_ignore_unsupported` wrappers and must stay quiet. +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(())` @@ -52,15 +90,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(())`. @@ -76,16 +124,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 @@ -155,6 +214,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 /// @@ -192,10 +257,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(())`. @@ -369,6 +437,81 @@ mod tests { assert_eq!(copied, test_value); } + #[test] + #[cfg(target_os = "linux")] + fn test_copy_xattrs_continues_after_failure() { + use std::path::PathBuf; + use std::process::Command; + + // tmpfs accepts large user-xattr values while most disk filesystems + // cap them near the block size. Put the source on /dev/shm and the + // destination on the build filesystem so the first attribute fails to + // copy while the source holds it fine; skip when this machine cannot + // produce that layout. + let pid = std::process::id(); + let source_dir = PathBuf::from(format!("/dev/shm/xattr_copy_fail_{pid}")); + let dest_dir = std::env::temp_dir().join(format!("xattr_copy_fail_{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 temp dir + } + + let mut usable_size = None; + for size in [9_100, 40_000] { + let value = "y".repeat(size); + let source_probe = source_dir.join(format!("probe_{size}")); + let dest_probe = dest_dir.join(format!("probe_{size}")); + std::fs::write(&source_probe, "x").ok(); + std::fs::write(&dest_probe, "x").ok(); + let src_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()); + std::fs::remove_file(&source_probe).ok(); + std::fs::remove_file(&dest_probe).ok(); + if src_accepts && dest_rejects { + usable_size = Some(size); + break; + } + } + let Some(size) = usable_size else { + std::fs::remove_dir_all(&source_dir).ok(); + std::fs::remove_dir_all(&dest_dir).ok(); + return; // skip: this filesystem combination cannot fail the copy + }; + + // 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("source"); + let dest = dest_dir.join("dest"); + std::fs::write(&source, "data").unwrap(); + std::fs::write(&dest, "data").unwrap(); + let big_value = "y".repeat(size); + xattr::set(&source, "user.a_small", b"12345678").unwrap(); + xattr::set(&source, "user.m_big", big_value.as_bytes()).unwrap(); + xattr::set(&source, "user.z_small", b"87654321").unwrap(); + + let result = copy_xattrs(&source, &dest); + assert!(result.is_err(), "the failed attribute must fail the copy"); + + let copied_a = xattr::get(&dest, "user.a_small").unwrap(); + assert_eq!(copied_a.as_deref(), Some(b"12345678".as_slice())); + let copied_z = xattr::get(&dest, "user.z_small").unwrap(); + assert_eq!(copied_z.as_deref(), Some(b"87654321".as_slice())); + let copied_big = xattr::get(&dest, "user.m_big").unwrap(); + assert_eq!(copied_big, None); + + std::fs::remove_dir_all(&source_dir).ok(); + std::fs::remove_dir_all(&dest_dir).ok(); + } + #[test] fn test_apply_and_retrieve_xattrs() { let temp_dir = tempdir().unwrap(); diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 40d50da0b6..dd312e364c 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -9444,7 +9444,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 @@ -9457,7 +9457,7 @@ fn test_cp_xattr_failure_keeps_dest_contents() { .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..0e9fa1dd94 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -10,6 +10,8 @@ use rstest::rstest; use std::io::Write; #[cfg(not(windows))] use std::path::Path; +#[cfg(target_os = "linux")] +use std::path::PathBuf; #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] use uucore::selinux::get_getfattr_output; use uutests::new_ucmd; @@ -3229,6 +3231,217 @@ fn test_mv_cross_device_dir_xattr_preserved() { assert_eq!(out.stdout, b"dirvalue"); } +/// Size of an attribute value that the `/dev/shm` (tmpfs) filesystem accepts +/// while the given destination directory rejects it, plus the value itself. +/// This is what makes the first xattr fail on the destination but not on the +/// source. Returns `None` when this machine's filesystem combination cannot +/// produce that failure, in which case the test should be skipped. +#[cfg(target_os = "linux")] +fn tmpfs_to_target_failing_xattr_value(dest_dir: &Path) -> 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.join(format!("probe_{size}")); + std::fs::write(&source_probe, "x").ok(); + std::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()); + std::fs::remove_file(&source_probe).ok(); + std::fs::remove_file(&dest_probe).ok(); + if source_accepts && dest_rejects { + return Some(value); + } + } + None +} + +/// A failed xattr on a cross-device move must not stop the remaining +/// attributes from being copied: surviving attributes must still make it even +/// though `user.m_big` is rejected by the destination fs. The move +/// itself succeeds, GNU reports the failure on stderr and exits 0. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_xattr_partial_failure_keeps_remaining() { + use std::process::Command; + + 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!( + !source.exists(), + "the source must be removed even when an xattr fails" + ); + + let small_a_out = Command::new("getfattr") + .args(["-n", "user.a_small", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + small_a_out.status.success(), + "user.a_small was lost on the destination: {}", + String::from_utf8_lossy(&small_a_out.stderr) + ); + assert_eq!(small_a_out.stdout, b"12345678"); + + let small_z_out = Command::new("getfattr") + .args(["-n", "user.z_small", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + small_z_out.status.success(), + "user.z_small was lost on the destination: {}", + String::from_utf8_lossy(&small_z_out.stderr) + ); + assert_eq!(small_z_out.stdout, b"87654321"); + + let big_out = Command::new("getfattr") + .args(["-n", "user.m_big", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + !big_out.status.success(), + "user.m_big should have been rejected by the destination fs" + ); + + std::fs::remove_dir_all(&source_dir).ok(); + std::fs::remove_dir_all(&dest_dir).ok(); +} + +/// The same partial-failure behavior must apply to a cross-device directory +/// move: the directory's own surviving xattrs are preserved even when the +/// failing `user.m_big` is rejected. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_dir_xattr_partial_failure_completes() { + use std::process::Command; + + 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!( + dest.join("f.txt").exists(), + "directory contents must survive" + ); + + let small_a_out = Command::new("getfattr") + .args(["-n", "user.a_small", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + small_a_out.status.success(), + "directory user.a_small xattr was lost: {}", + String::from_utf8_lossy(&small_a_out.stderr) + ); + assert_eq!(small_a_out.stdout, b"12345678"); + + let small_z_out = Command::new("getfattr") + .args(["-n", "user.z_small", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + small_z_out.status.success(), + "directory user.z_small xattr was lost: {}", + String::from_utf8_lossy(&small_z_out.stderr) + ); + assert_eq!(small_z_out.stdout, b"87654321"); + + std::fs::remove_dir_all(&dest_dir).ok(); +} + /// Cross-device mv of a symlink onto an existing file must replace the /// destination atomically, matching GNU. #[test]