chmod/chown: symlink cycles detection (#11805)

src/uu/chmod/src/chmod.rs

@@ -6,6 +6,7 @@
 // spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's
 
 use clap::{Arg, ArgAction, Command};
+use std::collections::HashSet;
 use std::ffi::OsString;
 use std::fs;
 use std::os::unix::fs::{MetadataExt, PermissionsExt};
@@ -13,7 +14,7 @@ use std::path::{Path, PathBuf};
 use thiserror::Error;
 use uucore::display::Quotable;
 use uucore::error::{ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code};
-use uucore::fs::display_permissions_unix;
+use uucore::fs::{FileInformation, display_permissions_unix};
 use uucore::mode;
 use uucore::perms::{TraverseSymlinks, configure_symlink_and_recursion};
 
@@ -335,6 +336,7 @@ impl Chmoder {
         &self,
         path: &Path,
         is_command_line_arg: bool,
+        ancestors: &mut HashSet<FileInformation>,
     ) -> UResult<()> {
         let should_follow_symlink = match self.traverse_symlinks {
             TraverseSymlinks::All => true,
@@ -347,7 +349,7 @@ impl Chmoder {
         }
 
         match fs::metadata(path) {
-            Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false),
+            Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false, ancestors),
             Ok(_) => {
                 // It's a file symlink, chmod it
                 self.chmod_file(path)
@@ -403,7 +405,10 @@ impl Chmoder {
                 return Err(ChmodError::PreserveRoot("/".into()).into());
             }
             if self.recursive {
-                r = self.walk_dir_with_context(file, true).and(r);
+                let mut ancestors = HashSet::new();
+                r = self
+                    .walk_dir_with_context(file, true, &mut ancestors)
+                    .and(r);
             } else {
                 r = self.chmod_file(file).and(r);
             }
@@ -417,7 +422,12 @@ impl Chmoder {
 
     // Non-safe traversal implementation for platforms without safe_traversal support
     #[cfg(any(not(unix), target_os = "redox"))]
-    fn walk_dir_with_context(&self, file_path: &Path, is_command_line_arg: bool) -> UResult<()> {
+    fn walk_dir_with_context(
+        &self,
+        file_path: &Path,
+        is_command_line_arg: bool,
+        ancestors: &mut HashSet<FileInformation>,
+    ) -> UResult<()> {
         let mut r = self.chmod_file(file_path);
 
         // Determine whether to traverse symlinks based on context and traversal mode
@@ -429,6 +439,17 @@ impl Chmoder {
 
         // If the path is a directory (or we should follow symlinks), recurse into it
         if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() {
+            // Cycle detection: identify this directory by its target's inode (resolved
+            // via dereference=true) once, and reuse it for the backtrack below so we
+            // don't stat the same directory twice. If it's already on the current path,
+            // it's a cycle.
+            let dir_info = FileInformation::from_path(file_path, true).ok();
+            if let Some(info) = &dir_info {
+                if !ancestors.insert(info.clone()) {
+                    return r;
+                }
+            }
+
             // We buffer all paths in this dir to not keep too many fd's open during recursion
             let mut paths_in_this_dir = Vec::new();
 
@@ -445,22 +466,39 @@ impl Chmoder {
                 #[cfg(not(unix))]
                 {
                     if path.is_symlink() {
-                        r = self.handle_symlink_during_recursion(&path).and(r);
+                        r = self
+                            .handle_symlink_during_recursion(&path, ancestors)
+                            .and(r);
                     } else {
-                        r = self.walk_dir_with_context(path.as_path(), false).and(r);
+                        r = self
+                            .walk_dir_with_context(path.as_path(), false, ancestors)
+                            .and(r);
                     }
                 }
                 #[cfg(target_os = "redox")]
                 {
-                    r = self.walk_dir_with_context(path.as_path(), false).and(r);
+                    r = self
+                        .walk_dir_with_context(path.as_path(), false, ancestors)
+                        .and(r);
                 }
             }
+
+            // Backtrack: remove this directory from ancestors so sibling subtrees
+            // are not falsely treated as cycles.
+            if let Some(info) = dir_info {
+                ancestors.remove(&info);
+            }
         }
         r
     }
 
     #[cfg(all(unix, not(target_os = "redox")))]
-    fn walk_dir_with_context(&self, file_path: &Path, is_command_line_arg: bool) -> UResult<()> {
+    fn walk_dir_with_context(
+        &self,
+        file_path: &Path,
+        is_command_line_arg: bool,
+        ancestors: &mut HashSet<FileInformation>,
+    ) -> UResult<()> {
         let mut r = self.chmod_file(file_path);
 
         // Determine whether to traverse symlinks based on context and traversal mode
@@ -474,7 +512,7 @@ impl Chmoder {
         if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() {
             match DirFd::open(file_path, SymlinkBehavior::Follow) {
                 Ok(dir_fd) => {
-                    r = self.safe_traverse_dir(&dir_fd, file_path).and(r);
+                    r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r);
                 }
                 Err(err) => {
                     // Handle permission denied errors with proper file path context
@@ -490,9 +528,24 @@ impl Chmoder {
     }
 
     #[cfg(all(unix, not(target_os = "redox")))]
-    fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path) -> UResult<()> {
+    fn safe_traverse_dir(
+        &self,
+        dir_fd: &DirFd,
+        dir_path: &Path,
+        ancestors: &mut HashSet<FileInformation>,
+    ) -> UResult<()> {
         let mut r = Ok(());
 
+        // Cycle detection: identify this directory by (dev, ino) via the already-open
+        // fd. Using the fd is TOCTOU-safe (no path re-resolution through symlinks) and
+        // avoids a redundant path walk. If it's already on the current path, it's a cycle.
+        let dir_info = FileInformation::from_file(dir_fd).ok();
+        if let Some(info) = &dir_info {
+            if !ancestors.insert(info.clone()) {
+                return r; // cycle: this directory is already an ancestor
+            }
+        }
+
         let entries = dir_fd.read_dir()?;
 
         // Determine if we should follow symlinks (doesn't depend on entry_name)
@@ -516,7 +569,12 @@ impl Chmoder {
 
             if entry_path.is_symlink() {
                 r = self
-                    .handle_symlink_during_safe_recursion(&entry_path, dir_fd, &entry_name)
+                    .handle_symlink_during_safe_recursion(
+                        &entry_path,
+                        dir_fd,
+                        &entry_name,
+                        ancestors,
+                    )
                     .and(r);
             } else {
                 // For regular files and directories, chmod them.
@@ -540,7 +598,9 @@ impl Chmoder {
                 if meta.is_dir() {
                     match dir_fd.open_subdir(&entry_name, should_follow_symlink.into()) {
                         Ok(child_dir_fd) => {
-                            r = self.safe_traverse_dir(&child_dir_fd, &entry_path).and(r);
+                            r = self
+                                .safe_traverse_dir(&child_dir_fd, &entry_path, ancestors)
+                                .and(r);
                         }
                         Err(err) => {
                             let error = if err.kind() == std::io::ErrorKind::PermissionDenied {
@@ -554,6 +614,13 @@ impl Chmoder {
                 }
             }
         }
+
+        // Backtrack so sibling subtrees that legitimately reach the same directory
+        // (e.g. two symlinks to one dir) are not mistaken for cycles.
+        if let Some(info) = dir_info {
+            ancestors.remove(&info);
+        }
+
         r
     }
 
@@ -563,6 +630,7 @@ impl Chmoder {
         path: &Path,
         dir_fd: &DirFd,
         entry_name: &std::ffi::OsStr,
+        ancestors: &mut HashSet<FileInformation>,
     ) -> UResult<()> {
         // During recursion, determine behavior based on traversal mode
         match self.traverse_symlinks {
@@ -574,7 +642,7 @@ impl Chmoder {
                 // followed by Follow is the intended behavior and not a TOCTOU concern.
                 // Check if the symlink target is a directory, but handle dangling symlinks gracefully
                 match fs::metadata(path) {
-                    Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false),
+                    Ok(meta) if meta.is_dir() => self.walk_dir_with_context(path, false, ancestors),
                     Ok(meta) => {
                         // It's a file symlink, chmod it using safe traversal
                         self.safe_chmod_file(
@@ -629,9 +697,13 @@ impl Chmoder {
     }
 
     #[cfg(not(unix))]
-    fn handle_symlink_during_recursion(&self, path: &Path) -> UResult<()> {
+    fn handle_symlink_during_recursion(
+        &self,
+        path: &Path,
+        ancestors: &mut HashSet<FileInformation>,
+    ) -> UResult<()> {
         // Use the common symlink handling logic
-        self.handle_symlink_during_traversal(path, false)
+        self.handle_symlink_during_traversal(path, false, ancestors)
     }
 
     fn chmod_file(&self, file: &Path) -> UResult<()> {

src/uucore/src/lib/features/fs.rs

@@ -38,6 +38,7 @@ macro_rules! has {
 }
 
 /// Information to uniquely identify a file
+#[derive(Clone)]
 pub struct FileInformation(
     #[cfg(unix)] rustix::fs::Stat,
     #[cfg(windows)] winapi_util::file::Information,

src/uucore/src/lib/features/perms.rs

@@ -16,11 +16,15 @@ use clap::{Arg, ArgMatches, Command};
 
 use libc::{gid_t, uid_t};
 use options::traverse;
+#[cfg(target_os = "linux")]
+use std::collections::HashSet;
 use std::ffi::OsString;
 
 #[cfg(not(target_os = "linux"))]
 use walkdir::WalkDir;
 
+#[cfg(target_os = "linux")]
+use crate::features::fs::FileInformation;
 #[cfg(target_os = "linux")]
 use crate::features::safe_traversal::{DirFd, SymlinkBehavior};
 
@@ -450,13 +454,30 @@ impl ChownExecutor {
             return 1;
         };
 
+        let mut ancestors = HashSet::new();
         let mut ret = 0;
-        self.safe_traverse_dir(&dir_fd, root, &mut ret);
+        self.safe_traverse_dir(&dir_fd, root, &mut ret, &mut ancestors);
         ret
     }
 
     #[cfg(target_os = "linux")]
-    fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path, ret: &mut i32) {
+    fn safe_traverse_dir(
+        &self,
+        dir_fd: &DirFd,
+        dir_path: &Path,
+        ret: &mut i32,
+        ancestors: &mut HashSet<FileInformation>,
+    ) {
+        // Cycle detection: identify this directory by (dev, ino) via the already-open
+        // fd. Using the fd is TOCTOU-safe (no path re-resolution through symlinks) and
+        // avoids a redundant path walk. If it's already on the current path, it's a cycle.
+        let dir_info = FileInformation::from_file(dir_fd).ok();
+        if let Some(info) = &dir_info {
+            if !ancestors.insert(info.clone()) {
+                return; // cycle detected, stop silently
+            }
+        }
+
         // Read directory entries
         let entries = match dir_fd.read_dir() {
             Ok(entries) => entries,
@@ -544,7 +565,7 @@ impl ChownExecutor {
             if meta.is_dir() && (follow || !meta.file_type().is_symlink()) {
                 match dir_fd.open_subdir(&entry_name, follow.into()) {
                     Ok(subdir_fd) => {
-                        self.safe_traverse_dir(&subdir_fd, &entry_path, ret);
+                        self.safe_traverse_dir(&subdir_fd, &entry_path, ret, ancestors);
                     }
                     Err(e) => {
                         *ret = 1;
@@ -559,6 +580,12 @@ impl ChownExecutor {
                 }
             }
         }
+
+        // Backtrack so sibling subtrees that legitimately reach the same directory
+        // (e.g. two symlinks to one dir) are not mistaken for cycles.
+        if let Some(info) = dir_info {
+            ancestors.remove(&info);
+        }
     }
 
     #[cfg(not(target_os = "linux"))]

tests/by-util/test_chmod.rs

@@ -2,7 +2,7 @@
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
-// spell-checker:ignore (words) dirfd subdirs openat FDCWD
+// spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr
 
 use std::fs::{OpenOptions, Permissions, metadata, set_permissions};
 use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
@@ -1440,3 +1440,63 @@ fn test_chmod_colored_output() {
         .stderr_contains("\x1b[31merreur\x1b[0m") // Red "erreur" in French
         .stderr_contains("\x1b[33m--invalid-option\x1b[0m"); // Yellow invalid option
 }
+
+#[test]
+fn test_chmod_symlink_cycles() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    at.mkdir_all("a/b/c");
+    at.symlink_dir("a", "a/b/c/d");
+
+    // Pin the directory modes so the assertions below don't depend on the ambient
+    // umask (a fresh mkdir under umask 077/027 would not be 0755).
+    at.set_mode("a", 0o755);
+    at.set_mode("a/b", 0o755);
+    at.set_mode("a/b/c", 0o755);
+
+    scene
+        .ucmd()
+        .arg("-vRL")
+        .arg("+r")
+        .arg("a")
+        .run()
+        // cSpell:disable
+        .stdout_contains_line("mode of 'a' retained as 0755 (rwxr-xr-x)")
+        .stdout_contains_line("mode of 'a/b' retained as 0755 (rwxr-xr-x)")
+        .stdout_contains_line("mode of 'a/b/c' retained as 0755 (rwxr-xr-x)")
+        .stdout_contains_line("mode of 'a/b/c/d' retained as 0755 (rwxr-xr-x)")
+        .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0755 (rwxr-xr-x)")
+        .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0755 (rwxr-xr-x)")
+        .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0755 (rwxr-xr-x)");
+    // cSpell:enable
+}
+
+#[test]
+fn test_chmod_symlink_two_links_same_dir() {
+    // Two symlinks pointing at the same directory is NOT a cycle: neither link is
+    // an ancestor of the other, so the target's contents must be visited through
+    // *both* links (and through the real directory). This guards the backtracking
+    // in the cycle-detection logic against false positives.
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    // cSpell:disable
+    at.mkdir_all("base/realdir");
+    at.touch("base/realdir/file");
+    at.symlink_dir("base/realdir", "base/link1");
+    at.symlink_dir("base/realdir", "base/link2");
+
+    // Assert on the path only (not the mode bits), so the test is robust to the
+    // file's umask-dependent permissions across platforms.
+    scene
+        .ucmd()
+        .arg("-vRL")
+        .arg("+r")
+        .arg("base")
+        .run()
+        .stdout_contains("mode of 'base/realdir/file'")
+        .stdout_contains("mode of 'base/link1/file'")
+        .stdout_contains("mode of 'base/link2/file'");
+    // cSpell:enable
+}

tests/by-util/test_chown.rs

@@ -943,3 +943,89 @@ fn test_chown_no_dereference_symlink_to_dir() {
         "dir's ctime should not have changed"
     );
 }
+
+#[test]
+fn test_chown_symlink_cycles() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let result = scene.cmd("whoami").run();
+    if skipping_test_is_okay(&result, "whoami: cannot find name for user ID") {
+        return;
+    }
+    let user_name = String::from(result.stdout_str().trim());
+    assert!(!user_name.is_empty());
+
+    at.mkdir_all("a/b/c");
+    at.symlink_dir("a", "a/b/c/d");
+
+    let result = scene.ucmd().arg("-vRL").arg(&user_name).arg("a").run();
+
+    if cfg!(target_os = "macos") || cfg!(target_os = "openbsd") || cfg!(target_os = "android") {
+        result
+            .stderr_contains(format!("ownership of 'a' retained as {user_name}"))
+            .stderr_contains(format!("ownership of 'a/b' retained as {user_name}"))
+            .stderr_contains(format!("ownership of 'a/b/c' retained as {user_name}"))
+            .stderr_does_not_contain(format!("ownership of 'a/b/c/d' retained as {user_name}"))
+            .stderr_does_not_contain(format!("ownership of 'a/b/c/d/b' retained as {user_name}"))
+            .stderr_does_not_contain(format!(
+                "ownership of 'a/b/c/d/b/c' retained as {user_name}"
+            ));
+    } else {
+        result
+            .success()
+            .stderr_contains(format!("ownership of 'a' retained as {user_name}"))
+            .stderr_contains(format!("ownership of 'a/b' retained as {user_name}"))
+            .stderr_contains(format!("ownership of 'a/b/c' retained as {user_name}"))
+            .stderr_contains(format!("ownership of 'a/b/c/d' retained as {user_name}"))
+            .stderr_does_not_contain(format!("ownership of 'a/b/c/d/b' retained as {user_name}"))
+            .stderr_does_not_contain(format!(
+                "ownership of 'a/b/c/d/b/c' retained as {user_name}"
+            ))
+            .stderr_does_not_contain(format!(
+                "ownership of 'a/b/c/d/b/c/d' retained as {user_name}"
+            ));
+    }
+}
+
+#[test]
+fn test_chown_symlink_two_links_same_dir() {
+    // Two symlinks pointing at the same directory is NOT a cycle: neither link is
+    // an ancestor of the other, so the target's contents must be visited through
+    // *both* links. This guards the backtracking in the linux safe-traversal
+    // cycle detection against false positives.
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let result = scene.cmd("whoami").run();
+    if skipping_test_is_okay(&result, "whoami: cannot find name for user ID") {
+        return;
+    }
+    let user_name = String::from(result.stdout_str().trim());
+    assert!(!user_name.is_empty());
+
+    // cSpell:disable
+    at.mkdir_all("base/realdir");
+    at.touch("base/realdir/file");
+    at.symlink_dir("base/realdir", "base/link1");
+    at.symlink_dir("base/realdir", "base/link2");
+
+    let result = scene.ucmd().arg("-vRL").arg(&user_name).arg("base").run();
+
+    // Only linux uses the safe-traversal cycle detection that this test exercises;
+    // other platforms fall back to walkdir with its own loop handling.
+    if cfg!(target_os = "linux") {
+        result
+            .success()
+            .stderr_contains(format!(
+                "ownership of 'base/realdir/file' retained as {user_name}"
+            ))
+            .stderr_contains(format!(
+                "ownership of 'base/link1/file' retained as {user_name}"
+            ))
+            .stderr_contains(format!(
+                "ownership of 'base/link2/file' retained as {user_name}"
+            ));
+    }
+    // cSpell:enable
+}