Skip to content

Commit ebc0ba6

Browse files
committed
fix(install): follow symlink components in destination path with -D
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).
1 parent 37d07e4 commit ebc0ba6

3 files changed

Lines changed: 130 additions & 67 deletions

File tree

src/uu/install/src/install.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -631,8 +631,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
631631
};
632632

633633
let dir_exists = if to_create.exists() {
634-
fs::symlink_metadata(to_create)
635-
.is_ok_and(|m| m.is_dir() && !m.file_type().is_symlink())
634+
metadata(to_create).is_ok_and(|m| m.is_dir())
636635
} else {
637636
false
638637
};
@@ -643,7 +642,7 @@ fn standard(mut paths: Vec<OsString>, b: &Behavior) -> UResult<()> {
643642
if b.target_dir.is_none()
644643
&& sources.len() == 1
645644
&& !is_potential_directory_path(&target)
646-
&& let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::NoFollow)
645+
&& let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::Follow)
647646
&& let Some(filename) = target.file_name()
648647
{
649648
target_parent_fd = Some(dir_fd);

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

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -377,16 +377,16 @@ fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec<OsString>)> {
377377
let mut components: Vec<OsString> = Vec::new();
378378

379379
loop {
380-
// Use symlink_metadata to NOT follow symlinks
381-
match fs::symlink_metadata(&current) {
380+
// Use metadata (follow symlinks) so that symlinks to directories are
381+
// treated as existing ancestors rather than components to create.
382+
match fs::metadata(&current) {
382383
Ok(meta) => {
383-
if meta.is_dir() && !meta.file_type().is_symlink() {
384-
// Found a real directory (not a symlink to a directory)
384+
if meta.is_dir() {
385+
// Found a directory (real or via symlink)
385386
components.reverse();
386387
return Ok((current, components));
387388
}
388-
// It's a symlink, file, or other non-directory - treat as needing creation
389-
// This ensures symlinks get replaced by open_or_create_subdir
389+
// It's a file or other non-directory - treat as needing creation
390390
if let Some(file_name) = current.file_name() {
391391
components.push(file_name.to_os_string());
392392
}
@@ -456,9 +456,10 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu
456456
match file_type {
457457
libc::S_IFDIR => parent_fd.open_subdir(name, SymlinkBehavior::NoFollow),
458458
libc::S_IFLNK => {
459-
parent_fd.unlink_at(name, false)?;
460-
parent_fd.mkdir_at(name, mode)?;
461-
parent_fd.open_subdir(name, SymlinkBehavior::NoFollow)
459+
// Follow symlinks to directories (GNU coreutils behavior).
460+
// O_DIRECTORY in open_subdir ensures we only succeed if the
461+
// symlink resolves to a directory; dangling or non-dir symlinks error out.
462+
parent_fd.open_subdir(name, SymlinkBehavior::Follow)
462463
}
463464
_ => Err(io::Error::new(
464465
io::ErrorKind::AlreadyExists,
@@ -1150,23 +1151,23 @@ mod tests {
11501151
}
11511152

11521153
#[test]
1153-
fn test_create_dir_all_safe_replaces_symlink() {
1154+
fn test_create_dir_all_safe_follows_symlink() {
11541155
let temp_dir = TempDir::new().unwrap();
11551156
let target_dir = temp_dir.path().join("target");
11561157
fs::create_dir(&target_dir).unwrap();
11571158

1158-
// Create a symlink where we want to create a directory
1159-
let symlink_path = temp_dir.path().join("link_to_replace");
1159+
// Create a symlink pointing to an existing directory
1160+
let symlink_path = temp_dir.path().join("link");
11601161
symlink(&target_dir, &symlink_path).unwrap();
11611162
assert!(symlink_path.is_symlink());
11621163

1163-
// create_dir_all_safe should replace the symlink with a real directory
1164+
// create_dir_all_safe should follow the symlink (GNU coreutils behavior)
11641165
let dir_fd = create_dir_all_safe(&symlink_path, 0o755).unwrap();
11651166
assert!(dir_fd.as_raw_fd() >= 0);
11661167

1167-
// Verify the symlink was replaced with a real directory
1168-
assert!(symlink_path.is_dir());
1169-
assert!(!symlink_path.is_symlink());
1168+
// Verify the symlink is preserved (not replaced with a real directory)
1169+
assert!(symlink_path.is_symlink());
1170+
assert!(symlink_path.is_dir()); // still resolves to a directory via the symlink
11701171
}
11711172

11721173
#[test]
@@ -1183,8 +1184,8 @@ mod tests {
11831184
fn test_create_dir_all_safe_nested_symlink_in_path() {
11841185
let temp_dir = TempDir::new().unwrap();
11851186

1186-
// Create: parent/symlink -> target
1187-
// Then try to create: parent/symlink/subdir
1187+
// Create: parent/link -> target
1188+
// Then create: parent/link/subdir
11881189
let parent = temp_dir.path().join("parent");
11891190
let target = temp_dir.path().join("target");
11901191
fs::create_dir(&parent).unwrap();
@@ -1193,18 +1194,17 @@ mod tests {
11931194
let symlink_in_path = parent.join("link");
11941195
symlink(&target, &symlink_in_path).unwrap();
11951196

1196-
// Try to create parent/link/subdir - the symlink should be replaced
1197+
// Try to create parent/link/subdir - the symlink should be followed (GNU behavior)
11971198
let nested_path = symlink_in_path.join("subdir");
11981199
let dir_fd = create_dir_all_safe(&nested_path, 0o755).unwrap();
11991200
assert!(dir_fd.as_raw_fd() >= 0);
12001201

1201-
// The symlink should have been replaced with a real directory
1202-
assert!(!symlink_in_path.is_symlink());
1203-
assert!(symlink_in_path.is_dir());
1204-
assert!(nested_path.is_dir());
1202+
// The symlink should be preserved, not replaced
1203+
assert!(symlink_in_path.is_symlink());
1204+
assert!(symlink_in_path.is_dir()); // resolves via symlink
12051205

1206-
// Target directory should not contain subdir (race attack prevented)
1207-
assert!(!target.join("subdir").exists());
1206+
// subdir should have been created inside the real target directory
1207+
assert!(target.join("subdir").exists());
12081208
}
12091209

12101210
#[test]

tests/by-util/test_install.rs

Lines changed: 103 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -2573,90 +2573,154 @@ fn test_install_normal_file_replaces_symlink() {
25732573
#[test]
25742574
#[cfg(unix)]
25752575
fn test_install_d_symlink_race_condition() {
2576-
// Test for symlink race condition fix (issue #10013)
2577-
// Verifies that pre-existing symlinks in path are handled safely
2576+
// Test that pre-existing symlinks in the path are followed (GNU coreutils behavior).
2577+
// install -D should traverse symlink components rather than replacing them.
25782578
use std::os::unix::fs::symlink;
25792579

25802580
let scene = TestScenario::new(util_name!());
25812581
let at = &scene.fixtures;
25822582

2583-
// Create test directories
25842583
at.mkdir("target");
2585-
2586-
// Create source file
25872584
at.write("source_file", "test content");
25882585

2589-
// Set up a pre-existing symlink attack scenario
25902586
at.mkdir_all("testdir/a");
2591-
let intermediate_dir = at.plus("testdir/a/b");
2592-
symlink(at.plus("target"), &intermediate_dir).unwrap();
2587+
symlink(at.plus("target"), at.plus("testdir/a/b")).unwrap();
25932588

2594-
// Run install -D which should detect and handle the symlink
2595-
let result = scene
2589+
// install -D should follow the symlink and write into the symlink target
2590+
scene
25962591
.ucmd()
25972592
.arg("-D")
25982593
.arg(at.plus("source_file"))
25992594
.arg(at.plus("testdir/a/b/c/file"))
2600-
.run();
2601-
2602-
let wrong_location = at.plus("target/c/file");
2595+
.succeeds();
26032596

2604-
// The critical assertion: file must NOT be in symlink target (race prevented)
2597+
// File must be written through the symlink, i.e. inside the real target dir
26052598
assert!(
2606-
!wrong_location.exists(),
2607-
"RACE CONDITION NOT PREVENTED: File was created in symlink target"
2599+
at.plus("target/c/file").exists(),
2600+
"File should be written through the symlink into the real target directory"
2601+
);
2602+
assert_eq!(
2603+
fs::read_to_string(at.plus("target/c/file")).unwrap(),
2604+
"test content"
26082605
);
26092606

2610-
// If the command succeeded, verify the file is in the correct location
2611-
if result.succeeded() {
2612-
assert!(at.file_exists("testdir/a/b/c/file"));
2613-
assert_eq!(at.read("testdir/a/b/c/file"), "test content");
2614-
// The symlink should have been replaced with a real directory
2615-
assert!(
2616-
at.plus("testdir/a/b").is_dir() && !at.plus("testdir/a/b").is_symlink(),
2617-
"Intermediate path should be a real directory, not a symlink"
2618-
);
2619-
}
2607+
// The symlink must not have been replaced with a real directory
2608+
assert!(
2609+
at.plus("testdir/a/b").is_symlink(),
2610+
"Intermediate symlink should be preserved, not replaced with a real directory"
2611+
);
26202612
}
26212613

26222614
#[test]
26232615
#[cfg(unix)]
26242616
fn test_install_d_symlink_race_condition_concurrent() {
2625-
// Test pre-existing symlinks in intermediate paths are handled correctly
2617+
// Verify symlink-following behavior is consistent (companion to the test above).
26262618
use std::os::unix::fs::symlink;
26272619

26282620
let scene = TestScenario::new(util_name!());
26292621
let at = &scene.fixtures;
26302622

2631-
// Create test directories and source file using testing framework
26322623
at.mkdir("target2");
26332624
at.write("source_file2", "test content 2");
26342625

2635-
// Set up intermediate directory with symlink
26362626
at.mkdir_all("testdir2/a");
26372627
symlink(at.plus("target2"), at.plus("testdir2/a/b")).unwrap();
26382628

2639-
// Run install -D
26402629
scene
26412630
.ucmd()
26422631
.arg("-D")
26432632
.arg(at.plus("source_file2"))
26442633
.arg(at.plus("testdir2/a/b/c/file"))
26452634
.succeeds();
26462635

2647-
// Verify file was created at the intended destination
2648-
assert!(at.file_exists("testdir2/a/b/c/file"));
2649-
assert_eq!(at.read("testdir2/a/b/c/file"), "test content 2");
2636+
// File should be in the real target directory (symlink was followed)
2637+
assert!(
2638+
at.plus("target2/c/file").exists(),
2639+
"File should be written through the symlink into the real target directory"
2640+
);
2641+
assert_eq!(
2642+
fs::read_to_string(at.plus("target2/c/file")).unwrap(),
2643+
"test content 2"
2644+
);
26502645

2651-
// Verify file was NOT created in symlink target
2646+
// Symlink should be preserved
26522647
assert!(
2653-
!at.plus("target2/c/file").exists(),
2654-
"File should NOT be in symlink target location"
2648+
at.plus("testdir2/a/b").is_symlink(),
2649+
"Intermediate symlink should be preserved, not replaced with a real directory"
26552650
);
2651+
}
2652+
2653+
#[test]
2654+
#[cfg(unix)]
2655+
fn test_install_d_follows_symlink_prefix() {
2656+
// Regression test for: install -D replaces symlink components instead of following them.
2657+
// Reproduces the exact scenario from the bug report: a symlinked install prefix
2658+
// (common in BOSH, Homebrew, Nix, stow) must be followed, not destroyed.
2659+
use std::os::unix::fs::symlink;
2660+
2661+
let scene = TestScenario::new(util_name!());
2662+
let at = &scene.fixtures;
2663+
2664+
// Simulate: ln -s /tmp/target /tmp/link
2665+
at.mkdir("target");
2666+
symlink(at.plus("target"), at.plus("link")).unwrap();
2667+
2668+
at.write("file.txt", "hello");
26562669

2657-
// Verify intermediate path is now a real directory
2670+
// install -D -m 644 file.txt link/subdir/file.txt
2671+
scene
2672+
.ucmd()
2673+
.args(&["-D", "-m", "644"])
2674+
.arg(at.plus("file.txt"))
2675+
.arg(at.plus("link/subdir/file.txt"))
2676+
.succeeds();
2677+
2678+
// GNU expected: /tmp/link remains a symlink, file written to /tmp/target/subdir/file.txt
2679+
assert!(
2680+
at.plus("link").is_symlink(),
2681+
"The symlinked prefix must remain a symlink"
2682+
);
2683+
assert!(
2684+
at.plus("target/subdir/file.txt").exists(),
2685+
"File must be written into the real target directory via the symlink"
2686+
);
2687+
assert_eq!(
2688+
fs::read_to_string(at.plus("target/subdir/file.txt")).unwrap(),
2689+
"hello"
2690+
);
2691+
}
2692+
2693+
#[test]
2694+
#[cfg(unix)]
2695+
fn test_install_d_dangling_symlink_in_path_errors() {
2696+
// A dangling symlink as a path component must not be silently replaced with a
2697+
// real directory. GNU coreutils errors out in this case; we should too.
2698+
use std::os::unix::fs::symlink;
2699+
2700+
let scene = TestScenario::new(util_name!());
2701+
let at = &scene.fixtures;
2702+
2703+
// Create a symlink pointing to a nonexistent target (dangling)
2704+
symlink(at.plus("nonexistent"), at.plus("dangling")).unwrap();
2705+
assert!(at.plus("dangling").is_symlink());
2706+
2707+
at.write("file.txt", "hello");
2708+
2709+
// install -D file.txt dangling/subdir/file.txt should fail
2710+
scene
2711+
.ucmd()
2712+
.args(&["-D", "-m", "644"])
2713+
.arg(at.plus("file.txt"))
2714+
.arg(at.plus("dangling/subdir/file.txt"))
2715+
.fails();
2716+
2717+
// The dangling symlink must not have been replaced with a real directory
2718+
assert!(
2719+
at.plus("dangling").is_symlink(),
2720+
"Dangling symlink must not be replaced with a real directory"
2721+
);
26582722
assert!(
2659-
at.plus("testdir2/a/b").is_dir() && !at.plus("testdir2/a/b").is_symlink(),
2660-
"Intermediate directory should be a real directory, not a symlink"
2723+
!at.plus("nonexistent").exists(),
2724+
"The symlink target must not have been created"
26612725
);
26622726
}

0 commit comments

Comments
 (0)