From 21c8e42c8ffd8beaee535a081d03479346c54901 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 7 Aug 2025 23:09:45 +0200 Subject: [PATCH] chmod: fix recursive symlink handling for -H/-L/-P flags (Closes: #8422) --- src/uu/chmod/src/chmod.rs | 78 +++++++++++++--- tests/by-util/test_chmod.rs | 172 ++++++++++++++++++++++++++++++++++-- 2 files changed, 228 insertions(+), 22 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 99e0657d3c3..65c2df07cf4 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -311,7 +311,7 @@ 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); } @@ -319,31 +319,62 @@ impl Chmoder { 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 @@ -351,17 +382,23 @@ impl Chmoder { // 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( diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 8386c4d32f7..ea5d3f898a6 100644 --- a/tests/by-util/test_chmod.rs +++ b/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 + ); +}