chown, chgrp: hold the --from file without O_PATH by abendrothj · Pull Request #15129 · uutils/coreutils

abendrothj · 2026-10-06T07:28:24Z

Outside Linux, --from was still decided on a stat and the change made by name, so a rename in between changed a file that never matched.

The file is now held on every platform with the descriptor walk: O_PATH with fchownat(AT_EMPTY_PATH) on Linux and Android, and a read-only non-blocking open with fchown elsewhere. On macOS, -h holds a symlink itself with O_SYMLINK (second commit). A file that can't be held that way (a socket, a device node, a symlink under -h elsewhere, or a file the caller can't read) is still changed by name, as before. Linux behaviour doesn't change. Run on macOS, Linux and Debian GNU/Hurd (on Hurd, the tests that fail also fail on main); FreeBSD and AIX are only cross-checked with clippy.

Based on #15128.

Closes #15027

Copilot

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

codspeed · 2026-10-06T15:46:56Z

Merging this PR will not alter performance

⚠️

117 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_wide_tree[(5000, 500)]

cksum_default

cksum_multiple_files

du_deep_tree[(100, 3)]

du_all_wide_tree[(5000, 500)]

dd_copy_partial

cksum_raw_output

cksum_crc

cksum_crc32b

wc_bytes_synthetic[1]

…and 107 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]

✅ 401 untouched benchmarks
⏩ 54 skipped benchmarks¹

_{Comparing abendrothj:chown-from-hold-without-opath (ccf8100) with main (6292c03)}

54 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. ↩

sylvestre · 2026-10-06T16:28:36Z

 use libc::{gid_t, uid_t};
 use options::traverse;
-#[cfg(target_os = "linux")]
+#[cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox")))]


this cfg is now repeated 23 times, could we use a cfg alias or move this code into its own module?

most of them came from #15128, so the alias is there: has_safe_traversal, from uucore's build script with cfg_aliases as #15120 does, and this PR uses it too. perms.rs doesn't spell out the targets anymore.

sylvestre · 2026-10-06T16:28:43Z

+/// Whether the file `meta` describes can be held open by [`PinnedFile`].
+#[cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox")))]
+// mode_t is u16 on macOS and u32 on Linux
+#[allow(clippy::unnecessary_cast)]


could you please use meta.file_type() with FileTypeExt (is_socket, is_char_device...)? then you don't need the cast or the allow

the -R entries come with safe_traversal's Metadata, which has no std FileType, so it uses rustix's FileType::from_raw_mode(meta.mode() as _) for both kinds. the allow is gone.

sylvestre · 2026-10-06T16:28:54Z

+        open: impl FnOnce() -> IOResult<PinnedFile>,
+    ) -> IOResult<Hold> {
+        if !can_hold(meta, follow) {
+            return if unprivileged {


as root on macOS, chown --from=0 0 /dev/null now fails with "Operation not supported" where it used to work. is that really what we want?
same for -h on a symlink on OpenBSD/NetBSD

no: root now changes such a file by name like everyone else, as on main. that's a socket, a device node, or a symlink under -h where it can't be held. it leaves the rename race open for those files only; the comment on hold says so.

sylvestre · 2026-10-06T16:28:58Z

+    /// passed `--from`, through [`Self::hold`] if `--from` was given. Returns
+    /// whether it was changed.
+    #[cfg(not(any(target_os = "aix", target_os = "hurd", target_os = "redox")))]
+    fn chown_entry(


the condition in this function is hard to follow, could it be simplified?

split it: changes_link_itself names the -L -h symlink case, and the function now picks how to change the file once, then changes it.

sylvestre · 2026-10-06T16:29:06Z

+    // FIFO from waiting for a writer.
+    #[cfg(not(any(target_os = "linux", target_os = "android", target_os = "freebsd")))]
+    let flags = OFlag::O_RDONLY
+        | OFlag::O_NONBLOCK


please add a test in tests/by-util/test_chown.rs with --from on a fifo and on a 000 file

added test_chown_from_fifo_and_unreadable_file, as operands and under -R. on macOS the 000 file goes through the by-name fallback, and the test fails without it; on Linux O_PATH holds both. a missing O_NONBLOCK would make it hang rather than fail.

sylvestre · 2026-10-06T16:29:13Z

+    /// FreeBSD (14.5 at least) fails a change through an O_PATH descriptor
+    /// with EBADF whatever the reason. Report what a change by name would.
+    #[cfg(target_os = "freebsd")]
+    fn freebsd_chown_errno(&self, e: nix::errno::Errno) -> nix::errno::Errno {


guessing EPERM/EROFS from fstatfs seems fragile.
why not use the read-only open on FreeBSD too?

done, FreeBSD takes the read-only open now and the errno guessing is gone. O_PATH is only used on Linux and Android, named by a has_o_path alias. FreeBSD is only cross-checked with clippy.

sylvestre · 2026-10-06T16:29:25Z

a bit too large for my taste

Copilot

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

abendrothj · 2026-10-07T02:48:29Z

smaller now: +219/-77 (it was +258/-66), about 85 of it tests. the alias moved to #15128, and the macOS O_SYMLINK part is its own commit.

Copilot

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

--from was checked on a stat of the name and the change was then made by name, so a rename landing in between changed a file that never had the required owner. On Linux, hold the file open with O_PATH, check that it is the one that was stat'd and still passes --from, and change it through that descriptor with fchownat(AT_EMPTY_PATH). This covers operands and -R entries. A file found replaced is left alone; as with GNU, that fails without a message, and -v reports the change that was not made.

On Linux, a directory operand was changed through a descriptor opened for reading, and when that open failed it was skipped without a message. Change it by name then, or through an O_PATH descriptor with --from, as for any other file. Under -R it is still left alone and the failure to read it reported, as with GNU.

Under -R -H, a symlink met in the tree was stat'd itself, while the change went to the file it points to. --from was judged on, and -v reported, the link's owner. Use the target's, as GNU does.

With -R on Linux, a change below the operand that failed was reported without the "failed to change" line that -v adds for the operand and on other platforms.

With -h, `-R -H` and `-R -L` did not descend into a symlinked directory operand, and `-R -L -h` changed the files that symlinks in the tree point to rather than the links. As GNU does, follow such a symlink to descend and to judge --from, and change the link itself.

The descriptor-based -R walk ran only on Linux; other platforms used walkdir, which stops at a symlink loop under -L with an error where GNU just skips the directory already being visited. Use the descriptor walk wherever safe_traversal is available, keeping walkdir for Redox. Outside Linux, --from entries are still changed by name. safe_traversal's platforms are named once, by a has_safe_traversal cfg alias in uucore's build script.

With --from, a file was held open and changed through that descriptor only on Linux; elsewhere it was changed by name, so a rename landing after the check changed a file that never passed --from. Hold it on every platform the descriptor walk runs on: with O_PATH on Linux and Android, and with a read-only non-blocking open elsewhere. A file that cannot be held that way, such as a socket, a device node, a symlink under -h or a file the caller cannot read, is still changed by name, which leaves the race open for it as before.

Under -h with --from, a symlink is changed itself. Without O_PATH it could not be held, so it was changed by name. Apple systems can open the symlink itself with O_SYMLINK; hold it that way there, as on Linux.

Copilot

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

abendrothj · 2026-10-12T11:39:02Z

Rebased. main now has has_o_path from #15120, so this no longer adds it.

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

Copilot AI reviewed Oct 6, 2026

This was referenced Oct 6, 2026

chown, chgrp: decide --from on the file that is changed #15029

Open

chown, chgrp: use the descriptor walk for -R on all safe_traversal platforms #15128

Draft

sylvestre reviewed Oct 6, 2026

View reviewed changes

abendrothj force-pushed the chown-from-hold-without-opath branch from abc5740 to 0c67785 Compare October 7, 2026 02:47

Copilot AI balanced review requested due to automatic review settings October 7, 2026 02:47

Copilot AI reviewed Oct 7, 2026

abendrothj force-pushed the chown-from-hold-without-opath branch from 0c67785 to 24c9c2f Compare October 7, 2026 08:02

Copilot AI balanced review requested due to automatic review settings October 7, 2026 08:02

Copilot AI reviewed Oct 7, 2026

abendrothj added 8 commits October 12, 2026 04:28

chown, chgrp: judge --from on a symlink's target under -R -H

8934ad8

Under -R -H, a symlink met in the tree was stat'd itself, while the change went to the file it points to. --from was judged on, and -v reported, the link's owner. Use the target's, as GNU does.

chown, chgrp: report failed -R changes with -v as for operands

a37ca9a

With -R on Linux, a change below the operand that failed was reported without the "failed to change" line that -v adds for the operand and on other platforms.

chown, chgrp: hold a symlink itself on Apple systems

ccf8100

Under -h with --from, a symlink is changed itself. Without O_PATH it could not be held, so it was changed by name. Apple systems can open the symlink itself with O_SYMLINK; hold it that way there, as on Linux.

abendrothj force-pushed the chown-from-hold-without-opath branch from 24c9c2f to ccf8100 Compare October 12, 2026 11:38

Copilot AI balanced review requested due to automatic review settings October 12, 2026 11:38

Copilot AI reviewed Oct 12, 2026

Conversation

abendrothj commented Oct 6, 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 Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

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!

sylvestre commented Oct 6, 2026

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Oct 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abendrothj commented Oct 6, 2026 •

edited

Loading

codspeed Bot commented Oct 6, 2026 •

edited

Loading