chmod, chown: check --preserve-root on the directory actually descended into by abendrothj · Pull Request #14972 · uutils/coreutils

abendrothj · 2026-09-30T02:28:17Z

The recursive operand of chmod, chown and chgrp was checked against --preserve-root with its own path lookup, separate from the one the change and the descent used. A rename landing in between could send a -H/-L recursion into "/" after the check had passed.

chown/chgrp now also decide on the stat that the operand's descriptor is already verified against. chmod opens the operand, checks that descriptor, and changes its mode through it; it still reopens the directory to descend, like GNU, and only continues if it is the same directory. Under -P this also stops the operand's own mode change from following a symlink swapped in at the directory's name. If the operand can't be opened because it isn't readable yet or no descriptors are left, chmod changes its mode by name, as GNU does; other errors are reported. Run on macOS, Linux and Debian GNU/Hurd (on Hurd, the tests that fail also fail on main); AIX is only cross-checked with clippy (AIX with libc 0.2.189, see #15083).

Closes #14990

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

codspeed · 2026-09-30T03:05:31Z

Merging this PR will degrade performance by 29.38%

⚠️

122 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

du_all_wide_tree[(5000, 500)]

sort_reverse_utf8_locale

sort_unique_utf8_locale

sort_mixed_utf8_locale

sort_very_long_lines_utf8_locale

cksum_default

cksum_multiple_files

du_wide_tree[(5000, 500)]

df_deep_directory

dd_copy_partial

…and 112 more

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

sort_dictionary_order[500000]

⚠️

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

❌ 2 regressed benchmarks
✅ 399 untouched benchmarks
⏩ 70 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`factor_three_39_bit_primes`	300.2 ms	582.9 ms	-48.49%
❌	Simulation	`factor_thirteen_39_bit_primes`	8.9 s	9.2 s	-3.19%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing abendrothj:chmod-chown-operand-by-fd (73df228) with main (98f90f2)²}

70 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. ↩
No successful run was found on main (f406891) during the generation of this report, so 98f90f2 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

github-actions · 2026-09-30T03:14:41Z

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)

abendrothj · 2026-10-01T00:53:35Z

Opened #14990.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-01T02:10:56Z

The SELinux GNU job failed while booting its VM, before any test ran; unrelated to this change.

sylvestre · 2026-10-05T21:19:27Z

    #[error("{}", translate!("chmod-error-changing-permissions", "file" => _0.quote(), "err" => strip_errno(_1)))]
    ChangingPermissions(PathBuf, std::io::Error),
+    #[error("{}", translate!("perms-cannot-access-replaced", "file" => _0.quote()))]
+    #[cfg_attr(


could you please gate the variant with cfg instead of allow(dead_code)?

Done in e1382d6: the variant is under cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox"))), the same gate as its users, and the allow is gone.

sylvestre · 2026-10-05T21:19:49Z

-                    r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r);
+                Ok(dir_fd) => Some(dir_fd),
+                Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => None,
+                Err(err) => return Err(err.into()),


before, chmod_file was still attempted when the open failed. now we bail out without changing the mode, is that intended?

No, that wasn't intended. Fixed in e1382d6: if the first open or fstat fails for any reason, chmod falls back to the pathname chmod as before. When the open works, the descriptor is used for the root check and the mode change, and it's dropped before descending.

correction to my answer above: falling back on any error was too broad. a rename that makes the open fail (a file swapped in, say, or on macOS apparently the rename alone, which is what CI hit) got whatever was there changed by path, and the descent after it wasn't checked.
now only EACCES/EMFILE/ENFILE fall back, as in GNU 9.12; other errors are reported with the name.

sylvestre · 2026-10-05T21:19:53Z

+                    .as_ref()
+                    .is_some_and(|pinned| !Self::same_dir(pinned, &dir_fd))
+                {
+                    return r.and(Err(ChmodError::Replaced(file_path.into()).into()));


please add a test in tests/by-util/test_chmod.rs, this new path isn't covered at all

Added in 0e38dba: --preserve-root through a symlink operand with -H/-L and with -h, an unreadable operand, a low fd limit, and a test that keeps swapping the operand for a symlink to / across 200 runs. The swap test is the one that reaches the second open; it fails with the merge-base sources and passes now. The fd-limit test fails on ab7438f and passes now. The symlink tests are stopped by the existing earlier check, so they're there for the -h combinations, not the race.

small correction to my list: the race test swaps the operand between two directories of the fixture, not to /, so a failure stays inside it. it now also swaps in a file. without the fix it fails on the bare error message; the wrong descent itself it only catches by chance (once in 2000 runs on Linux, outside the suite). no test reaches the EMFILE/ENFILE fallback or the macOS case.

macos ci failed on the race test's error check, not on the descent: macOS 26 returns EINVAL from stat/chmod/open on op while the racer renames over it (a small C loop on the CI image got it ~25k times in 2M lookups, never on macOS 27). the test now accepts any one error naming op, and checks the modes first.
on the macOS CI image it passes 120 runs; with the chmod from before the fix and only the mode check, 32 of 180 runs descend into the wrong directory. so on macOS it now catches the wrong descent itself, not just the error message. on Linux I haven't re-measured that.

sylvestre · 2026-10-05T21:19:56Z

+                {
+                    return r.and(Err(ChmodError::Replaced(file_path.into()).into()));
+                }
+                if self.preserve_root && Self::is_root_fd(&dir_fd) {


when pinned is Some, same_dir already proved this isn't /, so do we need this second check?

No. In e1382d6 the identity comparison handles the Some case, and the root check only runs on the fallback path.

sylvestre · 2026-10-05T21:20:03Z

+/// answer describes the file the caller is about to act on even if the path
+/// has been re-pointed since.
+#[cfg(unix)]
+pub fn dev_ino_is_root_dir(dev: u64, ino: u64) -> bool {


why not just take a &Metadata? both callers have one

The two callers have different types: chown has a std::fs::Metadata, but chmod gets safe_traversal's metadata from fstat on the descriptor. So in b8be553 it takes &impl MetadataExt, which both implement, and neither caller has to stat the path again.

sylvestre · 2026-10-05T21:20:10Z

+    /// descent is pinned to, whatever the path points at by the time it is asked.
+    #[cfg(unix)]
+    #[test]
+    fn test_meta_is_root_ignores_the_path() {


this is almost the same test as test_dev_ino_is_root_dir and test_is_root_fd, could we keep just one?

Kept just test_metadata_is_root_dir in fs.rs in b8be553; the perms/chmod copies are gone, and is_root_fd went with them in e1382d6.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-07T08:12:13Z

rebased on #15030: the gates here are Redox-only now, like the rest of chmod. with the old ones, chmod wouldn't have built on Hurd/AIX once both were merged (main + this failed Hurd clippy). cross-checked with clippy for Hurd and AIX, not run there. back out of draft once ci is green.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

xtqqczze · 2026-10-07T17:08:35Z

 // file that was distributed with this source code.

-// spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's
+// spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's fchmod EMFILE ENFILE


consider putting EMFILE and ENFILE in .vscode/cspell.dictionaries/workspace.wordlist.txt in the vars/errno section.

Done, moved both to the wordlist.

sylvestre · 2026-10-07T18:16:06Z

+            return self.chmod_file(file_path);
+        }
+
+        // Change the mode through a descriptor, checked not to be "/", rather than by


nobody will read a 9 lines comment :) please make it 2-3 lines

Done, down to 3 lines.

sylvestre · 2026-10-07T18:16:11Z

-            match DirFd::open(file_path, should_follow_symlink.into()) {
-                Ok(dir_fd) => {
-                    r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r);
+        if !((!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir()) {


the double negation is hard to read, could you please name the condition (e.g. let descend = ...) and test !descend?

sylvestre · 2026-10-07T18:16:20Z

+        }
+    }
+
+    // The mode change can also leave it unreadable: the operand keeps its new


this is a different scenario, could you please move it to its own test?

Done, it's now test_chmod_recursive_operand_made_unreadable.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The recursive operand's --preserve-root check looked the path up again, separately from the stat that the operand's descriptor is then verified against. A rename landing between the two let that check see a harmless directory while the chown and the descent went into "/" under -H or -L. Also decide on that stat, through a new uucore::fs::metadata_is_root_dir. The path lookup stays, so a symlink to "/" that the stat did not follow is still reported as before.

The operand of a recursive chmod was checked against --preserve-root by path, changed by path and then opened by path to descend, so a rename landing between those steps could send the mode change and the descent into "/" under -H, or through a symlink swapped in under -P. Open the operand first, check that descriptor is not "/", and change the mode through it. The directory is then opened again to descend, as GNU does, so the new mode still decides whether it can be read, and the descent goes ahead only if that is the same directory. If the operand can't be opened because it isn't readable yet or no descriptors are left, or under -h with -H or -L, the mode is changed by path as before; other open errors are reported.

Copilot AI balanced review requested due to automatic review settings September 30, 2026 02:28

Copilot AI reviewed Sep 30, 2026

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copilot AI balanced review requested due to automatic review settings October 1, 2026 01:17

abendrothj force-pushed the chmod-chown-operand-by-fd branch from b5edd8c to ab7438f Compare October 1, 2026 01:17

Copilot AI reviewed Oct 1, 2026

sylvestre reviewed Oct 5, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 07:15

Copilot AI reviewed Oct 6, 2026

abendrothj force-pushed the chmod-chown-operand-by-fd branch from 0e38dba to a400b6f Compare October 6, 2026 23:17

Copilot AI balanced review requested due to automatic review settings October 6, 2026 23:17

Copilot AI reviewed Oct 6, 2026

Copilot AI balanced review requested due to automatic review settings October 7, 2026 07:38

Copilot AI reviewed Oct 7, 2026

abendrothj marked this pull request as draft October 7, 2026 08:07

abendrothj force-pushed the chmod-chown-operand-by-fd branch from 1066329 to 9989952 Compare October 7, 2026 08:12

abendrothj marked this pull request as ready for review October 7, 2026 14:48

Copilot AI balanced review requested due to automatic review settings October 7, 2026 14:48

Copilot AI reviewed Oct 7, 2026

xtqqczze reviewed Oct 7, 2026

View reviewed changes

sylvestre reviewed Oct 7, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 7, 2026 21:31

Copilot AI reviewed Oct 7, 2026

abendrothj force-pushed the chmod-chown-operand-by-fd branch from eec0a25 to ff34b06 Compare October 7, 2026 21:38

Copilot AI balanced review requested due to automatic review settings October 13, 2026 10:18

abendrothj force-pushed the chmod-chown-operand-by-fd branch from ff34b06 to 2d04aa0 Compare October 13, 2026 10:18

Copilot AI reviewed Oct 13, 2026

abendrothj added 2 commits October 13, 2026 03:34

abendrothj force-pushed the chmod-chown-operand-by-fd branch from 2d04aa0 to 73df228 Compare October 13, 2026 10:36

Conversation

abendrothj commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 29.38%

Performance Changes

Footnotes

Uh oh!

github-actions Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as outdated.

abendrothj commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Oct 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abendrothj Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Oct 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

xtqqczze Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

abendrothj commented Sep 30, 2026 •

edited

Loading

codspeed Bot commented Sep 30, 2026 •

edited

Loading

github-actions Bot commented Sep 30, 2026 •

edited

Loading

abendrothj Oct 6, 2026 •

edited

Loading

xtqqczze Oct 7, 2026 •

edited

Loading