diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 0d47ab72f37..9942ba6d6ae 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1847,6 +1847,9 @@ fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyRe } else { copy_xattrs(source, dest) }; + // Every attribute has been tried; report the first one that failed. + let copy_xattrs_result = copy_xattrs_result + .and_then(|failed| failed.into_iter().next().map_or(Ok(()), |(_, e)| Err(e))); // Restore read-only if we changed it. if was_readonly { diff --git a/src/uu/mv/locales/en-US.ftl b/src/uu/mv/locales/en-US.ftl index e67d37ab349..be6462b0b31 100644 --- a/src/uu/mv/locales/en-US.ftl +++ b/src/uu/mv/locales/en-US.ftl @@ -38,6 +38,7 @@ mv-error-dangling-symlink = can't determine symlink type, since it is dangling mv-error-no-symlink-support = your operating system does not support symlinks mv-error-permission-denied = Permission denied mv-error-inter-device-move-failed = inter-device move failed: {$from} to {$to}; unable to remove target: {$err} +mv-error-setting-attribute = setting attribute {$name}: {$err} mv-error-exchange-two-operands = --exchange requires exactly two operands mv-error-exchange-not-supported = --exchange is not supported on this platform diff --git a/src/uu/mv/locales/fr-FR.ftl b/src/uu/mv/locales/fr-FR.ftl index a9d6f01bf6d..abf3f552ac9 100644 --- a/src/uu/mv/locales/fr-FR.ftl +++ b/src/uu/mv/locales/fr-FR.ftl @@ -38,6 +38,7 @@ mv-error-dangling-symlink = impossible de déterminer le type de lien symbolique mv-error-no-symlink-support = votre système d'exploitation ne prend pas en charge les liens symboliques mv-error-permission-denied = Permission refusée mv-error-inter-device-move-failed = échec du déplacement inter-périphérique : {$from} vers {$to} ; impossible de supprimer la cible : {$err} +mv-error-setting-attribute = définition de l'attribut {$name} : {$err} mv-error-exchange-two-operands = --exchange nécessite exactement deux opérandes mv-error-exchange-not-supported = --exchange n'est pas pris en charge sur cette plateforme diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 67a02e03a6f..68cc9c21887 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -1051,7 +1051,9 @@ fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> { target_os = "netbsd" ))] { - let _ = fsxattr::copy_xattrs_ignore_unsupported(from, to); + if let Ok(failed) = fsxattr::copy_xattrs_ignore_unsupported(from, to) { + show_xattr_failures(failed); + } } let _ = preserve_ownership(from, to); fs::remove_file(from) @@ -1384,7 +1386,9 @@ fn copy_file_with_hardlinks_helper( target_os = "netbsd" ))] { - let _ = fsxattr::copy_xattrs_ignore_unsupported(from, to); + if let Ok(failed) = fsxattr::copy_xattrs_ignore_unsupported(from, to) { + show_xattr_failures(failed); + } } // Preserve ownership (uid/gid) from the source let _ = preserve_ownership(from, to); @@ -1455,7 +1459,9 @@ fn rename_file_fallback( target_os = "netbsd" ))] { - let _ = fsxattr::copy_xattrs_fd_ignore_unsupported(&src_file, &dst_file); + if let Ok(failed) = fsxattr::copy_xattrs_fd_ignore_unsupported(&src_file, &dst_file) { + show_xattr_failures(failed); + } } // chown before chmod: chown(2) clears setuid/setgid for non-root, @@ -1485,6 +1491,31 @@ fn rename_file_fallback( Ok(()) } +/// Report each xattr that a cross-device move could not copy. Like GNU, these +/// are only warnings: the move itself still succeeds. +#[cfg(any( + target_os = "freebsd", + target_os = "hurd", + target_os = "linux", + target_os = "android", + target_os = "netbsd" +))] +fn show_xattr_failures(failed: Vec<(OsString, io::Error)>) { + use uucore::error::strip_errno; + use uucore::show_error; + + for (name, err) in failed { + show_error!( + "{}", + translate!( + "mv-error-setting-attribute", + "name" => name.quote(), + "err" => strip_errno(&err) + ) + ); + } +} + /// Preserve ownership (uid/gid) from source to destination. /// Uses lchown so it works on symlinks without following them. /// Errors are silently ignored for non-root users who cannot chown. diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index b180cf9a654..373fd1b770a 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -29,64 +29,100 @@ fn is_xattr_unsupported(_err: &std::io::Error) -> bool { false } -/// Copies extended attributes (xattrs) from one path to another. -/// All errors propagate, including `ENOTSUP` / `EOPNOTSUPP`; for -/// best-effort callers see [`copy_xattrs_ignore_unsupported`]. -pub fn copy_xattrs>(source: P, dest: P) -> std::io::Result<()> { - for attr_name in xattr::list(&source)? { - if let Some(value) = xattr::get(&source, &attr_name)? { - xattr::set(&dest, &attr_name, &value)?; +/// The result of copying xattrs: each attribute that could not be copied, +/// along with its error. Only failing to list the attributes of the source +/// is an `Err`. +pub type XattrCopyResult = std::io::Result>; + +/// Leaves `ENOTSUP` / `EOPNOTSUPP` out of the result of copying xattrs, for +/// callers where xattr preservation is best-effort. +fn without_unsupported(result: XattrCopyResult) -> XattrCopyResult { + match result { + Ok(mut failed) => { + failed.retain(|(_, e)| !is_xattr_unsupported(e)); + Ok(failed) } + Err(e) if is_xattr_unsupported(&e) => Ok(Vec::new()), + Err(e) => Err(e), } - Ok(()) } -/// Like [`copy_xattrs`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())` -/// for callers where xattr preservation is best-effort. -pub fn copy_xattrs_ignore_unsupported>(source: P, dest: P) -> std::io::Result<()> { - match copy_xattrs(source, dest) { - Err(e) if is_xattr_unsupported(&e) => Ok(()), - res => res, +/// Copies each attribute in `names` that `keep` accepts, reading its value +/// with `get` and writing it with `set`. An attribute that cannot be copied +/// does not stop the others; it is returned along with its error. +fn copy_each_xattr( + names: impl IntoIterator, + keep: impl Fn(&OsStr) -> bool, + get: impl Fn(&OsStr) -> std::io::Result>>, + set: impl Fn(&OsStr, &[u8]) -> std::io::Result<()>, +) -> Vec<(OsString, std::io::Error)> { + let mut failed = Vec::new(); + for attr_name in names.into_iter().filter(|name| keep(name)) { + let result = get(&attr_name).and_then(|value| match value { + Some(value) => set(&attr_name, &value), + None => Ok(()), + }); + if let Err(e) = result { + failed.push((attr_name, e)); + } } + failed +} + +/// Copies extended attributes (xattrs) from one path to another. +/// +/// An attribute that cannot be copied does not stop the others from being +/// copied. Returns each attribute that failed along with its error, including +/// `ENOTSUP` / `EOPNOTSUPP`; for best-effort callers see +/// [`copy_xattrs_ignore_unsupported`]. Only failing to list the attributes of +/// `source` is an `Err`. +pub fn copy_xattrs>(source: P, dest: P) -> XattrCopyResult { + Ok(copy_each_xattr( + xattr::list(&source)?, + |_| true, + |name| xattr::get(&source, name), + |name, value| xattr::set(&dest, name, value), + )) +} + +/// Like [`copy_xattrs`], but leaves out `ENOTSUP` / `EOPNOTSUPP` for callers +/// where xattr preservation is best-effort. +pub fn copy_xattrs_ignore_unsupported>(source: P, dest: P) -> XattrCopyResult { + without_unsupported(copy_xattrs(source, dest)) } /// 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`]. +/// the path-based [`copy_xattrs`], and reports failures the same way. #[cfg(unix)] -pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> std::io::Result<()> { +pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> XattrCopyResult { use xattr::FileExt; - for attr_name in source.list_xattr()? { - if let Some(value) = source.get_xattr(&attr_name)? { - dest.set_xattr(&attr_name, &value)?; - } - } - Ok(()) + Ok(copy_each_xattr( + source.list_xattr()?, + |_| true, + |name| source.get_xattr(name), + |name, value| dest.set_xattr(name, value), + )) } -/// Like [`copy_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to `Ok(())`. +/// Like [`copy_xattrs_fd`], but leaves out `ENOTSUP` / `EOPNOTSUPP`. #[cfg(unix)] pub fn copy_xattrs_fd_ignore_unsupported( source: &std::fs::File, dest: &std::fs::File, -) -> std::io::Result<()> { - match copy_xattrs_fd(source, dest) { - Err(e) if is_xattr_unsupported(&e) => Ok(()), - res => res, - } +) -> XattrCopyResult { + without_unsupported(copy_xattrs_fd(source, dest)) } /// Like `copy_xattrs`, but skips the security.selinux attribute. #[cfg(unix)] -pub fn copy_xattrs_skip_selinux>(source: P, dest: P) -> std::io::Result<()> { - 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)?; - } - } - Ok(()) +pub fn copy_xattrs_skip_selinux>(source: P, dest: P) -> XattrCopyResult { + Ok(copy_each_xattr( + xattr::list(&source)?, + |name| name.as_bytes() != b"security.selinux", + |name| xattr::get(&source, name), + |name, value| xattr::set(&dest, name, value), + )) } /// Copies only the POSIX ACL xattrs (`system.posix_acl_access` and @@ -335,7 +371,7 @@ mod tests { let test_value = b"test value"; xattr::set(&source_path, test_attr, test_value).unwrap(); - copy_xattrs(&source_path, &dest_path).unwrap(); + assert!(copy_xattrs(&source_path, &dest_path).unwrap().is_empty()); let copied_value = xattr::get(&dest_path, test_attr).unwrap().unwrap(); assert_eq!(copied_value, test_value); @@ -364,7 +400,7 @@ mod tests { .open(&dest_path) .unwrap(); - copy_xattrs_fd(&src, &dst).unwrap(); + assert!(copy_xattrs_fd(&src, &dst).unwrap().is_empty()); let copied = xattr::get(&dest_path, test_attr).unwrap().unwrap(); assert_eq!(copied, test_value); diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index cba1277c68c..905d8d0207e 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -9605,6 +9605,76 @@ fn test_cp_xattr_failure_keeps_dest_contents() { std_fs::remove_dir_all(&dest_dir).ok(); } +/// An xattr the destination refuses must not stop the other xattrs from being +/// copied. cp still reports the failure and exits 1. tmpfs takes large values +/// while ext4 caps a value at one block, so the large attributes fail there +/// and the small ones must survive. +#[test] +#[cfg(target_os = "linux")] +#[cfg_attr( + wasi_runner, + ignore = "WASI sandbox: host paths (/dev/shm) not visible" +)] +fn test_cp_preserve_xattr_failure_keeps_the_rest() { + use rustc_hash::FxHashMap; + use std::ffi::OsStr; + use tempfile::TempDir; + use uucore::fsxattr::{apply_xattrs, retrieve_xattrs}; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let too_big = vec![b'x'; 8000]; + at.touch("probe"); + let probe = FxHashMap::from_iter([(OsString::from("user.probe"), too_big.clone())]); + if apply_xattrs(at.plus("probe"), probe).is_ok() { + println!("test skipped: the destination filesystem accepts large xattr values"); + return; + } + + // tmpfs lists attributes sorted by name, so interleaving the names puts a + // refused attribute before a kept one whichever way the list is sorted. + let attrs: FxHashMap> = [ + ("user.a_kept", b"first".to_vec()), + ("user.b_too_big", too_big.clone()), + ("user.c_kept", b"middle".to_vec()), + ("user.d_too_big", too_big), + ("user.e_kept", b"last".to_vec()), + ] + .into_iter() + .map(|(name, value)| (OsString::from(name), value)) + .collect(); + + let src_dir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); + let file = src_dir.path().join("file"); + let dir = src_dir.path().join("dir"); + std_fs::write(&file, "content").unwrap(); + std_fs::create_dir(&dir).unwrap(); + if apply_xattrs(&file, attrs.clone()).is_err() { + println!("test skipped: /dev/shm does not accept user xattrs"); + return; + } + apply_xattrs(&dir, attrs.clone()).unwrap(); + + // A regular file is copied through file descriptors, a directory by path. + for (src, dest) in [(&file, "file"), (&dir, "dir")] { + scene + .ucmd() + .args(&["-r", "--preserve=xattr"]) + .arg(src) + .arg(dest) + .fails_with_code(1) + .stderr_contains(format!("cp: setting attributes for '{dest}")); + + let copied = retrieve_xattrs(at.plus(dest)).unwrap(); + for name in ["user.a_kept", "user.c_kept", "user.e_kept"] { + let name = OsStr::new(name); + assert_eq!(copied.get(name), attrs.get(name), "{name:?} on {dest}"); + } + } +} + #[test] #[cfg(not(windows))] #[cfg_attr( diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index e2c3f534685..b2bf30e8732 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -3257,6 +3257,74 @@ fn test_mv_cross_device_dir_xattr_preserved() { assert_eq!(out.stdout, b"dirvalue"); } +/// An xattr the destination refuses must not stop the other xattrs from being +/// copied in a cross-device move. Each refused one is reported, but the move +/// still succeeds. tmpfs takes large values while ext4 caps a value at one +/// block, so the large attributes fail there and the small ones must survive. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_xattr_failure_keeps_the_rest() { + use rustc_hash::FxHashMap; + use std::ffi::{OsStr, OsString}; + use tempfile::TempDir; + use uucore::fsxattr::{apply_xattrs, retrieve_xattrs}; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let too_big = vec![b'x'; 8000]; + at.touch("probe"); + let probe = FxHashMap::from_iter([(OsString::from("user.probe"), too_big.clone())]); + if apply_xattrs(at.plus("probe"), probe).is_ok() { + println!("test skipped: the destination filesystem accepts large xattr values"); + return; + } + + // tmpfs lists attributes sorted by name, so interleaving the names puts a + // refused attribute before a kept one whichever way the list is sorted. + let attrs: FxHashMap> = [ + ("user.a_kept", b"first".to_vec()), + ("user.b_too_big", too_big.clone()), + ("user.c_kept", b"middle".to_vec()), + ("user.d_too_big", too_big), + ("user.e_kept", b"last".to_vec()), + ] + .into_iter() + .map(|(name, value)| (OsString::from(name), value)) + .collect(); + + let src_dir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); + let file = src_dir.path().join("file"); + let dir = src_dir.path().join("dir"); + std::fs::write(&file, "content").unwrap(); + std::fs::create_dir(&dir).unwrap(); + std::fs::write(dir.join("file"), "content").unwrap(); + if apply_xattrs(&file, attrs.clone()).is_err() { + println!("test skipped: /dev/shm does not accept user xattrs"); + return; + } + apply_xattrs(dir.join("file"), attrs.clone()).unwrap(); + + // A single file, then a file copied as part of a directory. + for (src, dest, moved) in [(&file, "file", "file"), (&dir, "dir", "dir/file")] { + scene + .ucmd() + .arg(src) + .arg(dest) + .succeeds() + .stderr_contains("mv: setting attribute 'user.b_too_big': ") + .stderr_contains("mv: setting attribute 'user.d_too_big': "); + + assert!(!src.exists()); + let copied = retrieve_xattrs(at.plus(moved)).unwrap(); + for name in ["user.a_kept", "user.c_kept", "user.e_kept"] { + let name = OsStr::new(name); + assert_eq!(copied.get(name), attrs.get(name), "{name:?} on {moved}"); + } + } +} + /// Cross-device mv of a symlink onto an existing file must replace the /// destination atomically, matching GNU. #[test]