Merge pull request #8450 from sylvestre/fix-chmod-symlink-handling

chmod: fix recursive symlink handling for -H/-L/-P flags (Closes: #8422)

Author: cakebaker

src/uu/chmod/src/chmod.rs

@@ -311,57 +311,94 @@ impl Chmoder {
                 return Err(ChmodError::PreserveRoot(filename.to_string()).into());
             }
             if self.recursive {
-                r = self.walk_dir(file);
+                r = self.walk_dir_with_context(file, true);
             } else {
                 r = self.chmod_file(file).and(r);
             }
         }
         r
     }
 
-    fn walk_dir(&self, file_path: &Path) -> UResult<()> {
+    fn walk_dir_with_context(&self, file_path: &Path, is_command_line_arg: bool) -> UResult<()> {
         let mut r = self.chmod_file(file_path);
-        // Determine whether to traverse symlinks based on `self.traverse_symlinks`
+
+        // Determine whether to traverse symlinks based on context and traversal mode
         let should_follow_symlink = match self.traverse_symlinks {
             TraverseSymlinks::All => true,
-            TraverseSymlinks::First => {
-                file_path == file_path.canonicalize().unwrap_or(file_path.to_path_buf())
-            }
+            TraverseSymlinks::First => is_command_line_arg, // Only follow symlinks that are command line args
             TraverseSymlinks::None => false,
         };
 
         // 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() {
             for dir_entry in file_path.read_dir()? {
-                let path = dir_entry?.path();
-                if !path.is_symlink() {
-                    r = self.walk_dir(path.as_path());
-                } else if should_follow_symlink {
-                    r = self.chmod_file(path.as_path()).and(r);
+                let path = match dir_entry {
+                    Ok(entry) => entry.path(),
+                    Err(err) => {
+                        r = r.and(Err(err.into()));
+                        continue;
+                    }
+                };
+                if path.is_symlink() {
+                    r = self.handle_symlink_during_recursion(&path).and(r);
+                } else {
+                    r = self.walk_dir_with_context(path.as_path(), false).and(r);
                 }
             }
         }
         r
     }
 
+    fn handle_symlink_during_recursion(&self, path: &Path) -> UResult<()> {
+        // During recursion, determine behavior based on traversal mode
+        match self.traverse_symlinks {
+            TraverseSymlinks::All => {
+                // Follow all symlinks during recursion
+                // 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(_) => {
+                        // It's a file symlink, chmod it
+                        self.chmod_file(path)
+                    }
+                    Err(_) => {
+                        // Dangling symlink, chmod it without dereferencing
+                        self.chmod_file_internal(path, false)
+                    }
+                }
+            }
+            TraverseSymlinks::First | TraverseSymlinks::None => {
+                // Don't follow symlinks encountered during recursion
+                // For these symlinks, don't dereference them even if dereference is normally true
+                self.chmod_file_internal(path, false)
+            }
+        }
+    }
+
     #[cfg(windows)]
     fn chmod_file(&self, file: &Path) -> UResult<()> {
         // chmod is useless on Windows
         // it doesn't set any permissions at all
         // instead it just sets the readonly attribute on the file
         Ok(())
     }
+
     #[cfg(unix)]
     fn chmod_file(&self, file: &Path) -> UResult<()> {
+        self.chmod_file_internal(file, self.dereference)
+    }
+
+    #[cfg(unix)]
+    fn chmod_file_internal(&self, file: &Path, dereference: bool) -> UResult<()> {
         use uucore::{mode::get_umask, perms::get_metadata};
 
-        let metadata = get_metadata(file, self.dereference);
+        let metadata = get_metadata(file, dereference);
 
         let fperm = match metadata {
             Ok(meta) => meta.mode() & 0o7777,
             Err(err) => {
                 // Handle dangling symlinks or other errors
-                return if file.is_symlink() && !self.dereference {
+                return if file.is_symlink() && !dereference {
                     if self.verbose {
                         println!(
                             "neither symbolic link {} nor referent has been changed",
@@ -418,7 +455,20 @@ impl Chmoder {
                     }
                 }
 
-                self.change_file(fperm, new_mode, file)?;
+                // Special handling for symlinks when not dereferencing
+                if file.is_symlink() && !dereference {
+                    // TODO: On most Unix systems, symlink permissions are ignored by the kernel,
+                    // so changing them has no effect. We skip this operation for compatibility.
+                    // Note that "chmod without dereferencing" effectively does nothing on symlinks.
+                    if self.verbose {
+                        println!(
+                            "neither symbolic link {} nor referent has been changed",
+                            file.quote()
+                        );
+                    }
+                } else {
+                    self.change_file(fperm, new_mode, file)?;
+                }
                 // if a permission would have been removed if umask was 0, but it wasn't because umask was not 0, print an error and fail
                 if (new_mode & !naively_expected_new_mode) != 0 {
                     return Err(ChmodError::NewPermissions(

tests/by-util/test_chmod.rs

@@ -944,10 +944,8 @@ fn test_chmod_dangling_symlink_recursive_combos() {
         at.symlink_file(dangling_target, symlink);
 
         let mut ucmd = scene.ucmd();
-        for f in &flags {
-            ucmd.arg(f);
-        }
-        ucmd.arg("u+x")
+        ucmd.args(&flags)
+            .arg("u+x")
             .umask(0o022)
             .arg(symlink)
             .fails()
@@ -1002,10 +1000,8 @@ fn test_chmod_traverse_symlink_combo() {
         set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
 
         let mut ucmd = scene.ucmd();
-        for f in &flags {
-            ucmd.arg(f);
-        }
-        ucmd.arg("u+x")
+        ucmd.args(&flags)
+            .arg("u+x")
             .umask(0o022)
             .arg(directory)
             .succeeds()
@@ -1027,3 +1023,163 @@ fn test_chmod_traverse_symlink_combo() {
         );
     }
 }
+
+#[test]
+fn test_chmod_recursive_symlink_to_directory_command_line() {
+    // Test behavior when the symlink itself is a command-line argument
+    let scenarios = [
+        (vec!["-R"], true), // Default behavior (-H): follow symlinks that are command line args
+        (vec!["-R", "-H"], true), // Explicit -H: follow symlinks that are command line args
+        (vec!["-R", "-L"], true), // -L: follow all symlinks
+        (vec!["-R", "-P"], false), // -P: never follow symlinks
+    ];
+
+    for (flags, should_follow_symlink_dir) in scenarios {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+
+        let target_dir = "target_dir";
+        let symlink_to_dir = "link_dir";
+        let file_in_target = "file_in_target";
+
+        at.mkdir(target_dir);
+        at.touch(format!("{target_dir}/{file_in_target}"));
+        at.symlink_dir(target_dir, symlink_to_dir);
+
+        set_permissions(
+            at.plus(format!("{target_dir}/{file_in_target}")),
+            Permissions::from_mode(0o644),
+        )
+        .unwrap();
+
+        let mut ucmd = scene.ucmd();
+        ucmd.args(&flags)
+            .arg("go-rwx")
+            .arg(symlink_to_dir) // The symlink itself is the command-line argument
+            .succeeds()
+            .no_stderr();
+
+        let actual_file_perms = at
+            .metadata(&format!("{target_dir}/{file_in_target}"))
+            .permissions()
+            .mode();
+
+        if should_follow_symlink_dir {
+            // When following symlinks, the file inside the target directory should have its permissions changed
+            assert_eq!(
+                actual_file_perms, 0o100_600,
+                "For flags {flags:?}, expected file perms when following symlinks = 600, got = {actual_file_perms:o}",
+            );
+        } else {
+            // When not following symlinks, the file inside the target directory should be unchanged
+            assert_eq!(
+                actual_file_perms, 0o100_644,
+                "For flags {flags:?}, expected file perms when not following symlinks = 644, got = {actual_file_perms:o}",
+            );
+        }
+    }
+}
+
+#[test]
+fn test_chmod_recursive_symlink_during_traversal() {
+    // Test behavior when symlinks are encountered during directory traversal
+    let scenarios = [
+        (vec!["-R"], false), // Default behavior (-H): don't follow symlinks encountered during traversal
+        (vec!["-R", "-H"], false), // Explicit -H: don't follow symlinks encountered during traversal
+        (vec!["-R", "-L"], true),  // -L: follow all symlinks including those found during traversal
+        (vec!["-R", "-P"], false), // -P: never follow symlinks
+    ];
+
+    for (flags, should_follow_symlink_dir) in scenarios {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+
+        let directory = "dir";
+        let target_dir = "target_dir";
+        let symlink_to_dir = "link_dir";
+        let file_in_target = "file_in_target";
+
+        at.mkdir(directory);
+        at.mkdir(target_dir);
+        at.touch(format!("{target_dir}/{file_in_target}"));
+        at.symlink_dir(target_dir, &format!("{directory}/{symlink_to_dir}"));
+
+        set_permissions(
+            at.plus(format!("{target_dir}/{file_in_target}")),
+            Permissions::from_mode(0o644),
+        )
+        .unwrap();
+
+        let mut ucmd = scene.ucmd();
+        ucmd.args(&flags)
+            .arg("go-rwx")
+            .arg(directory) // The directory is the command-line argument
+            .succeeds()
+            .no_stderr();
+
+        let actual_file_perms = at
+            .metadata(&format!("{target_dir}/{file_in_target}"))
+            .permissions()
+            .mode();
+
+        if should_follow_symlink_dir {
+            // When following symlinks, the file inside the target directory should have its permissions changed
+            assert_eq!(
+                actual_file_perms, 0o100_600,
+                "For flags {flags:?}, expected file perms when following symlinks = 600, got = {actual_file_perms:o}",
+            );
+        } else {
+            // When not following symlinks, the file inside the target directory should be unchanged
+            assert_eq!(
+                actual_file_perms, 0o100_644,
+                "For flags {flags:?}, expected file perms when not following symlinks = 644, got = {actual_file_perms:o}",
+            );
+        }
+    }
+}
+
+#[test]
+fn test_chmod_recursive_symlink_combinations() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let directory = "dir";
+    let target_dir = "target_dir";
+    let target_file = "target_file";
+    let symlink_to_dir = "link_dir";
+    let symlink_to_file = "link_file";
+    let file_in_target = "file";
+
+    at.mkdir(directory);
+    at.mkdir(target_dir);
+    at.touch(target_file);
+    at.touch(format!("{target_dir}/{file_in_target}"));
+    at.symlink_dir(target_dir, &format!("{directory}/{symlink_to_dir}"));
+    at.symlink_file(target_file, &format!("{directory}/{symlink_to_file}"));
+
+    set_permissions(at.plus(target_file), Permissions::from_mode(0o644)).unwrap();
+    set_permissions(
+        at.plus(format!("{target_dir}/{file_in_target}")),
+        Permissions::from_mode(0o644),
+    )
+    .unwrap();
+
+    // Test with -R -L (follow all symlinks)
+    scene
+        .ucmd()
+        .arg("-R")
+        .arg("-L")
+        .arg("go-rwx")
+        .arg(directory)
+        .succeeds()
+        .no_stderr();
+
+    // Both target file and file in target directory should have permissions changed
+    assert_eq!(at.metadata(target_file).permissions().mode(), 0o100_600);
+    assert_eq!(
+        at.metadata(&format!("{target_dir}/{file_in_target}"))
+            .permissions()
+            .mode(),
+        0o100_600
+    );
+}