From 4d48626eeb5f68f051ea67fe56e3f341e870323d Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Tue, 29 Jul 2025 16:28:14 +0800 Subject: [PATCH 1/3] cp: File logic error when preserving attributes for fifos There was a mismatch between the test, the comment, and the code. The intention was to _preserve_ attributes if the source of a fifo/pipe still exists. Perhaps the clearest way is to modify the test (as that avoids code duplication). Also add a basic test for that, if permissions are preserved, the rest should also be preserved correctly. Fixes the first part of #8402. --- src/uu/cp/src/cp.rs | 2 +- tests/by-util/test_cp.rs | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 2cdd79255cf..a4d0cff4df6 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -2446,7 +2446,7 @@ fn copy_file( copy_attributes(&src, dest, &options.attributes)?; } } - } else if source_is_stream && source.exists() { + } else if source_is_stream && !source.exists() { // Some stream files may not exist after we have copied it, // like anonymous pipes. Thus, we can't really copy its // attributes. However, this is already handled in the stream diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 8575d1959e1..730c7a87e9f 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -4,7 +4,7 @@ // file that was distributed with this source code. // spell-checker:ignore (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs neve ROOTDIR USERDIR outfile uufs xattrs -// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel +// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel prwx use uucore::display::Quotable; use uutests::util::TestScenario; use uutests::{at_and_ucmd, new_ucmd, path_concat, util_name}; @@ -3087,13 +3087,20 @@ fn test_cp_link_backup() { fn test_cp_fifo() { let (at, mut ucmd) = at_and_ucmd!(); at.mkfifo("fifo"); - ucmd.arg("-r") + // Also test that permissions are preserved + at.set_mode("fifo", 0o731); + ucmd.arg("--preserve=mode") + .arg("-r") .arg("fifo") .arg("fifo2") .succeeds() .no_stderr() .no_stdout(); assert!(at.is_fifo("fifo2")); + + let metadata = std::fs::metadata(at.subdir.join("fifo2")).unwrap(); + let permission = uucore::fs::display_permissions(&metadata, true); + assert_eq!(permission, "prwx-wx--x".to_string()); } #[test] From 4b11692db30d1501511a548ecdcece6383433941 Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Wed, 30 Jul 2025 16:08:42 +0800 Subject: [PATCH 2/3] cp: Also copy attributes in copy_symlink Most attributes can actually be applied to symlinks (mode and xattr are exceptions), so try to copy them, if possible. Fort those 2 exceptions, we don't need any special handling as: - Mode errors would be ignored anyway. - The source symlink cannot have any xattr anyway, so no attribute is attempted to be set on the destination. --- src/uu/cp/src/copydir.rs | 2 +- src/uu/cp/src/cp.rs | 6 ++- src/uucore/src/lib/features/perms.rs | 9 ++++ tests/by-util/test_cp.rs | 72 +++++++++++++++++++++++++++- 4 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index b11a5a2bd37..f05777cb485 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -227,7 +227,7 @@ fn copy_direntry( // If the source is a symbolic link and the options tell us not to // dereference the link, then copy the link object itself. if source_absolute.is_symlink() && !options.dereference { - return copy_link(&source_absolute, &local_to_target, symlinked_files); + return copy_link(&source_absolute, &local_to_target, symlinked_files, options); } // If the source is a directory and the destination does not diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index a4d0cff4df6..e826ac3c470 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -2563,7 +2563,7 @@ fn copy_helper( #[cfg(unix)] copy_fifo(dest, options.overwrite, options.debug)?; } else if source_is_symlink { - copy_link(source, dest, symlinked_files)?; + copy_link(source, dest, symlinked_files, options)?; } else { let copy_debug = copy_on_write( source, @@ -2600,6 +2600,7 @@ fn copy_link( source: &Path, dest: &Path, symlinked_files: &mut HashSet, + options: &Options, ) -> CopyResult<()> { // Here, we will copy the symlink itself (actually, just recreate it) let link = fs::read_link(source)?; @@ -2608,7 +2609,8 @@ fn copy_link( if dest.is_symlink() || dest.is_file() { fs::remove_file(dest)?; } - symlink_file(&link, dest, symlinked_files) + symlink_file(&link, dest, symlinked_files)?; + copy_attributes(source, dest, &options.attributes) } /// Generate an error message if `target` is not the correct `target_type` diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 52da95cd1bf..72fb0b27caa 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -41,6 +41,15 @@ pub struct Verbosity { pub level: VerbosityLevel, } +impl Default for Verbosity { + fn default() -> Self { + Self { + groups_only: false, + level: VerbosityLevel::Normal, + } + } +} + /// Actually perform the change of owner on a path fn chown>(path: P, uid: uid_t, gid: gid_t, follow: bool) -> IOResult<()> { let path = path.as_ref(); diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 730c7a87e9f..432cff31318 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -4,7 +4,7 @@ // file that was distributed with this source code. // spell-checker:ignore (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs neve ROOTDIR USERDIR outfile uufs xattrs -// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel prwx +// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel prwx doesnotexist use uucore::display::Quotable; use uutests::util::TestScenario; use uutests::{at_and_ucmd, new_ucmd, path_concat, util_name}; @@ -3103,6 +3103,76 @@ fn test_cp_fifo() { assert_eq!(permission, "prwx-wx--x".to_string()); } +#[cfg(unix)] +#[cfg(not(target_vendor = "apple"))] +fn find_other_group(current: u32) -> Option { + // Get the first group that doesn't match current + nix::unistd::getgroups().ok()?.iter().find_map(|group| { + let gid = group.as_raw(); + (gid != current).then_some(gid) + }) +} + +#[cfg(target_vendor = "apple")] +fn find_other_group(_current: u32) -> Option { + None +} + +#[test] +#[cfg(unix)] +fn test_cp_r_symlink() { + let (at, mut ucmd) = at_and_ucmd!(); + // Specifically test copying a link in a subdirectory, as the internal path + // is slightly different. + at.mkdir("tmp"); + // Create a symlink to a non-existent file to make sure + // we don't try to resolve it. + at.symlink_file("doesnotexist", "tmp/symlink"); + let symlink = at.subdir.join("tmp").join("symlink"); + + // If we can find such a group, change the owner to a non-default to test + // that (group) ownership is preserved. + let metadata = std::fs::symlink_metadata(&symlink).unwrap(); + let other_gid = find_other_group(metadata.gid()); + if let Some(gid) = other_gid { + uucore::perms::wrap_chown( + &symlink, + &metadata, + None, + Some(gid), + false, + uucore::perms::Verbosity::default(), + ) + .expect("Cannot chgrp symlink."); + } else { + println!("Cannot find a second group to chgrp to."); + } + + // Use -r to make sure we copy the symlink itself + // --preserve will include ownership + ucmd.arg("--preserve") + .arg("-r") + .arg("tmp") + .arg("tmp2") + .succeeds() + .no_stderr() + .no_stdout(); + + // Is symlink2 still a symlink, and does it point at the same place? + assert!(at.is_symlink("tmp2/symlink")); + let symlink2 = at.subdir.join("tmp2/symlink"); + assert_eq!( + std::fs::read_link(&symlink).unwrap(), + std::fs::read_link(&symlink2).unwrap(), + ); + + // If we found a suitable group, is the group correct after the copy. + if let Some(gid) = other_gid { + let metadata2 = std::fs::symlink_metadata(&symlink2).unwrap(); + assert_eq!(metadata2.gid(), gid); + } +} + #[test] fn test_dir_recursive_copy() { let scene = TestScenario::new(util_name!()); From f4bf8987cd5d0eed77b54fb3fe45cadaa8791034 Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Wed, 30 Jul 2025 21:04:03 +0800 Subject: [PATCH 3/3] tests/by-util/test_cp.rs: Merge #[cfg] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Dorian Péron <72708393+RenjiSann@users.noreply.github.com> --- tests/by-util/test_cp.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 432cff31318..03727365292 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -3103,8 +3103,7 @@ fn test_cp_fifo() { assert_eq!(permission, "prwx-wx--x".to_string()); } -#[cfg(unix)] -#[cfg(not(target_vendor = "apple"))] +#[cfg(all(unix, not(target_vendor = "apple")))] fn find_other_group(current: u32) -> Option { // Get the first group that doesn't match current nix::unistd::getgroups().ok()?.iter().find_map(|group| {