diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index c05288e21..d2eb22ce6 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/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::(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, cmode: Option, + 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 => { diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 73b84be72..62e7d56ed 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/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 { + 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>(&self, path: P, follow: bool) -> Option { let path = path.as_ref(); - let meta = if follow { - path.metadata() - } else { - path.symlink_metadata() - }; + + let meta = get_metadata(path, follow); + match meta { Err(e) => { match self.verbosity.level { @@ -516,6 +522,45 @@ pub struct GidUidOwnerFilter { } type GidUidFilterOwnerParser = fn(&ArgMatches) -> UResult; +/// 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> { + 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 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 (recursive, dereference, traverse_symlinks) = configure_symlink_and_recursion(&matches)?; 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, diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 167e79cd0..6f508afd6 100644 --- a/tests/by-util/test_chmod.rs +++ b/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("chmod: file: new permissions are r-xrwxrwx, not r-xr-xr-x\n"); + .stderr_is( + // 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 + ); + } +}