Skip to content

cp: fix -p to not preserve xattrs by default #9708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
AnarchistHoneybun wants to merge 14 commits into
base: main
Choose a base branch
from
Open

cp: fix -p to not preserve xattrs by default #9708

AnarchistHoneybun wants to merge 14 commits into from

Conversation

@AnarchistHoneybun
Copy link
Contributor

@AnarchistHoneybun AnarchistHoneybun commented Dec 19, 2025

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.

@AnarchistHoneybun
cp: fix -p to not preserve xattrs by default
68522d9 
Fixes uutils#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.
@github-actions
Copy link

github-actions bot commented Dec 19, 2025

GNU testsuite comparison:

GNU test failed: tests/cp/acl. tests/cp/acl is passing on 'main'. Maybe you have to rebase?

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 19, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #9708 will not alter performance

Comparing AnarchistHoneybun:bug/cp-preserve-xattr-9704 (5d0ec1c) with main (666c6df)

Summary

✅ 139 untouched
⏩ 37 skipped1

Footnotes

  1. 37 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@github-actions
Copy link

github-actions bot commented Dec 19, 2025

GNU testsuite comparison:

GNU test failed: tests/cp/acl. tests/cp/acl is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link

github-actions bot commented Dec 19, 2025

GNU testsuite comparison:

GNU test failed: tests/cp/acl. tests/cp/acl is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/tail/inotify-dir-recreate is now passing!

@github-actions
Copy link

github-actions bot commented Dec 19, 2025

GNU testsuite comparison:

GNU test failed: tests/cp/acl. tests/cp/acl is passing on 'main'. Maybe you have to rebase?

ChrisDryden
ChrisDryden reviewed Dec 19, 2025
View reviewed changes
tests/by-util/test_cp.rs Outdated
);

// mode, ownership, and timestamps should be preserved
#[cfg(not(any(target_os = "freebsd", target_os = "macos")))]
Copy link
Collaborator

@ChrisDryden ChrisDryden Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are redundant because its at the top of the test already

ChrisDryden
ChrisDryden reviewed Dec 19, 2025
View reviewed changes
tests/by-util/test_cp.rs Outdated
use std::process::Command;
use uutests::util::compare_xattrs;

let scene = TestScenario::new(util_name!());
Copy link
Collaborator

@ChrisDryden ChrisDryden Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the at_and_ucmd macro here?

@ChrisDryden
Copy link
Collaborator

ChrisDryden commented Dec 19, 2025

I think the issue that was provided was an oversimplification and luckily the GNU test catches it.

The issue #9704 is correct that cp -p shouldn't preserve general xattrs, the implementation in PR #9708 is too expansive - it also removes ACL preservation, which GNU cp -p does support.

We need to make a fix with granular xattr handling that preserves ACLs but not other xattrs.

@ChrisDryden
Copy link
Collaborator

ChrisDryden commented Dec 19, 2025

Maybe combining your approach with changing of the preserve code to also add ACL xattrs?

  /// Copies only ACL-related xattrs (system.posix_acl_*) from source to dest.
  pub fn copy_acl_xattrs<P: AsRef<Path>>(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(())
  }



  handle_preserve(&attributes.mode, || -> CopyResult<()> {
      // existing mode code...

      // then
      #[cfg(all(unix, not(target_os = "android")))]
      copy_acl_xattrs(source, dest)?;

      Ok(())
  })?;

@ChrisDryden ChrisDryden mentioned this pull request Dec 19, 2025
Open
@AnarchistHoneybun
Copy link
Contributor Author

AnarchistHoneybun commented Dec 20, 2025

i will look into this today

@github-actions
Copy link

github-actions bot commented Dec 20, 2025

GNU testsuite comparison:

GNU test failed: tests/cp/acl. tests/cp/acl is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/tail/inotify-dir-recreate is now passing!

@github-actions
Copy link

github-actions bot commented Dec 20, 2025

GNU testsuite comparison:

GNU test failed: tests/cp/acl. tests/cp/acl is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

AnarchistHoneybun and others added 2 commits December 21, 2025 10:26
@AnarchistHoneybun
Merge branch 'main' into bug/cp-preserve-xattr-9704
92b4ec2
@AnarchistHoneybun
cp: preserve ACL xattrs with -p while excluding others
f0366d3 
implement granular xattr handling to preserve ACLs with -p but not general xattrs
@github-actions
Copy link

github-actions bot commented Dec 21, 2025

GNU testsuite comparison:

Congrats! The gnu test tests/basenc/bounded-memory is now passing!

sylvestre
sylvestre reviewed Dec 21, 2025
View reviewed changes
@github-actions
Copy link

github-actions bot commented Dec 21, 2025

GNU testsuite comparison:

Congrats! The gnu test tests/basenc/bounded-memory is now passing!

@github-actions
Copy link

github-actions bot commented Dec 21, 2025

GNU testsuite comparison:

Congrats! The gnu test tests/basenc/bounded-memory is now passing!

sylvestre
sylvestre reviewed Dec 22, 2025
View reviewed changes
src/uu/cp/src/cp.rs
use std::{fmt, io};
#[cfg(all(unix, not(target_os = "android")))]
use uucore::fsxattr::copy_xattrs;
use uucore::fsxattr::{copy_acl_xattrs, copy_xattrs};
Copy link
Contributor

@sylvestre sylvestre Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clippy warning here :)

Copy link
Contributor Author

@AnarchistHoneybun AnarchistHoneybun Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason this doesn't come up for me when I locally run it, should I be adding the openbsd flag here too, that we have on :1722?

@github-actions
Copy link

github-actions bot commented Dec 23, 2025

GNU testsuite comparison:

Congrats! The gnu test tests/printf/printf-surprise is now passing!
Note: The gnu test tests/dd/no-allocate was skipped on 'main' but is now failing.
Note: The gnu test tests/misc/write-errors was skipped on 'main' but is now failing.

@github-actions
Copy link

github-actions bot commented Jan 5, 2026

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sylvestre sylvestre sylvestre left review comments

@ChrisDryden ChrisDryden ChrisDryden left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cp -p preserves extended attributes

3 participants

@AnarchistHoneybun @ChrisDryden @sylvestre