From 68522d95fd5a63683b3cf89e022f2bc8a3629b80 Mon Sep 17 00:00:00 2001 From: AnarchistHoneybun Date: Fri, 19 Dec 2025 09:39:54 +0530 Subject: [PATCH 1/4] cp: fix -p to not preserve xattrs by default Fixes #9704 The -p flag should only preserve mode, ownership, and timestamps, matching GNU cp behavior. Extended attributes require explicit --preserve=xattr or -a (archive mode). Changed Attributes::DEFAULT to set xattr preservation to No, preventing unintended security risks and filesystem compatibility issues. Adds regression test. --- src/uu/cp/src/cp.rs | 2 +- tests/by-util/test_cp.rs | 106 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index c1df9ed13..ea65aae0f 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -897,7 +897,7 @@ impl Attributes { ownership: Preserve::Yes { required: true }, mode: Preserve::Yes { required: true }, timestamps: Preserve::Yes { required: true }, - xattr: Preserve::Yes { required: true }, + xattr: Preserve::No { explicit: false }, ..Self::NONE }; diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index c5d1f9390..f1507c344 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1773,6 +1773,112 @@ fn test_cp_preserve_xattr() { } } +// -p preserves mode, ownership, and timestamps, but not xattrs +// xattrs require explicit --preserve=xattr or -a +#[test] +#[cfg(all( + unix, + not(any(target_os = "android", target_os = "macos", target_os = "openbsd")) +))] +fn test_cp_preserve_default_no_xattr() { + use std::process::Command; + use uutests::util::compare_xattrs; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let source_file = "source_with_xattr"; + let dest_file_p = "dest_with_p_flag"; + let dest_file_explicit = "dest_with_explicit_xattr"; + + at.touch(source_file); + + // set xattr on source + let xattr_key = "user.test_preserve"; + let xattr_value = "test_value"; + match Command::new("setfattr") + .args([ + "-n", + xattr_key, + "-v", + xattr_value, + &at.plus_as_string(source_file), + ]) + .status() + .map(|status| status.code()) + { + Ok(Some(0)) => {} + Ok(_) => { + println!("test skipped: setfattr failed"); + return; + } + Err(e) => { + println!("test skipped: setfattr failed with {e}"); + return; + } + } + + // verify xattr was set + let getfattr_output = Command::new("getfattr") + .args([&at.plus_as_string(source_file)]) + .output() + .expect("failed to run getfattr"); + + assert!( + getfattr_output.status.success(), + "getfattr failed: {}", + String::from_utf8_lossy(&getfattr_output.stderr) + ); + + let stdout = String::from_utf8_lossy(&getfattr_output.stdout); + assert!( + stdout.contains(xattr_key), + "xattr not found on source: {xattr_key}" + ); + + // -p should not preserve xattrs + scene + .ucmd() + .args(&[ + "-p", + &at.plus_as_string(source_file), + &at.plus_as_string(dest_file_p), + ]) + .succeeds() + .no_output(); + + // xattrs should not be copied + assert!( + !compare_xattrs(&at.plus(source_file), &at.plus(dest_file_p)), + "cp -p incorrectly preserved xattrs" + ); + + // mode, ownership, and timestamps should be preserved + #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] + { + let metadata_src = at.metadata(source_file); + let metadata_dst = at.metadata(dest_file_p); + assert_metadata_eq!(metadata_src, metadata_dst); + } + + // --preserve=xattr should explicitly preserve xattrs + scene + .ucmd() + .args(&[ + "--preserve=xattr", + &at.plus_as_string(source_file), + &at.plus_as_string(dest_file_explicit), + ]) + .succeeds() + .no_output(); + + // xattrs should be copied + assert!( + compare_xattrs(&at.plus(source_file), &at.plus(dest_file_explicit)), + "cp --preserve=xattr did not preserve xattrs" + ); +} + #[test] #[cfg(all(target_os = "linux", not(feature = "feat_selinux")))] fn test_cp_preserve_all_context_fails_on_non_selinux() { From f0366d3949353888bd00dab6b2ff2fc745b6bccd Mon Sep 17 00:00:00 2001 From: AnarchistHoneybun Date: Sun, 21 Dec 2025 10:40:38 +0530 Subject: [PATCH 2/4] cp: preserve ACL xattrs with -p while excluding others implement granular xattr handling to preserve ACLs with -p but not general xattrs --- src/uu/cp/src/cp.rs | 8 +++++++- src/uucore/src/lib/features/fsxattr.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index ea65aae0f..137bc18d0 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -16,7 +16,7 @@ use std::os::unix::net::UnixListener; use std::path::{Path, PathBuf, StripPrefixError}; use std::{fmt, io}; #[cfg(all(unix, not(target_os = "android")))] -use uucore::fsxattr::copy_xattrs; +use uucore::fsxattr::{copy_acl_xattrs, copy_xattrs}; use uucore::translate; use clap::{Arg, ArgAction, ArgMatches, Command, builder::ValueParser, value_parser}; @@ -1715,6 +1715,12 @@ pub(crate) fn copy_attributes( exacl::getfacl(source, None) .and_then(|acl| exacl::setfacl(&[dest], &acl, None)) .map_err(|err| CpError::Error(err.to_string()))?; + + // When preserving mode with -p, also preserve ACL xattrs + // ACLs are stored as xattrs (system.posix_acl_*) and should be + // preserved even when general xattrs are not + #[cfg(all(unix, not(target_os = "android")))] + copy_acl_xattrs(source, dest)?; } Ok(()) diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index 1f1356ee5..2dce1e899 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -29,6 +29,32 @@ pub fn copy_xattrs>(source: P, dest: P) -> std::io::Result<()> { Ok(()) } +/// Copies only ACL-related xattrs (system.posix_acl_*) from source to dest. +/// +/// This is used for preserving ACLs with `-p` flag, which should preserve +/// ACLs but not other extended attributes. +/// +/// # Arguments +/// +/// * `source` - A reference to the source path. +/// * `dest` - A reference to the destination path. +/// +/// # Returns +/// +/// A result indicating success or failure. +pub fn copy_acl_xattrs>(source: P, dest: P) -> std::io::Result<()> { + for attr_name in xattr::list(&source)? { + if let Some(name_str) = attr_name.to_str() { + if name_str.starts_with("system.posix_acl_") { + if let Some(value) = xattr::get(&source, &attr_name)? { + xattr::set(&dest, &attr_name, &value)?; + } + } + } + } + Ok(()) +} + /// Retrieves the extended attributes (xattrs) of a given file or directory. /// /// # Arguments From 7305e5191ed04029f5359e16b812bd612ce0778c Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 21 Dec 2025 09:36:58 +0100 Subject: [PATCH 3/4] opnbsd isn't supported --- src/uu/cp/src/cp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 137bc18d0..8f4ff1a3c 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1719,7 +1719,7 @@ pub(crate) fn copy_attributes( // When preserving mode with -p, also preserve ACL xattrs // ACLs are stored as xattrs (system.posix_acl_*) and should be // preserved even when general xattrs are not - #[cfg(all(unix, not(target_os = "android")))] + #[cfg(all(unix, not(any(target_os = "android", target_os = "openbsd"))))] copy_acl_xattrs(source, dest)?; } From 78b5b5ff9e8aea2f54a2a6337066892f089cde4d Mon Sep 17 00:00:00 2001 From: AnarchistHoneybun Date: Tue, 23 Dec 2025 08:50:08 +0530 Subject: [PATCH 4/4] tests: use at_and_ucmd macro and remove redundant cfg guards --- tests/by-util/test_cp.rs | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 4512f86e1..875898e19 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1801,8 +1801,7 @@ fn test_cp_preserve_default_no_xattr() { use std::process::Command; use uutests::util::compare_xattrs; - let scene = TestScenario::new(util_name!()); - let at = &scene.fixtures; + let (at, mut ucmd) = at_and_ucmd!(); let source_file = "source_with_xattr"; let dest_file_p = "dest_with_p_flag"; @@ -1854,15 +1853,13 @@ fn test_cp_preserve_default_no_xattr() { ); // -p should not preserve xattrs - scene - .ucmd() - .args(&[ - "-p", - &at.plus_as_string(source_file), - &at.plus_as_string(dest_file_p), - ]) - .succeeds() - .no_output(); + ucmd.args(&[ + "-p", + &at.plus_as_string(source_file), + &at.plus_as_string(dest_file_p), + ]) + .succeeds() + .no_output(); // xattrs should not be copied assert!( @@ -1871,16 +1868,12 @@ fn test_cp_preserve_default_no_xattr() { ); // mode, ownership, and timestamps should be preserved - #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] - { - let metadata_src = at.metadata(source_file); - let metadata_dst = at.metadata(dest_file_p); - assert_metadata_eq!(metadata_src, metadata_dst); - } + let metadata_src = at.metadata(source_file); + let metadata_dst = at.metadata(dest_file_p); + assert_metadata_eq!(metadata_src, metadata_dst); // --preserve=xattr should explicitly preserve xattrs - scene - .ucmd() + new_ucmd!() .args(&[ "--preserve=xattr", &at.plus_as_string(source_file),