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 improve performance by 8.09%

⚠️

This run is associated to a commit that was force-pushed

It won't be associated with its branch history. Be careful when comparing its performance to other runs.

⚠️

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

⚡ 7 improved benchmarks
✅ 177 untouched benchmarks
⏩ 269 skipped benchmarks¹

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	`three_39_bit_primes`	578.4 ms	506.6 ms	+14.18%
⚡	`tsort_complex_dag[50000]`	97.2 ms	87.6 ms	+10.86%
⚡	`five_38_bit_primes`	1.9 s	1.7 s	+9.31%
⚡	`tsort_tree_dag[(10, 3)]`	39.4 ms	36.5 ms	+7.92%
⚡	`tsort_wide_dag[100000]`	167 ms	157.6 ms	+5.98%
⚡	`tsort_linear_chain[1000000]`	2 s	1.9 s	+4.99%
⚡	`thirteen_39_bit_primes`	9.1 s	8.7 s	+3.72%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing abendrothj:chmod-chown-operand-by-fd (1066329) with main (4e2be1c)}

269 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 · 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.

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::dev_ino_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. A directory that cannot be read before its mode change, and -H with --no-dereference, still go by path.

Rename dev_ino_is_root_dir to metadata_is_root_dir and pass it the metadata instead of its (dev, ino). chown has a std::fs::Metadata and chmod the safe_traversal::Metadata of a descriptor; both implement MetadataExt, which is what it takes. Keep only its own unit test: the chmod and chown ones checked the same thing through thin wrappers.

When opening a recursive operand failed with anything but EACCES, chmod reported the bare errno and left the operand's mode unchanged. Fall back to changing it by path whatever the error, as the path-based code did, and let the descent's open report why the directory can't be read. The descriptor that the mode was changed through is also closed before the directory is opened again to descend: kept open, every level reached through a symlink under -L took two descriptors, and "chmod -R -L" over a chain of ten failed with "Too many open files" under a limit of 20 that the path-based code and GNU fit in. Only its identity is kept, and the re-opened directory is checked for "/" only when there is none to compare against: being that same directory already rules "/" out. Gate the Replaced error with cfg rather than allowing it dead.

Run "chmod -R" while another thread keeps re-pointing the operand, a symlink, between two directories of the fixture: the descent must only enter the directory whose mode was changed, else it fails with "replaced". Without that check, or changing the mode by path, between one run in three and one in nine descends into the other directory. Also cover a symlink operand to "/" under --preserve-root with -H, -L and -P plus a trailing slash, and -h, whose operand is changed by path.

Falling back to a by-path change whatever the error let a rename force the fallback: point the name at a file or a symlink and the open fails, the mode of whatever is there then gets changed by path, and the descent that follows is not checked against the directory changed. On macOS the race alone can fail the open. Fall back only for EACCES, EMFILE and ENFILE, where GNU also changes the mode by path, and report other errors naming the operand. The descent's open names it too instead of printing the bare errno. The race test now also points the operand at a file.

On macOS a lookup through the symlink being replaced can fail too, with EINVAL from chmod(2) or a failed stat, so the error's cause is not checked. The modes are checked first, so every round tests that chmod only descends into the directory it changed.

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 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 added 5 commits October 7, 2026 01:07

abendrothj added 2 commits October 7, 2026 01:07

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

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 improve performance by 8.09%

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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