Skip to content

install: create leading directories in write-only directories - #14779

Open
abendrothj wants to merge 2 commits into
uutils:mainfrom
abendrothj:fix/install-write-only-leading-dirs
Open

abendrothj wants to merge 2 commits into
uutils:mainfrom
abendrothj:fix/install-write-only-leading-dirs

Conversation

@abendrothj

@abendrothj abendrothj commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

install -D could not create leading directories inside a directory that is writable and searchable but not readable, where GNU 9.12 can:

$ mkdir wx && chmod 300 wx && echo hi > f
$ install -D f wx/sub/f
install: cannot create directory 'wx/sub': Permission denied

create_dir_all_safe anchored its walk with an O_RDONLY | O_DIRECTORY open of the deepest existing ancestor, which needs read permission; mkdirat only needs write and execute. It now uses DirFd::open_anchor, which falls back to a search-only descriptor (O_PATH on Linux/Android, O_SEARCH on Apple targets, FreeBSD, NetBSD, illumos and Solaris) on EACCES. The NoFollow opens in the descent are unchanged. Platforms with neither flag, including OpenBSD, still fail with EACCES here.

Based on #15120; until it merges, the diff here shows its commit too.

Fixes #14778

Copilot AI lite review requested due to automatic review settings September 21, 2026 09:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

GNU testsuite comparison:

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

Copilot AI review requested due to automatic review settings September 22, 2026 01:04
@abendrothj
abendrothj force-pushed the fix/install-write-only-leading-dirs branch from 8d87373 to e53e63f Compare September 22, 2026 01:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@abendrothj
abendrothj force-pushed the fix/install-write-only-leading-dirs branch from e53e63f to 18a794d Compare September 22, 2026 01:40
Copilot AI review requested due to automatic review settings September 22, 2026 01:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@abendrothj

Copy link
Copy Markdown
Contributor Author

About the red Tests (unix) job: that runner is OpenBSD 7.9, which has neither O_PATH nor O_SEARCH, so there's no way to anchor *at calls on a directory we can't read and this approach can't work there. safe_traversal now exports SEARCH_ONLY_SUPPORTED and the new test skips when it's false.

To be explicit about the gap: install -D into a write-only directory still fails on OpenBSD, exactly as it did before this PR. The fix applies on Linux, macOS, FreeBSD, NetBSD and the Solaris family. I could add a path-based fallback for the rest, but that trades the fd-anchored traversal for plain path resolution and I'd rather not do that silently — tell me if you want it.

@abendrothj
abendrothj force-pushed the fix/install-write-only-leading-dirs branch from 18a794d to 3e557aa Compare October 1, 2026 01:15
Copilot AI lite review requested due to automatic review settings October 1, 2026 01:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Critical cross-platform compilation and test-gating issues remain unresolved.

Review effort: Lite
Findings: 2 High severity

Open (2)

Comment thread src/uucore/src/lib/features/safe_traversal.rs
Comment thread tests/by-util/test_install.rs Outdated
@codspeed

codspeed Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Merging this PR will regress 6 benchmarks

⚡ 11 improved benchmarks
❌ 6 regressed benchmarks
✅ 382 untouched benchmarks
⏩ 54 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation false_consecutive_calls 294.7 ns 350.2 ns -15.86%
❌ Simulation five_38_bit_primes 1.5 s 1.7 s -10.81%
❌ Simulation tsort_complex_dag[50000] 87.6 ms 97.1 ms -9.79%
❌ Simulation tsort_tree_dag[(10, 3)] 36.5 ms 39.4 ms -7.33%
❌ Simulation tsort_wide_dag[100000] 157.6 ms 167 ms -5.65%
❌ Simulation tsort_linear_chain[1000000] 1.9 s 2 s -4.75%
⚡ Simulation three_39_bit_primes 901.6 ms 544.5 ms +65.57%
⚡ Simulation cut_fields_custom_delim 65.9 ms 51.9 ms +27%
⚡ Simulation cut_fields_tab 57.4 ms 45.5 ms +26.24%
⚡ Simulation cut_bytes 18.1 ms 15.5 ms +16.69%
⚡ Simulation join_french_locale 26.7 ms 24 ms +11.1%
⚡ Simulation join_full_match 26.7 ms 24 ms +11.07%
⚡ Simulation cut_characters 25.4 ms 22.9 ms +10.88%
⚡ Simulation join_partial_overlap 22.1 ms 20.4 ms +8.36%
⚡ Simulation join_unicode_locale 2.6 ms 2.4 ms +6.67%
⚡ Simulation join_custom_separator 25.9 ms 24.4 ms +6.08%
⚡ Simulation rm_recursive_tree 14.4 ms 13.7 ms +5.28%

Tip

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


Comparing abendrothj:fix/install-write-only-leading-dirs (0e67229) with main (8edf725)2

Open in CodSpeed

Footnotes

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

  2. No successful run was found on main (8460811) during the generation of this report, so 8edf725 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

Copilot AI lite review requested due to automatic review settings October 1, 2026 06:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

A critical unit test fails on OpenBSD and the Linux fallback documentation needs correction.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Correct inaccurate O_PATH and O_NOFOLLOW explanation

src/​uucore/​src/​lib/​features/​safe_traversal.rs:216

The Linux explanation here is inaccurate: O_PATH does not ignore O_NOFOLLOW; with O_PATH|O_NOFOLLOW it refers to the symlink itself (and combining O_DIRECTORY can reject it), rather than resolving it. The fallback intentionally omits O_NOFOLLOW to preserve Follow semantics, so document that choice directly instead of attributing it to an ignored flag.

This issue also appears on line 230 of the same file.

Comment thread src/uucore/src/lib/features/safe_traversal.rs
Copilot AI lite review requested due to automatic review settings October 1, 2026 07:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

/// directories where GNU succeeds. `O_PATH` (Linux) and `O_SEARCH` (POSIX
/// 2008) both yield a descriptor that anchors `*at` calls without reading.
/// Such a descriptor cannot list directory entries.
#[cfg(any(target_os = "linux", target_os = "android"))]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the target list three times is a lot, could we do the two positive cases and #[cfg(not(any(unix_with_search)))] via a small cfg alias?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 991db22, which is now its own PR (#15120): uucore's build script defines has_o_path and has_o_search, and the constants and unit tests are gated on those. The install integration test still spells out the target list once, because uucore's build-script cfgs don't reach the root test crate; comments on both sides point at each other.

/// Where it cannot, creating an entry inside a write-only directory fails with
/// `EACCES` instead of succeeding the way `mkdir` does (OpenBSD, for example,
/// has neither `O_PATH` nor `O_SEARCH`).
pub const SEARCH_ONLY_SUPPORTED: bool = SEARCH_ONLY.is_some();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a pub const just so a test can skip? could the test be cfg'd on the same targets instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gone. SEARCH_ONLY and open_search_only are private now, and the tests are cfg'd on has_o_path/has_o_search instead (991db22).

.map_err(|_| denied)
}

/// Open a subdirectory relative to this directory

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the real errno is dropped here, we will report EACCES for an ENOENT or ELOOP

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 991db22: the search-only retry returns its own error, so an ENOENT or ELOOP from the retry is reported as such instead of the first open's EACCES. A unit test calls the retry directly for ENOENT, ELOOP and ENOTDIR.

@abendrothj
abendrothj force-pushed the fix/install-write-only-leading-dirs branch from 57c03da to edb49be Compare October 6, 2026 07:15
Copilot AI lite review requested due to automatic review settings October 6, 2026 07:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Open a directory readable, and on EACCES retry with O_PATH or O_SEARCH,
which anchor *at calls without read access. The targets with either flag
are named by the has_o_path and has_o_search cfg aliases. If the retry
fails, its own error is returned.
install -D opened the deepest existing ancestor read-only, so it failed
with EACCES when that directory had write and execute but not read
permission, while GNU install succeeds. Open it with DirFd::open_anchor.
@abendrothj
abendrothj force-pushed the fix/install-write-only-leading-dirs branch from edb49be to 0e67229 Compare October 8, 2026 04:09
Copilot AI lite review requested due to automatic review settings October 8, 2026 04:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

This branch has not been deployed

No deployments
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.

install: -D cannot create leading directories in a write-only directory

3 participants