From 9d2e7fbc2f3e49b68f3fdd0752edccbc7c3a71bc Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Thu, 17 Sep 2026 00:19:26 +0200 Subject: [PATCH 1/3] mv: keep copying xattrs after one fails on a cross-device move --- src/uu/cp/src/cp.rs | 28 +++- src/uu/mv/src/mv.rs | 10 +- src/uucore/src/lib/features/fsxattr.rs | 180 ++++++++++++++++++++++--- tests/by-util/test_cp.rs | 4 +- tests/by-util/test_mv.rs | 174 ++++++++++++++++++++++++ 5 files changed, 365 insertions(+), 31 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 321dc3c8b56..f3fdab24660 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -87,6 +87,12 @@ pub enum CpError { #[error("{}", translate!("cp-error-not-all-files-copied"))] NotAllFilesCopied, + /// Extended-attribute copying failed, but every failure was already + /// reported per-attribute when it happened (see `uucore::fsxattr`). + /// Only the exit code is still needed, so nothing more gets printed. + #[error("")] + XattrErrorsReported, + /// Simple [`walkdir::Error`] wrapper #[error("{0}")] WalkDirErr(#[from] walkdir::Error), @@ -1423,9 +1429,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 @@ -1858,11 +1867,20 @@ fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyRe } // If copying xattrs failed, propagate that error now with context. + // + // `copy_xattrs*` already reported each failing attribute on stderr, so + // only an ENOTSUP-based failure (destination filesystem without xattr + // support) still needs the context added here. Anything else fails + // silently, keeping a single diagnostic per failing attribute. 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 3536c54a737..72a07430ade 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 b3762ced37c..d487ef57577 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,78 @@ 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 + }; + + // user.big is set first so it comes first in list order: when it + // fails, user.small must still make it to the destination. + 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.big", big_value.as_bytes()).unwrap(); + xattr::set(&source, "user.small", b"12345678").unwrap(); + let source_list = xattr::list(&source).unwrap().collect::>(); + assert_eq!(source_list[0], OsString::from("user.big")); + + let result = copy_xattrs(&source, &dest); + assert!(result.is_err(), "the failed attribute must fail the copy"); + + let copied_small = xattr::get(&dest, "user.small").unwrap(); + assert_eq!(copied_small.as_deref(), Some(b"12345678".as_slice())); + let copied_big = xattr::get(&dest, "user.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 40d50da0b64..dd312e364c5 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 2a33c8d9c2f..5f8d519557f 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(not(windows))] +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,178 @@ 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: the later `user.small` must survive even +/// though the earlier `user.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 + }; + + // user.big is set first so it comes first in list order: when it fails, + // user.small must still be copied anyway. + let source = source_dir.join("src"); + std::fs::write(&source, "data").unwrap(); + Command::new("setfattr") + .args(["-n", "user.big", "-v", &big_value]) + .arg(&source) + .status() + .unwrap(); + Command::new("setfattr") + .args(["-n", "user.small", "-v", "12345678"]) + .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.big'"); + assert!( + !source.exists(), + "the source must be removed even when an xattr fails" + ); + + let small_out = Command::new("getfattr") + .args(["-n", "user.small", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + small_out.status.success(), + "user.small was lost on the destination: {}", + String::from_utf8_lossy(&small_out.stderr) + ); + assert_eq!(small_out.stdout, b"12345678"); + + let big_out = Command::new("getfattr") + .args(["-n", "user.big", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + !big_out.status.success(), + "user.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 `user.small` xattr is preserved even when the +/// earlier `user.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.big", "-v", &big_value]) + .arg(&source_dir) + .status() + .unwrap(); + Command::new("setfattr") + .args(["-n", "user.small", "-v", "12345678"]) + .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.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_out = Command::new("getfattr") + .args(["-n", "user.small", "--only-values", "--absolute-names"]) + .arg(&dest) + .output() + .expect("getfattr failed"); + assert!( + small_out.status.success(), + "directory user.small xattr was lost: {}", + String::from_utf8_lossy(&small_out.stderr) + ); + assert_eq!(small_out.stdout, b"12345678"); + + 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] From d2650ea75c3af70662b8ae0e0449b38c7fec0c42 Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Thu, 17 Sep 2026 00:50:10 +0200 Subject: [PATCH 2/3] cp: shorten xattr error comments --- src/uu/cp/src/cp.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index f3fdab24660..1d1467f4cf8 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -87,9 +87,7 @@ pub enum CpError { #[error("{}", translate!("cp-error-not-all-files-copied"))] NotAllFilesCopied, - /// Extended-attribute copying failed, but every failure was already - /// reported per-attribute when it happened (see `uucore::fsxattr`). - /// Only the exit code is still needed, so nothing more gets printed. + /// Xattr copying already reported each failure; only the exit code is needed. #[error("")] XattrErrorsReported, @@ -1866,12 +1864,7 @@ 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 failing attribute on stderr, so - // only an ENOTSUP-based failure (destination filesystem without xattr - // support) still needs the context added here. Anything else fails - // silently, keeping a single diagnostic per failing attribute. + // `copy_xattrs*` already reported each failure; add context only when xattrs are unsupported. copy_xattrs_result.map_err(|e| { if uucore::fsxattr::is_xattr_unsupported(&e) { CpError::IoErrContext( From f61ed64a7435c5f1a6cc43bfa314db1d04a91088 Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Thu, 17 Sep 2026 12:56:55 +0200 Subject: [PATCH 3/3] mv: fix xattr test iteration order and lint issues --- src/uucore/src/lib/features/fsxattr.rs | 21 +++--- tests/by-util/test_mv.rs | 99 ++++++++++++++++++-------- 2 files changed, 81 insertions(+), 39 deletions(-) diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index d487ef57577..7ecfd06b1b9 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -485,24 +485,27 @@ mod tests { return; // skip: this filesystem combination cannot fail the copy }; - // user.big is set first so it comes first in list order: when it - // fails, user.small must still make it to the destination. + // 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.big", big_value.as_bytes()).unwrap(); - xattr::set(&source, "user.small", b"12345678").unwrap(); - let source_list = xattr::list(&source).unwrap().collect::>(); - assert_eq!(source_list[0], OsString::from("user.big")); + 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_small = xattr::get(&dest, "user.small").unwrap(); - assert_eq!(copied_small.as_deref(), Some(b"12345678".as_slice())); - let copied_big = xattr::get(&dest, "user.big").unwrap(); + 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(); diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 5f8d519557f..0e9fa1dd949 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -10,7 +10,7 @@ use rstest::rstest; use std::io::Write; #[cfg(not(windows))] use std::path::Path; -#[cfg(not(windows))] +#[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; @@ -3266,8 +3266,8 @@ fn tmpfs_to_target_failing_xattr_value(dest_dir: &Path) -> Option { } /// A failed xattr on a cross-device move must not stop the remaining -/// attributes from being copied: the later `user.small` must survive even -/// though the earlier `user.big` is rejected by the destination fs. The move +/// 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")] @@ -3287,17 +3287,24 @@ fn test_mv_cross_device_xattr_partial_failure_keeps_remaining() { return; // skip: this filesystem combination cannot produce the failure }; - // user.big is set first so it comes first in list order: when it fails, - // user.small must still be copied anyway. + // 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.big", "-v", &big_value]) + .args(["-n", "user.a_small", "-v", "12345678"]) .arg(&source) .status() .unwrap(); Command::new("setfattr") - .args(["-n", "user.small", "-v", "12345678"]) + .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(); @@ -3309,32 +3316,44 @@ fn test_mv_cross_device_xattr_partial_failure_keeps_remaining() { .arg(&source) .arg(&dest) .succeeds() - .stderr_contains("setting attribute 'user.big'"); + .stderr_contains("setting attribute 'user.m_big'"); assert!( !source.exists(), "the source must be removed even when an xattr fails" ); - let small_out = Command::new("getfattr") - .args(["-n", "user.small", "--only-values", "--absolute-names"]) + let small_a_out = Command::new("getfattr") + .args(["-n", "user.a_small", "--only-values", "--absolute-names"]) .arg(&dest) .output() .expect("getfattr failed"); assert!( - small_out.status.success(), - "user.small was lost on the destination: {}", - String::from_utf8_lossy(&small_out.stderr) + small_a_out.status.success(), + "user.a_small was lost on the destination: {}", + String::from_utf8_lossy(&small_a_out.stderr) ); - assert_eq!(small_out.stdout, b"12345678"); + 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.big", "--only-values", "--absolute-names"]) + .args(["-n", "user.m_big", "--only-values", "--absolute-names"]) .arg(&dest) .output() .expect("getfattr failed"); assert!( !big_out.status.success(), - "user.big should have been rejected by the destination fs" + "user.m_big should have been rejected by the destination fs" ); std::fs::remove_dir_all(&source_dir).ok(); @@ -3342,8 +3361,8 @@ fn test_mv_cross_device_xattr_partial_failure_keeps_remaining() { } /// The same partial-failure behavior must apply to a cross-device directory -/// move: the directory's own `user.small` xattr is preserved even when the -/// earlier `user.big` is rejected. +/// 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() { @@ -3351,8 +3370,8 @@ fn test_mv_cross_device_dir_xattr_partial_failure_completes() { 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}")); + 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 } @@ -3364,12 +3383,17 @@ fn test_mv_cross_device_dir_xattr_partial_failure_completes() { std::fs::write(source_dir.join("f.txt"), "content").unwrap(); Command::new("setfattr") - .args(["-n", "user.big", "-v", &big_value]) + .args(["-n", "user.a_small", "-v", "12345678"]) .arg(&source_dir) .status() .unwrap(); Command::new("setfattr") - .args(["-n", "user.small", "-v", "12345678"]) + .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(); @@ -3381,24 +3405,39 @@ fn test_mv_cross_device_dir_xattr_partial_failure_completes() { .arg(&source_dir) .arg(&dest) .succeeds() - .stderr_contains("setting attribute 'user.big'"); + .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"); + 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_out = Command::new("getfattr") - .args(["-n", "user.small", "--only-values", "--absolute-names"]) + let small_z_out = Command::new("getfattr") + .args(["-n", "user.z_small", "--only-values", "--absolute-names"]) .arg(&dest) .output() .expect("getfattr failed"); assert!( - small_out.status.success(), - "directory user.small xattr was lost: {}", - String::from_utf8_lossy(&small_out.stderr) + small_z_out.status.success(), + "directory user.z_small xattr was lost: {}", + String::from_utf8_lossy(&small_z_out.stderr) ); - assert_eq!(small_out.stdout, b"12345678"); + assert_eq!(small_z_out.stdout, b"87654321"); std::fs::remove_dir_all(&dest_dir).ok(); }