Skip to content

fix(install): follow symlink components in destination path with -D#11505

Merged
Ecordonnier merged 1 commit into
uutils:mainfrom
abendrothj:fix/install-d-follow-symlinks
May 25, 2026
Merged

fix(install): follow symlink components in destination path with -D#11505
Ecordonnier merged 1 commit into
uutils:mainfrom
abendrothj:fix/install-d-follow-symlinks

Conversation

@abendrothj
Copy link
Copy Markdown
Contributor

@abendrothj abendrothj commented Mar 26, 2026

Summary

Fixes #11469

install -D was replacing pre-existing symlinks in the destination path with real directories instead of following them. This broke any workflow where part of the install prefix is a symlink; including BOSH deployments, Homebrew, Nix, stow, and any make install targeting a symlinked prefix.

Reproduction (from the issue):

mkdir -p /tmp/target
ln -s /tmp/target /tmp/link
echo hello > /tmp/file.txt
install -D -m 644 /tmp/file.txt /tmp/link/subdir/file.txt
# GNU coreutils 8.32: /tmp/link stays a symlink, file lands in /tmp/target/subdir/file.txt
# uutils 0.7.0:       /tmp/link is replaced with a real directory — wrong

Root cause

PR #10140 introduced create_dir_all_safe() in safe_traversal.rs to prevent TOCTOU symlink race conditions. The fix was correct in intent but too aggressive: open_or_create_subdir() unconditionally unlinked and recreated any symlink it encountered, including pre-existing legitimate ones.

Changes

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

  • open_or_create_subdir: when stat_at returns S_IFLNK, call open_subdir(Follow) instead of unlink_at + mkdir_at. The O_DIRECTORY flag already in open_subdir means dangling or non-directory symlinks still return an error cleanly.
  • find_existing_ancestor: switch from fs::symlink_metadata to fs::metadata so that a symlink-to-directory is recognised as an existing ancestor rather than a component to recreate (this was already the stated intent in the function's doc comment).

src/uu/install/src/install.rs

  • Align the dir_exists check and the DirFd::open call to also follow symlinks, consistent with the above.

tests/by-util/test_install.rs

TOCTOU / security note

The true TOCTOU race (a symlink injected during the operation into a not-yet-existing path component) is still blocked: mkdirat fails with EEXIST if an attacker creates a symlink between stat_at returning ENOENT and our mkdir_at. Newly-created directories are still opened with O_NOFOLLOW.

What changes is that pre-existing symlinks are now followed — which is exactly what GNU coreutils 8.32 does. The previous behavior was stricter than GNU in this regard.

@abendrothj abendrothj force-pushed the fix/install-d-follow-symlinks branch from 9c7f91a to 4fc3ac8 Compare March 26, 2026 11:13
@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout-group. tests/timeout/timeout-group is passing on 'main'. Maybe you have to rebase?
Note: The gnu test tests/seq/seq-epipe is now being skipped but was previously passing.
Congrats! The gnu test tests/cp/link-heap is now passing!
Note: The gnu test tests/misc/write-errors was skipped on 'main' but is now failing.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 26, 2026

Merging this PR will not alter performance

✅ 319 untouched benchmarks
⏩ 46 skipped benchmarks1


Comparing abendrothj:fix/install-d-follow-symlinks (dbe8864) with main (19c7f64)

Open in CodSpeed

Footnotes

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

@abendrothj abendrothj force-pushed the fix/install-d-follow-symlinks branch 2 times, most recently from ebc0ba6 to cd070c4 Compare March 26, 2026 20:34
@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/misc/io-errors. tests/misc/io-errors is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/cut/bounded-memory (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.

@abendrothj abendrothj force-pushed the fix/install-d-follow-symlinks branch from cd070c4 to ed62194 Compare March 28, 2026 12:03
@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/date/resolution (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.

@zhw2101024
Copy link
Copy Markdown
Contributor

The test fails in the mkfifo test, anyway hope this PR can be merged, and the related issue not left to the next released uutils.

@abendrothj
Copy link
Copy Markdown
Contributor Author

The test fails in the mkfifo test, anyway hope this PR can be merged, and the related issue not left to the next released uutils.

agreed. seems like flakiness, since mkfifo is unrelated.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 22, 2026

GNU testsuite comparison:

Skip an intermittent issue tests/cut/bounded-memory (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Note: The gnu test tests/tail/pipe-f is now being skipped but was previously passing.

install -D was replacing pre-existing symlinks in the destination path
with real directories instead of following them, breaking any workflow
where part of the install prefix is a symlink (BOSH, Homebrew, Nix,
stow, `make install` with a symlinked prefix).
@Ecordonnier Ecordonnier force-pushed the fix/install-d-follow-symlinks branch from 93383c7 to dbe8864 Compare May 24, 2026 20:03
@Ecordonnier Ecordonnier self-requested a review May 24, 2026 20:05
/// Find the deepest existing real directory ancestor for a path.
///
/// Returns the existing ancestor path and a list of components that need to be created.
/// Uses `symlink_metadata` to detect symlinks - symlinks are NOT followed and are
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This comment is wrong after the change. Should be changed to something like:

/// Uses `metadata` (follows symlinks) so that symlinks to directories are treated
/// as existing ancestors rather than components to create.

/// Open or create a subdirectory using fd-based operations only.
///
/// This is a helper function for `create_dir_all_safe` that handles a single
/// path component. If a symlink exists where a directory should be, it is
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this comment is wrong after the change and needs to be updated.

/// This function prevents TOCTOU race conditions by:
/// 1. Finding the deepest existing ancestor directory (path-based, but safe since it exists)
/// 2. Opening that ancestor with a file descriptor
/// 3. Creating all new directories using fd-based operations (mkdirat, openat with O_NOFOLLOW)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

after the change: existing symlinks are followed (with O_DIRECTORY as a safety net), while only newly created path components are protected with O_NOFOLLOW.

fn test_install_d_symlink_race_condition() {
// Test for symlink race condition fix (issue #10013)
// Verifies that pre-existing symlinks in path are handled safely
// Test that pre-existing symlinks in the path are followed (GNU coreutils behavior).
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the test name doesn't match what is being tested. it's not related to race-conditions any more

@Ecordonnier Ecordonnier merged commit 21fda8c into uutils:main May 25, 2026
172 of 173 checks passed
@Ecordonnier
Copy link
Copy Markdown
Collaborator

Thanks. I've merged the branch and will add the cosmetic changes in a follow-up PR. This should be merged ASAP since it is a critical fix.

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: install -D replaces symlink path components with real directories breaks our build system

3 participants