install: update comments and tests

Fix doc comments in safe_traversal that still described the old behavior
of replacing symlinks with real directories. Rename and deduplicate tests
that were originally written as race-condition regression tests but now
just verify symlink-following behavior.

Signed-off-by: Etienne Cordonnier <ecordonnier@snap.com>

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

@@ -465,11 +465,11 @@ impl DirFd {
     }
 }
 
-/// Find the deepest existing real directory ancestor for a path.
+/// Find the deepest existing 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
-/// treated as components that need to be created/replaced.
+/// Uses `metadata` (follows symlinks) so that symlinks to directories are treated as
+/// existing ancestors rather than components to create.
 fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec<OsString>)> {
     let mut current = path.to_path_buf();
     let mut components: Vec<OsString> = Vec::new();
@@ -537,8 +537,8 @@ fn find_existing_ancestor(path: &Path) -> io::Result<(PathBuf, Vec<OsString>)> {
 /// 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
-/// removed and replaced with a real directory.
+/// path component. If a symlink to a directory exists, it is followed (GNU
+/// coreutils behavior). Dangling symlinks and non-directory entries are errors.
 ///
 /// # Arguments
 /// * `parent_fd` - The parent directory file descriptor
@@ -580,8 +580,8 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu
 /// This prevents symlink race conditions by anchoring all operations to directory fds.
 ///
 /// # Security
-/// This function prevents TOCTOU race conditions by:
-/// 1. Finding the deepest existing ancestor directory (path-based, but safe since it exists)
+/// This function prevents TOCTOU race conditions for newly created directories by:
+/// 1. Finding the deepest existing ancestor directory (path-based, following symlinks)
 /// 2. Opening that ancestor with a file descriptor
 /// 3. Creating all new directories using fd-based operations (mkdirat, openat with O_NOFOLLOW)
 ///
@@ -589,8 +589,10 @@ fn open_or_create_subdir(parent_fd: &DirFd, name: &OsStr, mode: u32) -> io::Resu
 /// as the anchor. If an attacker replaces a newly-created directory with a symlink,
 /// our openat with O_NOFOLLOW will fail, preventing the attack.
 ///
-/// Existing symlinks in the path (like /var -> /private/var on macOS) are followed
-/// when finding the ancestor, which is safe since they already exist.
+/// Pre-existing symlinks to directories in the path are followed (GNU coreutils behavior).
+/// `O_DIRECTORY` is used when opening them, so dangling or non-directory symlinks error out.
+/// Note that a residual TOCTOU window exists between stat and open for such symlinks,
+/// which is the same trade-off made by GNU coreutils.
 ///
 /// # Arguments
 /// * `path` - The path to create directories for

tests/by-util/test_install.rs

@@ -2582,7 +2582,7 @@ fn test_install_normal_file_replaces_symlink() {
 
 #[test]
 #[cfg(unix)]
-fn test_install_d_symlink_race_condition() {
+fn test_install_d_symlink_in_path_is_followed() {
     // 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;
@@ -2621,45 +2621,6 @@ fn test_install_d_symlink_race_condition() {
     );
 }
 
-#[test]
-#[cfg(unix)]
-fn test_install_d_symlink_race_condition_concurrent() {
-    // 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;
-
-    at.mkdir("target2");
-    at.write("source_file2", "test content 2");
-
-    at.mkdir_all("testdir2/a");
-    symlink(at.plus("target2"), at.plus("testdir2/a/b")).unwrap();
-
-    scene
-        .ucmd()
-        .arg("-D")
-        .arg(at.plus("source_file2"))
-        .arg(at.plus("testdir2/a/b/c/file"))
-        .succeeds();
-
-    // 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"
-    );
-
-    // Symlink should be preserved
-    assert!(
-        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() {