From ed62194e7b0cb635f608080d3ab8140705be3045 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Thu, 26 Mar 2026 03:55:53 -0700 Subject: [PATCH] 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). --- src/uu/install/src/install.rs | 5 +- src/uucore/src/lib/features/safe_traversal.rs | 50 +++--- tests/by-util/test_install.rs | 142 +++++++++++++----- 3 files changed, 130 insertions(+), 67 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index a683ff9b908..362f3db1d04 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -631,8 +631,7 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { }; let dir_exists = if to_create.exists() { - fs::symlink_metadata(to_create) - .is_ok_and(|m| m.is_dir() && !m.file_type().is_symlink()) + metadata(to_create).is_ok_and(|m| m.is_dir()) } else { false }; @@ -643,7 +642,7 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { if b.target_dir.is_none() && sources.len() == 1 && !is_potential_directory_path(&target) - && let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::NoFollow) + && let Ok(dir_fd) = DirFd::open(to_create, SymlinkBehavior::Follow) && let Some(filename) = target.file_name() { target_parent_fd = Some(dir_fd); diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index 98fc81dd928..0723ca419bf 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -377,16 +377,16 @@ fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec)> { let mut components: Vec = Vec::new(); loop { - // Use symlink_metadata to NOT follow symlinks - match fs::symlink_metadata(¤t) { + // Use metadata (follow symlinks) so that symlinks to directories are + // treated as existing ancestors rather than components to create. + match fs::metadata(¤t) { Ok(meta) => { - if meta.is_dir() && !meta.file_type().is_symlink() { - // Found a real directory (not a symlink to a directory) + if meta.is_dir() { + // Found a directory (real or via symlink) components.reverse(); return Ok((current, components)); } - // It's a symlink, file, or other non-directory - treat as needing creation - // This ensures symlinks get replaced by open_or_create_subdir + // It's a file or other non-directory - treat as needing creation if let Some(file_name) = current.file_name() { components.push(file_name.to_os_string()); } @@ -456,9 +456,10 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu match file_type { libc::S_IFDIR => parent_fd.open_subdir(name, SymlinkBehavior::NoFollow), libc::S_IFLNK => { - parent_fd.unlink_at(name, false)?; - parent_fd.mkdir_at(name, mode)?; - parent_fd.open_subdir(name, SymlinkBehavior::NoFollow) + // Follow symlinks to directories (GNU coreutils behavior). + // O_DIRECTORY in open_subdir ensures we only succeed if the + // symlink resolves to a directory; dangling or non-dir symlinks error out. + parent_fd.open_subdir(name, SymlinkBehavior::Follow) } _ => Err(io::Error::new( io::ErrorKind::AlreadyExists, @@ -1150,23 +1151,23 @@ mod tests { } #[test] - fn test_create_dir_all_safe_replaces_symlink() { + fn test_create_dir_all_safe_follows_symlink() { let temp_dir = TempDir::new().unwrap(); let target_dir = temp_dir.path().join("target"); fs::create_dir(&target_dir).unwrap(); - // Create a symlink where we want to create a directory - let symlink_path = temp_dir.path().join("link_to_replace"); + // Create a symlink pointing to an existing directory + let symlink_path = temp_dir.path().join("link"); symlink(&target_dir, &symlink_path).unwrap(); assert!(symlink_path.is_symlink()); - // create_dir_all_safe should replace the symlink with a real directory + // create_dir_all_safe should follow the symlink (GNU coreutils behavior) let dir_fd = create_dir_all_safe(&symlink_path, 0o755).unwrap(); assert!(dir_fd.as_raw_fd() >= 0); - // Verify the symlink was replaced with a real directory - assert!(symlink_path.is_dir()); - assert!(!symlink_path.is_symlink()); + // Verify the symlink is preserved (not replaced with a real directory) + assert!(symlink_path.is_symlink()); + assert!(symlink_path.is_dir()); // still resolves to a directory via the symlink } #[test] @@ -1183,8 +1184,8 @@ mod tests { fn test_create_dir_all_safe_nested_symlink_in_path() { let temp_dir = TempDir::new().unwrap(); - // Create: parent/symlink -> target - // Then try to create: parent/symlink/subdir + // Create: parent/link -> target + // Then create: parent/link/subdir let parent = temp_dir.path().join("parent"); let target = temp_dir.path().join("target"); fs::create_dir(&parent).unwrap(); @@ -1193,18 +1194,17 @@ mod tests { let symlink_in_path = parent.join("link"); symlink(&target, &symlink_in_path).unwrap(); - // Try to create parent/link/subdir - the symlink should be replaced + // Try to create parent/link/subdir - the symlink should be followed (GNU behavior) let nested_path = symlink_in_path.join("subdir"); let dir_fd = create_dir_all_safe(&nested_path, 0o755).unwrap(); assert!(dir_fd.as_raw_fd() >= 0); - // The symlink should have been replaced with a real directory - assert!(!symlink_in_path.is_symlink()); - assert!(symlink_in_path.is_dir()); - assert!(nested_path.is_dir()); + // The symlink should be preserved, not replaced + assert!(symlink_in_path.is_symlink()); + assert!(symlink_in_path.is_dir()); // resolves via symlink - // Target directory should not contain subdir (race attack prevented) - assert!(!target.join("subdir").exists()); + // subdir should have been created inside the real target directory + assert!(target.join("subdir").exists()); } #[test] diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 5e0483ca0e0..04613458cd1 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -2573,70 +2573,59 @@ fn test_install_normal_file_replaces_symlink() { #[test] #[cfg(unix)] 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). + // install -D should traverse symlink components rather than replacing them. use std::os::unix::fs::symlink; let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; - // Create test directories at.mkdir("target"); - - // Create source file at.write("source_file", "test content"); - // Set up a pre-existing symlink attack scenario at.mkdir_all("testdir/a"); - let intermediate_dir = at.plus("testdir/a/b"); - symlink(at.plus("target"), &intermediate_dir).unwrap(); + symlink(at.plus("target"), at.plus("testdir/a/b")).unwrap(); - // Run install -D which should detect and handle the symlink - let result = scene + // install -D should follow the symlink and write into the symlink target + scene .ucmd() .arg("-D") .arg(at.plus("source_file")) .arg(at.plus("testdir/a/b/c/file")) - .run(); - - let wrong_location = at.plus("target/c/file"); + .succeeds(); - // The critical assertion: file must NOT be in symlink target (race prevented) + // File must be written through the symlink, i.e. inside the real target dir assert!( - !wrong_location.exists(), - "RACE CONDITION NOT PREVENTED: File was created in symlink target" + at.plus("target/c/file").exists(), + "File should be written through the symlink into the real target directory" + ); + assert_eq!( + fs::read_to_string(at.plus("target/c/file")).unwrap(), + "test content" ); - // If the command succeeded, verify the file is in the correct location - if result.succeeded() { - assert!(at.file_exists("testdir/a/b/c/file")); - assert_eq!(at.read("testdir/a/b/c/file"), "test content"); - // The symlink should have been replaced with a real directory - assert!( - at.plus("testdir/a/b").is_dir() && !at.plus("testdir/a/b").is_symlink(), - "Intermediate path should be a real directory, not a symlink" - ); - } + // The symlink must not have been replaced with a real directory + assert!( + at.plus("testdir/a/b").is_symlink(), + "Intermediate symlink should be preserved, not replaced with a real directory" + ); } #[test] #[cfg(unix)] fn test_install_d_symlink_race_condition_concurrent() { - // Test pre-existing symlinks in intermediate paths are handled correctly + // Verify symlink-following behavior is consistent (companion to the test above). use std::os::unix::fs::symlink; let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; - // Create test directories and source file using testing framework at.mkdir("target2"); at.write("source_file2", "test content 2"); - // Set up intermediate directory with symlink at.mkdir_all("testdir2/a"); symlink(at.plus("target2"), at.plus("testdir2/a/b")).unwrap(); - // Run install -D scene .ucmd() .arg("-D") @@ -2644,19 +2633,94 @@ fn test_install_d_symlink_race_condition_concurrent() { .arg(at.plus("testdir2/a/b/c/file")) .succeeds(); - // Verify file was created at the intended destination - assert!(at.file_exists("testdir2/a/b/c/file")); - assert_eq!(at.read("testdir2/a/b/c/file"), "test content 2"); + // File should be in the real target directory (symlink was followed) + assert!( + at.plus("target2/c/file").exists(), + "File should be written through the symlink into the real target directory" + ); + assert_eq!( + fs::read_to_string(at.plus("target2/c/file")).unwrap(), + "test content 2" + ); - // Verify file was NOT created in symlink target + // Symlink should be preserved assert!( - !at.plus("target2/c/file").exists(), - "File should NOT be in symlink target location" + at.plus("testdir2/a/b").is_symlink(), + "Intermediate symlink should be preserved, not replaced with a real directory" ); +} + +#[test] +#[cfg(unix)] +fn test_install_d_follows_symlink_prefix() { + // Regression test for: install -D replaces symlink components instead of following them. + // Reproduces the exact scenario from the bug report: a symlinked install prefix + // (common in BOSH, Homebrew, Nix, stow) must be followed, not destroyed. + use std::os::unix::fs::symlink; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // Simulate: ln -s /tmp/target /tmp/link + at.mkdir("target"); + symlink(at.plus("target"), at.plus("link")).unwrap(); + + at.write("file.txt", "hello"); - // Verify intermediate path is now a real directory + // install -D -m 644 file.txt link/subdir/file.txt + scene + .ucmd() + .args(&["-D", "-m", "644"]) + .arg(at.plus("file.txt")) + .arg(at.plus("link/subdir/file.txt")) + .succeeds(); + + // GNU expected: /tmp/link remains a symlink, file written to /tmp/target/subdir/file.txt + assert!( + at.plus("link").is_symlink(), + "The symlinked prefix must remain a symlink" + ); + assert!( + at.plus("target/subdir/file.txt").exists(), + "File must be written into the real target directory via the symlink" + ); + assert_eq!( + fs::read_to_string(at.plus("target/subdir/file.txt")).unwrap(), + "hello" + ); +} + +#[test] +#[cfg(unix)] +fn test_install_d_dangling_symlink_in_path_errors() { + // A dangling symlink as a path component must not be silently replaced with a + // real directory. GNU coreutils errors out in this case; we should too. + use std::os::unix::fs::symlink; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // Create a symlink pointing to a nonexistent target (dangling) + symlink(at.plus("nonexistent"), at.plus("dangling")).unwrap(); + assert!(at.plus("dangling").is_symlink()); + + at.write("file.txt", "hello"); + + // install -D file.txt dangling/subdir/file.txt should fail + scene + .ucmd() + .args(&["-D", "-m", "644"]) + .arg(at.plus("file.txt")) + .arg(at.plus("dangling/subdir/file.txt")) + .fails(); + + // The dangling symlink must not have been replaced with a real directory + assert!( + at.plus("dangling").is_symlink(), + "Dangling symlink must not be replaced with a real directory" + ); assert!( - at.plus("testdir2/a/b").is_dir() && !at.plus("testdir2/a/b").is_symlink(), - "Intermediate directory should be a real directory, not a symlink" + !at.plus("nonexistent").exists(), + "The symlink target must not have been created" ); }