Skip to content

cp: better permissions mode handling with umask awareness - #10904

Open
my4ng wants to merge 6 commits into
uutils:mainfrom
my4ng:cp_mode
Open

my4ng wants to merge 6 commits into
uutils:mainfrom
my4ng:cp_mode

Conversation

@my4ng

@my4ng my4ng commented Feb 13, 2026

Copy link
Copy Markdown
Contributor

Fix #9750, #10011, #10787, #10862, Supersede #10859.

This PR reworks the permissions mode handling such that its behaviour is in line with GNU's implementation. Specifically:

  • set umask to !0o077, which means (1) cp can actually create and work with new directories/files which will have 700 permissions mode before their final calculated mode is set. the original umask is kept and passed around for calculation in the case of newly created and non-mode-preserving dir/files. umask is restored to original on exit.

  • for explicit non-mode-preserving, newly created dir/files, use 777 and 666 modes for calculation respectively.

  • for existing dir/files, leave existing mode unchanged except when preserving mode.

  • for newly created dir/files, always set their permission mode to the correct one.

  • added 12 new tests for the combination of existing/not-existing, no-preserve/default/preserve, dir/file one each for directory and file with rstest.

  • improve performance by eliminating redundant heap allocations (e.g. PathBuf, String), ~25% from benchmark.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/cp/cp-parents. tests/cp/cp-parents is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/tty/tty-eof (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/factor/t34 is no longer failing!
Congrats! The gnu test tests/printf/printf-surprise is now passing!

@ChrisDryden

Copy link
Copy Markdown
Collaborator

Is there any way to split this PR up into something smaller? When its this big it gets pretty hard to review

@my4ng

my4ng commented Feb 13, 2026

Copy link
Copy Markdown
Contributor Author

I'm afraid not. Most changes are actually from the noise of propagating orig_umask, and the only significant changes being build_dir and handle_mode. Without either, it results in a dozen or more failed tests.

There is a GNU test regression: tests/cp/cp-parents, caused by the second to last test in that file due to created parent dir not having attributes set correctly. Will fix soon.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/pr/bounded-memory. tests/pr/bounded-memory is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/symlink (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/factor/t34 is no longer failing!
Note: The gnu test tests/tail/pipe-f is now being skipped but was previously passing.
Congrats! The gnu test tests/printf/printf-surprise is now passing!

Comment thread tests/by-util/test_cp.rs Outdated
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/cut/bounded-memory. tests/cut/bounded-memory is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/io-errors. tests/misc/io-errors is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/pr/bounded-memory. tests/pr/bounded-memory is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/tty/tty-eof (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/factor/t10 is no longer failing!

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/tail/tail-n0f is now passing!

@codspeed

codspeed Bot commented Feb 21, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 50.18%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 5 improved benchmarks
✅ 304 untouched benchmarks
⏩ 46 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation cp_archive_balanced_tree[(5, 4, 10)] 76.8 ms 62.1 ms +23.7%
Simulation cp_recursive_wide_tree[(6000, 800)] 187.8 ms 156.1 ms +20.28%
Simulation cp_preserve_metadata[(5, 4, 10)] 73.7 ms 59.2 ms +24.43%
Simulation cp_recursive_balanced_tree[(5, 4, 10)] 71.4 ms 56.6 ms +26.06%
Simulation cp_recursive_deep_tree[(120, 4)] 13.1 ms 8.7 ms +50.18%

Comparing my4ng:cp_mode (8d082ea) with main (b7e967c)

Open in CodSpeed

Footnotes

  1. 46 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.

@my4ng

my4ng commented Feb 21, 2026

Copy link
Copy Markdown
Contributor Author

There are some residual problems with copy_file that I want to fix. In particular, currently it is setting the permissions (unnecessarily) twice, but the change appears to be more complex than I anticipated. Might split that into a separate PR just for clarity since this one is working and big enough already.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/cut/bounded-memory is now being skipped but was previously passing.
Congrats! The gnu test tests/expand/bounded-memory is now passing!

@my4ng

my4ng commented Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

@ChrisDryden I have condensed the tests into two instead of the original 12, so it should be a lot more workable now.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/misc/io-errors is no longer failing!

@hphilm

hphilm commented Apr 14, 2026

Copy link
Copy Markdown

Seems with this even applied on top of 0.8.0 we have issues in AerynOS with cp -R and cp -a still using the source directory permissions of 770 even umask 0022 is given to get 755 normally. Here our test results:

# bash test.sh

Set my umask to 0022
What's my umask?
0022

whoami?
root

Which cp version are we using?
cp (uutils coreutils) 0.8.0

>> File permissions test:

Create a 0775 | rwxrwxr-x file:
mode of 'file.0775' changed from 0644 (rw-r--r--) to 0775 (rwxrwxr-x)
-rwxrwxr-x 1 root root 0 Apr 14 08:27 file.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'file.0775' -> 'file.0755'
Test assumption:
-rwxr-xr-x 1 root root 0 Apr 14 08:27 file.0755

>> Directory permissions test:

Create a 0775 | rwxrwxr-x directory:
mode of 'dir.0775' changed from 0755 (rwxr-xr-x) to 0775 (rwxrwxr-x)
drwxrwxr-x 1 root root 0 Apr 14 08:27 dir.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'dir.0775' -> 'dir.0755/'
Test assumption:
drwxrwxr-x 1 root root 0 Apr 14 08:27 dir.0755

Script we used to test it:

#!/usr/bin/env bash
echo -e "\nSet my umask to 0022"
umask 0022
echo -e "What's my umask?"
umask
echo -e "\nwhoami?"
whoami
echo -e "\nWhich cp version are we using?"
cp --version

echo -e "\n>> File permissions test:"
echo -e "\nCreate a 0775 | rwxrwxr-x file:"
touch file.0775 && chmod -c u=rwx,g=rwx,o=rx file.0775 && ls -l file.0775
echo -e "\nAssumption: cp -Rv respects 0022 umask whether or not the user is root"
cp -Rfv file.0775 file.0755
echo "Test assumption:"
ls -l file.0755

echo -e "\n>> Directory permissions test:"
echo -e "\nCreate a 0775 | rwxrwxr-x directory:"
[[ -d dir.0775 ]] && rm -rf dir.0775
mkdir dir.0775 && chmod -c u=rwx,g=rwx,o=rx dir.0775 && ls -ld dir.0775
echo -e "\nAssumption: cp -Rv respects 0022 umask whether or not the user is root"
[[ -d dir.0755 ]] && rm -rf dir.0755
cp -Rfv dir.0775 dir.0755
echo "Test assumption:"
ls -ld dir.0755

Using this patch on top of 0.8.0 without your patches restores the 0.6.0 behaviour ...

root@boulder:~/v0.76.0.tar.gz/a
# bash test.sh

Set my umask to 0022
What's my umask?
0022

whoami?
root

Which cp version are we using?
cp (uutils coreutils) 0.8.0

>> File permissions test:

Create a 0775 | rwxrwxr-x file:
mode of 'file.0775' changed from 0644 (rw-r--r--) to 0775 (rwxrwxr-x)
-rwxrwxr-x 1 root root 0 Apr 14 07:49 file.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'file.0775' -> 'file.0755'
Test assumption:
-rwxr-xr-x 1 root root 0 Apr 14 07:49 file.0755

>> Directory permissions test:

Create a 0775 | rwxrwxr-x directory:
mode of 'dir.0775' changed from 0755 (rwxr-xr-x) to 0775 (rwxrwxr-x)
drwxrwxr-x 1 root root 0 Apr 14 07:49 dir.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'dir.0775' -> 'dir.0755/'
Test assumption:
drwxr-xr-x 1 root root 0 Apr 14 07:49 dir.0755

@ermo

ermo commented Apr 15, 2026

Copy link
Copy Markdown

As a follow-up to the above post, it would appear that the basic takeaway from the above test script that I wrote for the AerynOS uutils-coreutils cp investigation (and also tested w/ GNU coreutils cp) is that IFF --preserve is NOT set, THEN the new files or directories being created by cp and cp -R should respect the umask.

IF we assume that source permissions & !umask == max target permissions THEN:

  • umask = 0000: source permissions & !0000 => max target perms: u=7&7=7=rwx, g=7&7=7=rwx, o=7&7=7=rwx
  • umask = 0002: source permissions & !0002 => max target perms: u=7&7=7=rwx, g=7&7=7=rwx, o=7&5=5=r-x
  • umask = 0022: source permissions & !0022 => max target perms: u=7&7=7=rwx, g=7&5=5=r-x, o=7&5=5=r-x
  • umask = 0222: source permissions & !0222 => max target perms: u=7&5=5=r-x, g=7&5=5=r-x, o=7&5=5=r-x

Effectively, if the source permissions do not reach the umask bitfilter "cap", the target permissions are the same as the source permissions.

There may be different implications for the setuid/setgid/sticky bits. Those have not been accounted for here or in the above script.

@hphilm

hphilm commented Apr 16, 2026

Copy link
Copy Markdown

I think that proofing with my patch on the current 0.8.0 code, preserve is set on new created folders regardless and therefore umask ignored completely...

@my4ng

my4ng commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

@hphilm I have rebased my PR to the main branch, and ran your test script and it behaves as expected:

Set my umask to 0022
What's my umask?
0022

whoami?
root

Which cp version are we using?
cp (uutils coreutils) 0.8.0

>> File permissions test:

Create a 0775 | rwxrwxr-x file:
mode of 'file.0775' changed from 0644 (rw-r--r--) to 0775 (rwxrwxr-x)
-rwxrwxr-x. 1 root root 0 Apr 16 16:38 file.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'file.0775' -> 'file.0755'
Test assumption:
-rwxr-xr-x. 1 root root 0 Apr 16 16:38 file.0755

>> Directory permissions test:

Create a 0775 | rwxrwxr-x directory:
mode of 'dir.0775' changed from 0755 (rwxr-xr-x) to 0775 (rwxrwxr-x)
drwxrwxr-x. 1 root root 0 Apr 16 16:38 dir.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'dir.0775' -> 'dir.0755/'
Test assumption:
drwxr-xr-x. 1 root root 0 Apr 16 16:38 dir.0755

All the directory/file tests still pass. Are you using the latest and complete version of this PR?

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Note: The gnu test tests/cut/cut-huge-range is now being skipped but was previously passing.
Note: The gnu test tests/rm/many-dir-entries-vs-OOM is now being skipped but was previously passing.

@hphilm

hphilm commented Apr 16, 2026

Copy link
Copy Markdown

I just pulled the PR again and compiled it against 4e68e67. However, I still see the "wrong" permissions on AerynOS:

root@boulder:~/4e68e67.tar.gz
# bash test.sh

Set my umask to 0022
What's my umask?
0022

whoami?
root

Which cp version are we using?
cp (uutils coreutils) 0.8.0

>> File permissions test:

Create a 0775 | rwxrwxr-x file:
mode of 'file.0775' changed from 0644 (rw-r--r--) to 0775 (rwxrwxr-x)
-rwxrwxr-x 1 root root 0 Apr 16 08:16 file.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'file.0775' -> 'file.0755'
Test assumption:
-rwxr-xr-x 1 root root 0 Apr 16 08:16 file.0755

>> Directory permissions test:

Create a 0775 | rwxrwxr-x directory:
mode of 'dir.0775' changed from 0755 (rwxr-xr-x) to 0775 (rwxrwxr-x)
drwxrwxr-x 1 root root 0 Apr 16 08:16 dir.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'dir.0775' -> 'dir.0755/'
Test assumption:
drwxrwxr-x 1 root root 0 Apr 16 08:16 dir.0755

See my changes here: aerynOS/recipes@3b4ce86
We might need some verbose debug lines to see which permissions are seen and which are set in the end.

@my4ng

my4ng commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

@hphilm Could you please provide the strace for the erring directory copy?

@hphilm

hphilm commented May 7, 2026

Copy link
Copy Markdown

Will test with latest main again as 7ba1bf8 might change some we might need.

@hphilm

hphilm commented May 12, 2026

Copy link
Copy Markdown

Your PR needs a rebase on 089db52 or later. I've tested main, but directory copy still fails on AerynOS:

root@boulder:~/x86_64
# bash ./test.sh

Set my umask to 0022
What's my umask?
0022

whoami?
root

Which cp version are we using?
cp (uutils coreutils) 0.8.0

>> File permissions test:

Create a 0775 | rwxrwxr-x file:
-rwxrwxr-x 1 root root 0 May 12 06:14 file.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'file.0775' -> 'file.0755'
Test assumption:
-rwxr-xr-x 1 root root 0 May 12 06:14 file.0755

>> Directory permissions test:

Create a 0775 | rwxrwxr-x directory:
mode of 'dir.0775' changed from 0755 (rwxr-xr-x) to 0775 (rwxrwxr-x)
drwxrwxr-x 1 root root 0 May 12 06:14 dir.0775

Assumption: cp -Rv respects 0022 umask whether or not the user is root
'dir.0775' -> 'dir.0755/'
Test assumption:
drwxrwxr-x 1 root root 0 May 12 06:14 dir.0755
root@boulder:~/x86_64

@hphilm

hphilm commented May 30, 2026

Copy link
Copy Markdown

Needs a rebase on 0.9.0 for further testing ...

@hphilm

hphilm commented Aug 16, 2026

Copy link
Copy Markdown

Needs a rebase on 0.10.0 for further testing ...

@hphilm

hphilm commented Sep 17, 2026

Copy link
Copy Markdown

Most likely superseded by #14576 and/or #14560

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cp -p doesn't strip setuid/setgid bits when chown fails

4 participants