Merge pull request #7025 from sylvestre/chmod-L-2

chmod: add support for the deref and links options

src/uu/chmod/src/chmod.rs

@@ -16,6 +16,7 @@ use uucore::fs::display_permissions_unix;
 use uucore::libc::mode_t;
 #[cfg(not(windows))]
 use uucore::mode;
+use uucore::perms::{configure_symlink_and_recursion, TraverseSymlinks};
 use uucore::{format_usage, help_about, help_section, help_usage, show, show_error};
 
 const ABOUT: &str = help_about!("chmod.md");
@@ -99,7 +100,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let quiet = matches.get_flag(options::QUIET);
     let verbose = matches.get_flag(options::VERBOSE);
     let preserve_root = matches.get_flag(options::PRESERVE_ROOT);
-    let recursive = matches.get_flag(options::RECURSIVE);
     let fmode = match matches.get_one::<String>(options::REFERENCE) {
         Some(fref) => match fs::metadata(fref) {
             Ok(meta) => Some(meta.mode() & 0o7777),
@@ -138,6 +138,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         return Err(UUsageError::new(1, "missing operand".to_string()));
     }
 
+    let (recursive, dereference, traverse_symlinks) = configure_symlink_and_recursion(&matches)?;
+
     let chmoder = Chmoder {
         changes,
         quiet,
@@ -146,6 +148,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         recursive,
         fmode,
         cmode,
+        traverse_symlinks,
+        dereference,
     };
 
     chmoder.chmod(&files)
@@ -237,6 +241,8 @@ struct Chmoder {
     recursive: bool,
     fmode: Option<u32>,
     cmode: Option<String>,
+    traverse_symlinks: TraverseSymlinks,
+    dereference: bool,
 }
 
 impl Chmoder {
@@ -248,12 +254,19 @@ impl Chmoder {
             let file = Path::new(filename);
             if !file.exists() {
                 if file.is_symlink() {
+                    if !self.dereference && !self.recursive {
+                        // The file is a symlink and we should not follow it
+                        // Don't try to change the mode of the symlink itself
+                        continue;
+                    }
                     if !self.quiet {
                         show!(USimpleError::new(
                             1,
                             format!("cannot operate on dangling symlink {}", filename.quote()),
                         ));
+                        set_exit_code(1);
                     }
+
                     if self.verbose {
                         println!(
                             "failed to change mode of {} from 0000 (---------) to 1500 (r-x-----T)",
@@ -273,6 +286,11 @@ impl Chmoder {
                 // So we set the exit code, because it hasn't been set yet if `self.quiet` is true.
                 set_exit_code(1);
                 continue;
+            } else if !self.dereference && file.is_symlink() {
+                // The file is a symlink and we should not follow it
+                // chmod 755 --no-dereference a/link
+                // should not change the permissions in this case
+                continue;
             }
             if self.recursive && self.preserve_root && filename == "/" {
                 return Err(USimpleError::new(
@@ -294,11 +312,23 @@ impl Chmoder {
 
     fn walk_dir(&self, file_path: &Path) -> UResult<()> {
         let mut r = self.chmod_file(file_path);
-        if !file_path.is_symlink() && file_path.is_dir() {
+        // Determine whether to traverse symlinks based on `self.traverse_symlinks`
+        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::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);
                 }
             }
         }
@@ -314,19 +344,22 @@ impl Chmoder {
     }
     #[cfg(unix)]
     fn chmod_file(&self, file: &Path) -> UResult<()> {
-        use uucore::mode::get_umask;
+        use uucore::{mode::get_umask, perms::get_metadata};
 
-        let fperm = match fs::metadata(file) {
+        let metadata = get_metadata(file, self.dereference);
+
+        let fperm = match metadata {
             Ok(meta) => meta.mode() & 0o7777,
             Err(err) => {
-                if file.is_symlink() {
+                // Handle dangling symlinks or other errors
+                if file.is_symlink() && !self.dereference {
                     if self.verbose {
                         println!(
                             "neither symbolic link {} nor referent has been changed",
                             file.quote()
                         );
                     }
-                    return Ok(());
+                    return Ok(()); // Skip dangling symlinks
                 } else if err.kind() == std::io::ErrorKind::PermissionDenied {
                     // These two filenames would normally be conditionally
                     // quoted, but GNU's tests expect them to always be quoted
@@ -339,6 +372,8 @@ impl Chmoder {
                 }
             }
         };
+
+        // Determine the new permissions to apply
         match self.fmode {
             Some(mode) => self.change_file(fperm, mode, file)?,
             None => {

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

@@ -250,6 +250,14 @@ fn is_root(path: &Path, would_traverse_symlink: bool) -> bool {
     false
 }
 
+pub fn get_metadata(file: &Path, follow: bool) -> Result<Metadata, std::io::Error> {
+    if follow {
+        file.metadata()
+    } else {
+        file.symlink_metadata()
+    }
+}
+
 impl ChownExecutor {
     pub fn exec(&self) -> UResult<()> {
         let mut ret = 0;
@@ -417,11 +425,9 @@ impl ChownExecutor {
 
     fn obtain_meta<P: AsRef<Path>>(&self, path: P, follow: bool) -> Option<Metadata> {
         let path = path.as_ref();
-        let meta = if follow {
+
-            path.metadata()
+        let meta = get_metadata(path, follow);
-        } else {
+
-            path.symlink_metadata()
-        };
         match meta {
             Err(e) => {
                 match self.verbosity.level {
@@ -516,6 +522,45 @@ pub struct GidUidOwnerFilter {
 }
 type GidUidFilterOwnerParser = fn(&ArgMatches) -> UResult<GidUidOwnerFilter>;
 
+/// Determines symbolic link traversal and recursion settings based on flags.
+/// Returns the updated `dereference` and `traverse_symlinks` values.
+pub fn configure_symlink_and_recursion(
+    matches: &ArgMatches,
+) -> Result<(bool, bool, TraverseSymlinks), Box<dyn crate::error::UError>> {
+    let mut dereference = if matches.get_flag(options::dereference::DEREFERENCE) {
+        Some(true) // Follow symlinks
+    } else if matches.get_flag(options::dereference::NO_DEREFERENCE) {
+        Some(false) // Do not follow symlinks
+    } else {
+        None // Default behavior
+    };
+
+    let mut traverse_symlinks = if matches.get_flag("L") {
+        TraverseSymlinks::All
+    } else if matches.get_flag("H") {
+        TraverseSymlinks::First
+    } else {
+        TraverseSymlinks::None
+    };
+
+    let recursive = matches.get_flag(options::RECURSIVE);
+    if recursive {
+        if traverse_symlinks == TraverseSymlinks::None {
+            if dereference == Some(true) {
+                return Err(USimpleError::new(
+                    1,
+                    "-R --dereference requires -H or -L".to_string(),
+                ));
+            }
+            dereference = Some(false);
+        }
+    } else {
+        traverse_symlinks = TraverseSymlinks::None;
+    }
+
+    Ok((recursive, dereference.unwrap_or(true), traverse_symlinks))
+}
+
 /// Base implementation for `chgrp` and `chown`.
 ///
 /// An argument called `add_arg_if_not_reference` will be added to `command` if
@@ -571,34 +616,7 @@ pub fn chown_base(
         .unwrap_or_default();
 
     let preserve_root = matches.get_flag(options::preserve_root::PRESERVE);
-
+    let (recursive, dereference, traverse_symlinks) = configure_symlink_and_recursion(&matches)?;
-    let mut dereference = if matches.get_flag(options::dereference::DEREFERENCE) {
-        Some(true)
-    } else if matches.get_flag(options::dereference::NO_DEREFERENCE) {
-        Some(false)
-    } else {
-        None
-    };
-
-    let mut traverse_symlinks = if matches.get_flag(options::traverse::TRAVERSE) {
-        TraverseSymlinks::First
-    } else if matches.get_flag(options::traverse::EVERY) {
-        TraverseSymlinks::All
-    } else {
-        TraverseSymlinks::None
-    };
-
-    let recursive = matches.get_flag(options::RECURSIVE);
-    if recursive {
-        if traverse_symlinks == TraverseSymlinks::None {
-            if dereference == Some(true) {
-                return Err(USimpleError::new(1, "-R --dereference requires -H or -L"));
-            }
-            dereference = Some(false);
-        }
-    } else {
-        traverse_symlinks = TraverseSymlinks::None;
-    }
 
     let verbosity_level = if matches.get_flag(options::verbosity::CHANGES) {
         VerbosityLevel::Changes
@@ -628,7 +646,7 @@ pub fn chown_base(
             level: verbosity_level,
         },
         recursive,
-        dereference: dereference.unwrap_or(true),
+        dereference,
         preserve_root,
         files,
         filter,

tests/by-util/test_chmod.rs

@@ -229,19 +229,52 @@ fn test_chmod_ugoa() {
         },
     ];
     run_tests(tests);
+}
 
+#[test]
+#[cfg(any(target_os = "linux", target_os = "macos"))]
+// TODO fix android, it has 0777
+// We should force for the umask on startup
+fn test_chmod_umask_expected() {
+    let current_umask = uucore::mode::get_umask();
+    assert_eq!(
+        current_umask,
+        0o022,
+        "Unexpected umask value: expected 022 (octal), but got {:03o}. Please adjust the test environment.",
+        current_umask
+    );
+}
+
+fn get_expected_symlink_permissions() -> u32 {
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    {
+        0o120_777
+    }
+    #[cfg(not(any(target_os = "linux", target_os = "android")))]
+    {
+        0o120_755
+    }
+}
+
+#[test]
+fn test_chmod_error_permissions() {
     // check that we print an error if umask prevents us from removing a permission
     let (at, mut ucmd) = at_and_ucmd!();
+
     at.touch("file");
     set_permissions(at.plus("file"), Permissions::from_mode(0o777)).unwrap();
+
     ucmd.args(&["-w", "file"])
+        .umask(0o022)
         .fails()
         .code_is(1)
-        // spell-checker:disable-next-line
+        .stderr_is(
-        .stderr_is("chmod: file: new permissions are r-xrwxrwx, not r-xr-xr-x\n");
+            // spell-checker:disable-next-line
+            "chmod: file: new permissions are r-xrwxrwx, not r-xr-xr-x\n",
+        );
     assert_eq!(
         metadata(at.plus("file")).unwrap().permissions().mode(),
-        0o100577
+        0o100_577
     );
 }
 
@@ -642,7 +675,10 @@ fn test_chmod_file_symlink_after_non_existing_file() {
         .stderr_contains(expected_stderr);
     assert_eq!(
         at.metadata(test_existing_symlink).permissions().mode(),
-        0o100_764
+        0o100_764,
+        "Expected mode: {:o}, but got: {:o}",
+        0o100_764,
+        at.metadata(test_existing_symlink).permissions().mode()
     );
 }
 
@@ -746,3 +782,192 @@ fn test_gnu_special_options() {
     scene.ucmd().arg("--").arg("--").arg("file").succeeds();
     scene.ucmd().arg("--").arg("--").fails();
 }
+
+#[test]
+fn test_chmod_dereference_symlink() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let target = "file";
+    let symlink = "symlink";
+
+    at.touch(target);
+    set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
+    at.symlink_file(target, symlink);
+
+    // Use --dereference: should modify the target file's permissions
+    scene
+        .ucmd()
+        .arg("--dereference")
+        .arg("u+x")
+        .arg(symlink)
+        .succeeds()
+        .no_stderr();
+    assert_eq!(at.metadata(target).permissions().mode(), 0o100_764);
+    assert_eq!(
+        at.symlink_metadata(symlink).permissions().mode(),
+        get_expected_symlink_permissions()
+    );
+}
+
+#[test]
+fn test_chmod_no_dereference_symlink() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let target = "file";
+    let symlink = "symlink";
+
+    at.touch(target);
+    set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
+    at.symlink_file(target, symlink);
+
+    scene
+        .ucmd()
+        .arg("--no-dereference")
+        .arg("u+x")
+        .arg(symlink)
+        .succeeds()
+        .no_stderr();
+    assert_eq!(at.metadata(target).permissions().mode(), 0o100_664);
+    assert_eq!(
+        at.symlink_metadata(symlink).permissions().mode(),
+        get_expected_symlink_permissions()
+    );
+}
+
+#[test]
+fn test_chmod_symlink_to_dangling_target_dereference() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let dangling_target = "nonexistent_file";
+    let symlink = "symlink";
+
+    at.symlink_file(dangling_target, symlink);
+
+    // Use --dereference: should fail due to dangling symlink
+    scene
+        .ucmd()
+        .arg("--dereference")
+        .arg("u+x")
+        .arg(symlink)
+        .fails()
+        .stderr_contains(format!("cannot operate on dangling symlink '{}'", symlink));
+}
+
+#[test]
+fn test_chmod_symlink_target_no_dereference() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file = "a";
+    let symlink = "symlink";
+    at.touch(file);
+    at.symlink_file(file, symlink);
+    set_permissions(at.plus(file), Permissions::from_mode(0o644)).unwrap();
+
+    scene
+        .ucmd()
+        .arg("--no-dereference")
+        .arg("755")
+        .arg(symlink)
+        .succeeds()
+        .no_stderr();
+    assert_eq!(
+        at.symlink_metadata(file).permissions().mode(),
+        0o100_644,
+        "Expected symlink permissions: {:o}, but got: {:o}",
+        0o100_644,
+        at.symlink_metadata(file).permissions().mode()
+    );
+}
+
+#[test]
+fn test_chmod_symlink_to_dangling_recursive() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let dangling_target = "nonexistent_file";
+    let symlink = "symlink";
+
+    at.symlink_file(dangling_target, symlink);
+
+    scene
+        .ucmd()
+        .arg("755")
+        .arg("-R")
+        .arg(symlink)
+        .fails()
+        .stderr_is("chmod: cannot operate on dangling symlink 'symlink'\n");
+    assert_eq!(
+        at.symlink_metadata(symlink).permissions().mode(),
+        get_expected_symlink_permissions(),
+        "Expected symlink permissions: {:o}, but got: {:o}",
+        get_expected_symlink_permissions(),
+        at.symlink_metadata(symlink).permissions().mode()
+    );
+}
+
+#[test]
+fn test_chmod_traverse_symlink_combo() {
+    let scenarios = [
+        (
+            vec!["-R", "-H"],
+            0o100_664,
+            get_expected_symlink_permissions(),
+        ),
+        (
+            vec!["-R", "-L"],
+            0o100_764,
+            get_expected_symlink_permissions(),
+        ),
+        (
+            vec!["-R", "-P"],
+            0o100_664,
+            get_expected_symlink_permissions(),
+        ),
+    ];
+
+    for (flags, expected_target_perms, expected_symlink_perms) in scenarios {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+
+        let directory = "dir";
+        let target = "file";
+        let symlink = "symlink";
+
+        at.mkdir(directory);
+        at.touch(target);
+        at.symlink_file(target, &format!("{directory}/{symlink}"));
+
+        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")
+            .umask(0o022)
+            .arg(directory)
+            .succeeds()
+            .no_stderr();
+
+        let actual_target = at.metadata(target).permissions().mode();
+        assert_eq!(
+            actual_target, expected_target_perms,
+            "For flags {:?}, expected target perms = {:o}, got = {:o}",
+            flags, expected_target_perms, actual_target
+        );
+
+        let actual_symlink = at
+            .symlink_metadata(&format!("{directory}/{symlink}"))
+            .permissions()
+            .mode();
+        assert_eq!(
+            actual_symlink, expected_symlink_perms,
+            "For flags {:?}, expected symlink perms = {:o}, got = {:o}",
+            flags, expected_symlink_perms, actual_symlink
+        );
+    }
+}