install: name the directory component that could not be created by abendrothj · Pull Request #14772 · uutils/coreutils

abendrothj · 2026-09-21T06:23:33Z

install named the whole leading-directory path instead of the component that failed, and with -D gave no reason at all:

install -D f dangling/sub/f   cannot create directory 'dangling/sub'
install -d dangling/sub       cannot create directory 'dangling/sub': File exists
GNU, both cases               cannot create directory 'dangling': File exists

create_dir_all_safe now returns the failing prefix along with its error, and the ancestor search walks up past files, symlink loops and unreadable directories instead of giving up with the whole path. A name that exists but can't be descended into reports EEXIST when it doesn't resolve and ENOTDIR when it resolves to a non-directory, which is what GNU prints. When the parent of the failing component can't be searched at all, that parent is named instead - the difference between r--/x, which GNU reports as r--, and r-x/x, which it reports as r-x/x. The message string gained a place for the errno.

install -d keeps creating directories by path: without O_PATH or O_SEARCH the fd walk can't anchor on a write-only directory, where GNU succeeds. There's a regression test for -d in a write-only directory.

Checked on Linux against GNU 9.12: the messages match in 27 of 31 -D, -d and -D -t cases (main: 8), not counting GNU's curly quotes for -d. The four that differ also differ on main: trailing-slash targets (-D f regular/sub/, -D f dangling/sub/), an existing parent without search permission (-D f r--/f), and -D -t regular/sub f, which #14999 covers.

Closes #14995

Copilot

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

abendrothj · 2026-09-21T06:28:10Z

Conflicts with #12715 in the two -d hunks. Whichever lands second resolves to with_umask(0, || create_dir_all_safe(&path_to_create, DEFAULT_MODE)) with show!(InstallError::CreateDirFailed(e.path, e.error)).

github-actions · 2026-09-21T07:37:37Z

GNU testsuite comparison:

GNU test failed: tests/id/setgid. tests/id/setgid is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/date/resolution (passes in this run but fails in the 'main' branch)

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 3

Open (4)

These newly added unit tests directly reference Unix-only items (libc::ENOTDIR / libc::EEXIST,… · New
Using libc::access() here can mispredict what the process can actually do because access()… · New
create_dir_all_safe is pub and its return type changed from io::Result<DirFd> to… · New
This From<CreateDirError> for io::Error conversion silently discards the path context that the… · New

codspeed · 2026-10-01T02:03:05Z

Merging this PR will degrade performance by 10.38%

⚠️

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

sort_mixed_utf8_locale

sort_reverse_utf8_locale

sort_unique_utf8_locale

cksum_multiple_files

sort_very_long_lines_utf8_locale

cksum_default

factor_multiple_u64s[2]

cksum_crc32b

cksum_raw_output

cksum_crc

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

⚠️

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

❌ 3 regressed benchmarks
✅ 398 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	382.5 ms	-21.51%
❌	Simulation	`factor_thirteen_39_bit_primes`	8.9 s	9.3 s	-4.84%
❌	Simulation	`factor_five_38_bit_primes`	1.6 s	1.7 s	-3.63%

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-create-dir-error-component (1906102) 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 (9c8b347) 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. ↩

Copilot

Copilot review overview

🔵 Needs a closer look

install -d still reports the full path for nested failures, and permission-sensitive naming cases lack regression coverage.

Review effort: Lite
Findings: 1 · 2

Open (3)

These newly added unit tests directly reference Unix-only items (libc::ENOTDIR / libc::EEXIST,…
create_dir_all_safe is pub and its return type changed from io::Result<DirFd> to…
Using libc::access() here can mispredict what the process can actually do because access()…

Resolved since last review (1)

This From<CreateDirError> for io::Error conversion silently discards the path context that the…

Previously missed (1)

In code that hasn't changed since last review

Report the failing path component for nested mkdir errors

src/uu/install/src/install.rs:517

install -d still passes the entire path_to_create as the error path, so a nested failure such as install -d dangling/sub (or regular/sub where regular is a file) will continue to report dangling/sub/regular/sub instead of the failing component. Keep the path-based mkdir behavior for write-only parents, but track the component that failed (or add a path-based helper that returns it) before constructing CreateDirFailed.

Devel08 · 2026-10-01T03:50:11Z

+            // check runs on the trimmed path so that a trailing slash, which
+            // makes `exists()` fail with ENOTDIR, is handled the same way.
+            if b.target_dir.is_some() && to_create.exists() && !to_create.is_dir() {
+                return Err(InstallError::NotADirectory(to_create_original.to_path_buf()).into());


Wouldn't that cause a TOCTOU?

It only decides which error gets printed. If the target is an existing non-directory, install stops with "Not a directory"; otherwise nothing is done with the result. If the path changes after the check, the directories are still created through create_dir_all_safe, which works through directory descriptors (mkdirat, openat with O_DIRECTORY) and refuses a file or dangling symlink at that name by itself. So a race can only change which message you get, to "cannot create directory". The check isn't new either: main already does the same exists()/is_dir() check on the -t target; this moves it after the trailing-slash trim.

Copilot

Copilot review overview

🟡 Changes recommended

A critical -t target-validation regression and a moderate safe-traversal issue remain unresolved.

Review effort: Lite
Findings: 2 · 2

Open (4)

Validate -t target directories outside the -D block · New
These newly added unit tests directly reference Unix-only items (libc::ENOTDIR / libc::EEXIST,…
create_dir_all_safe is pub and its return type changed from io::Result<DirFd> to…
Using libc::access() here can mispredict what the process can actually do because access()…

Previously missed (1)

In code that hasn't changed since last review

Add integration coverage for unsearchable parent error paths

src/uucore/src/lib/features/safe_traversal.rs:630

The new blame branch is what implements the documented r--/x versus r-x/x distinction, but the added tests only cover a regular file, a dangling symlink, and the successful -d write-only case. Add an integration test for an unsearchable parent (and ideally the loop/read-only error cases described by the PR) so this error-path naming behavior cannot regress.

Copilot

Copilot review overview

🟡 Changes recommended

Fix -t target validation and guard chmod-dependent tests for WASI.

Review effort: Lite
Findings: 3 · 2

Open (5)

Skip permission test on WASI · New
Validate -t target directories outside the -D block
These newly added unit tests directly reference Unix-only items (libc::ENOTDIR / libc::EEXIST,…
create_dir_all_safe is pub and its return type changed from io::Result<DirFd> to…
Using libc::access() here can mispredict what the process can actually do because access()…

Copilot

Copilot review overview

🔵 Needs a closer look

An outstanding documentation nit and mixed readiness signals warrant human review.

Review effort: Lite
Findings: 1

Open (1)

Skip permission test on WASI

Resolved since last review (4)

Validate -t target directories outside the -D block
These newly added unit tests directly reference Unix-only items (libc::ENOTDIR / libc::EEXIST,…
create_dir_all_safe is pub and its return type changed from io::Result<DirFd> to…
Using libc::access() here can mispredict what the process can actually do because access()…

Previously missed (1)

In code that hasn't changed since last review

Remove GNU test path reference from PR description

tests/by-util/test_install.rs:2891

The PR description references the GNU test path tests/install/basic-1. Please remove that path reference and summarize the observed behavior instead; this repository must not include references to GNU source/test paths.

abendrothj · 2026-10-01T09:48:29Z

Rechecked against GNU 9.12 built from the release tarball (9.10 behaves the same): this branch matches GNU in 19 of 24 -D cases. The five that differ are trailing-slash targets (-D f regular/sub/, -D f dangling/sub/), an unreadable existing parent (-D f r--/f), and two -D -t cases that #14999 covers. None of them match on main either. I've corrected the description.

abendrothj · 2026-10-06T08:05:27Z

Correction: with the current #12715 this is one hunk in directory(), and with_umask must not be nested there, since install already runs under it. Keep #12715's DirBuilder with DEFAULT_MODE and take this PR's error: show!(InstallError::CreateDirFailed(failed_create_dir_prefix(&path_to_create), e)), keeping the #[cfg(unix)]/#[cfg(not(unix))] split for failed.

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-12T11:26:54Z

Rebased onto #14779 and squashed. The anchor is now opened with open_anchor, so -D into write-only directories keeps working with the new error reporting.

install named the whole leading-directory path when creating it failed, and -D gave no reason. Name the first component that failed, with its errno, as GNU does, for both -D and -d.

Copilot

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

Copilot AI lite review requested due to automatic review settings September 21, 2026 06:23

Copilot AI reviewed Sep 21, 2026

Copilot AI review requested due to automatic review settings September 21, 2026 07:44

Copilot AI reviewed Sep 21, 2026

Copilot AI review requested due to automatic review settings September 21, 2026 08:32

Copilot AI reviewed Sep 21, 2026

Copilot AI lite review requested due to automatic review settings October 1, 2026 01:16

abendrothj force-pushed the fix/install-create-dir-error-component branch from e0c8f96 to f088cbd Compare October 1, 2026 01:16

Copilot started reviewing on behalf of abendrothj October 1, 2026 01:34 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread src/uucore/src/lib/features/safe_traversal.rs

Comment thread src/uucore/src/lib/features/safe_traversal.rs

Comment thread src/uucore/src/lib/features/safe_traversal.rs

Comment thread src/uucore/src/lib/features/safe_traversal.rs Outdated

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

Copilot started reviewing on behalf of abendrothj October 1, 2026 02:14 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Devel08 reviewed Oct 1, 2026

View reviewed changes

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

Copilot started reviewing on behalf of abendrothj October 1, 2026 04:04 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread src/uu/install/src/install.rs

Copilot AI lite review requested due to automatic review settings October 1, 2026 04:39

Copilot started reviewing on behalf of abendrothj October 1, 2026 04:40 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread tests/by-util/test_install.rs

abendrothj mentioned this pull request Oct 1, 2026

CI: GNU test tests/id/setgid fails intermittently on unrelated PRs #14998

Closed

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

Copilot started reviewing on behalf of abendrothj October 1, 2026 06:52 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Copilot AI lite review requested due to automatic review settings October 7, 2026 11:28

abendrothj force-pushed the fix/install-create-dir-error-component branch from 5867bbe to e24d398 Compare October 7, 2026 11:28

Copilot AI reviewed Oct 7, 2026

Copilot AI lite review requested due to automatic review settings October 12, 2026 11:26

abendrothj force-pushed the fix/install-create-dir-error-component branch from e24d398 to 2c6213f Compare October 12, 2026 11:26

Copilot AI reviewed Oct 12, 2026

abendrothj mentioned this pull request Oct 12, 2026

install: build on Redox #15276

Merged

install: name the directory component that could not be created

1906102

install named the whole leading-directory path when creating it failed, and -D gave no reason. Name the first component that failed, with its errno, as GNU does, for both -D and -d.

Copilot AI lite review requested due to automatic review settings October 13, 2026 10:16

abendrothj force-pushed the fix/install-create-dir-error-component branch from 2c6213f to 1906102 Compare October 13, 2026 10:16

Copilot AI reviewed Oct 13, 2026

Conversation

abendrothj commented Sep 21, 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!

abendrothj commented Sep 21, 2026

Uh oh!

github-actions Bot commented Sep 21, 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!

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

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Merging this PR will degrade performance by 10.38%

Performance Changes

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Devel08 Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

abendrothj Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

abendrothj commented Oct 1, 2026

Uh oh!

abendrothj commented Oct 6, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

abendrothj commented Sep 21, 2026 •

edited

Loading

github-actions Bot commented Sep 21, 2026 •

edited

Loading

codspeed Bot commented Oct 1, 2026 •

edited

Loading