chown: detect symlink cycles and exit early just like GNU does it

Author: nikolalukovic

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};
 
@@ -449,13 +453,28 @@ 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: resolve through symlinks so a symlink-to-ancestor is caught.
+        if let Ok(info) = FileInformation::from_path(dir_path, true) {
+            if ancestors.contains(&info) {
+                return; // cycle detected, stop silently
+            }
+            ancestors.insert(info);
+        }
+
         // Read directory entries
         let entries = match dir_fd.read_dir() {
             Ok(entries) => entries,
@@ -539,7 +558,7 @@ impl ChownExecutor {
             if meta.is_dir() && (follow || !meta.file_type().is_symlink()) {
                 match dir_fd.open_subdir(&entry_name, SymlinkBehavior::Follow) {
                     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;
@@ -554,6 +573,11 @@ impl ChownExecutor {
                 }
             }
         }
+
+        // Backtrack: remove this directory so sibling subtrees are not falsely flagged.
+        if let Ok(info) = FileInformation::from_path(dir_path, true) {
+            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};
@@ -1448,17 +1448,29 @@ fn test_chmod_symlink_cycles() {
     at.mkdir_all("a/b/c");
     at.symlink_dir("a", "a/b/c/d");
 
-    scene
-        .ucmd()
-        .arg("-vRL")
-        .arg("+r")
-        .arg("a")
-        .succeeds()
-        .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)");
+    let result = scene.ucmd().arg("-vRL").arg("+r").arg("a").run();
+
+    if cfg!(target_os = "android") {
+        result
+            // cSpell:disable
+            .stdout_contains_line("mode of 'a' retained as 0700 (rwx------)")
+            .stdout_contains_line("mode of 'a/b' retained as 0700 (rwx------)")
+            .stdout_contains_line("mode of 'a/b/c' retained as 0700 (rwx------)")
+            .stdout_contains_line("mode of 'a/b/c/d' retained as 0700 (rwx------)")
+            .stdout_does_not_contain("mode of 'a/b/c/d/b' retained as 0700 (rwx------)")
+            .stdout_does_not_contain("mode of 'a/b/c/d/b/c' retained as 0700 (rwx------)")
+            .stdout_does_not_contain("mode of 'a/b/c/d/b/c/d' retained as 0700 (rwx------)");
+        // cSpell:enable
+    } else {
+        result
+            // 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
+    }
 }

tests/by-util/test_chown.rs

@@ -899,3 +899,47 @@ fn test_chown_reference_file() {
         .stderr_contains("ownership of 'b' retained as")
         .no_stdout();
 }
+
+#[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}"
+            ));
+    }
+}