From c8d5723e3a68456984f942395e834d34fdf89f42 Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Wed, 7 Oct 2026 22:41:27 +0200 Subject: [PATCH 1/4] chmod: report every failure met under -R Failures met while walking a tree were returned up through `Result::and`, so only one of them reached stderr, after all the other output, and `-f` could not hide it. An unreadable directory was also reported as "cannot access", where GNU says "cannot read directory", and on Hurd the `read_dir` error came back without its path. Report each failure where it happens and keep the exit status, let `-f` hide the messages, report an unreadable directory as "cannot read directory", and with `-v` add the "could not be accessed" line that GNU prints for it. --- src/uu/chmod/locales/en-US.ftl | 2 + src/uu/chmod/locales/fr-FR.ftl | 2 + src/uu/chmod/src/chmod.rs | 178 +++++++++++++++++++-------------- tests/by-util/test_chmod.rs | 79 +++++++++++++-- 4 files changed, 176 insertions(+), 85 deletions(-) diff --git a/src/uu/chmod/locales/en-US.ftl b/src/uu/chmod/locales/en-US.ftl index 2f3f6748461..79eeac99d6b 100644 --- a/src/uu/chmod/locales/en-US.ftl +++ b/src/uu/chmod/locales/en-US.ftl @@ -12,6 +12,7 @@ chmod-error-preserve-root = it is dangerous to operate recursively on {$file} chmod-error-preserve-root-same-as = it is dangerous to operate recursively on {$file} (same as '/') chmod: use --no-preserve-root to override this failsafe chmod-error-permission-denied = cannot access {$file}: Permission denied +chmod-error-cannot-read-directory = cannot read directory {$file}: {$err} chmod-error-new-permissions = {$file}: new permissions are {$actual}, not {$expected} chmod-error-changing-permissions = changing permissions of {$file}: {$err} chmod-error-missing-operand = missing operand @@ -32,3 +33,4 @@ chmod-verbose-neither-changed = neither symbolic link {$file} nor referent has b chmod-verbose-mode-retained = mode of {$file} retained as {$mode_octal} ({$mode_display}) chmod-verbose-failed-change = failed to change mode of file {$file} from {$old_mode} ({$old_mode_display}) to {$new_mode} ({$new_mode_display}) chmod-verbose-mode-changed = mode of {$file} changed from {$old_mode} ({$old_mode_display}) to {$new_mode} ({$new_mode_display}) +chmod-verbose-could-not-be-accessed = {$file} could not be accessed diff --git a/src/uu/chmod/locales/fr-FR.ftl b/src/uu/chmod/locales/fr-FR.ftl index e8116f08f9c..4d88a5fbb99 100644 --- a/src/uu/chmod/locales/fr-FR.ftl +++ b/src/uu/chmod/locales/fr-FR.ftl @@ -24,6 +24,7 @@ chmod-error-preserve-root = il est dangereux d'opérer récursivement sur {$file chmod-error-preserve-root-same-as = il est dangereux d'opérer récursivement sur {$file} (identique à '/') chmod: utiliser --no-preserve-root pour outrepasser cette protection chmod-error-permission-denied = impossible d'accéder à {$file} : Permission refusée +chmod-error-cannot-read-directory = impossible de lire le répertoire {$file} : {$err} chmod-error-new-permissions = {$file} : les nouvelles permissions sont {$actual}, pas {$expected} chmod-error-changing-permissions = changement des permissions de {$file} : {$err} chmod-error-missing-operand = opérande manquant @@ -34,3 +35,4 @@ chmod-verbose-neither-changed = ni le lien symbolique {$file} ni la référence chmod-verbose-mode-retained = mode de {$file} conservé comme {$mode_octal} ({$mode_display}) chmod-verbose-failed-change = échec du changement de mode du fichier {$file} de {$old_mode} ({$old_mode_display}) vers {$new_mode} ({$new_mode_display}) chmod-verbose-mode-changed = mode de {$file} changé de {$old_mode} ({$old_mode_display}) vers {$new_mode} ({$new_mode_display}) +chmod-verbose-could-not-be-accessed = {$file} n'a pas pu être consulté diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index f586ef8a681..606c0cffeec 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -41,6 +41,8 @@ enum ChmodError { PreserveRootSameAs(PathBuf), #[error("{}", translate!("chmod-error-permission-denied", "file" => _0.quote()))] PermissionDenied(PathBuf), + #[error("{}", translate!("chmod-error-cannot-read-directory", "file" => _0.quote(), "err" => strip_errno(_1)))] + CannotReadDirectory(PathBuf, std::io::Error), #[error("{}", translate!("chmod-error-new-permissions", "file" => _0.maybe_quote(), "actual" => _1, "expected" => _2))] NewPermissions(PathBuf, String, String), #[error("{}", translate!("chmod-error-changing-permissions", "file" => _0.quote(), "err" => strip_errno(_1)))] @@ -416,6 +418,34 @@ impl Chmoder { } } + /// Report a failure met while walking a tree under `-R`. + /// + /// The walk goes on after a failure, so each one is reported where it + /// happens rather than returned, which would keep only one of them. `-f` + /// hides the message; the failure still decides the exit status, as in + /// GNU. An error whose message was already printed (`change_file`) has + /// nothing left to show. + fn report_walk_failure(&self, error: Box) { + let message = error.to_string(); + if !self.quiet && !message.is_empty() { + show_error!("{message}"); + } + set_exit_code(error.code()); + } + + /// Report a directory whose entries cannot be listed. Its own mode was + /// changed before the attempt, so with `-v` GNU also says that the + /// subtree was left alone. + fn report_unreadable_dir(&self, path: &Path, err: std::io::Error) { + self.report_walk_failure(ChmodError::CannotReadDirectory(path.into(), err).into()); + if self.verbose { + println!( + "{}", + translate!("chmod-verbose-could-not-be-accessed", "file" => path.quote()) + ); + } + } + fn print_neither_changed(file: OsString) -> std::io::Result<()> { use std::io::{Write as _, stdout}; writeln!( @@ -497,9 +527,7 @@ impl Chmoder { } if self.recursive { let mut ancestors = HashSet::new(); - r = self - .walk_dir_with_context(file, true, &mut ancestors) - .and(r); + self.walk_dir_with_context(file, true, &mut ancestors); } else { r = self.chmod_file(file).and(r); } @@ -527,15 +555,17 @@ impl Chmoder { file_path: &Path, is_command_line_arg: bool, ancestors: &mut HashSet, - ) -> UResult<()> { + ) { // Skip (and diagnose) an entry that is '/' (a symlink to it, or a bind // mount) before touching it. if self.descends_into_root(file_path) { show!(ChmodError::PreserveRootSameAs(file_path.into())); - return Ok(()); + return; } - let mut r = self.chmod_file(file_path); + if let Err(err) = self.chmod_file(file_path) { + self.report_walk_failure(err); + } // Determine whether to traverse symlinks based on context and traversal mode let should_follow_symlink = match self.traverse_symlinks { @@ -546,6 +576,14 @@ impl Chmoder { // 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() { + let entries = match file_path.read_dir() { + Ok(entries) => entries, + Err(err) => { + self.report_unreadable_dir(file_path, err); + return; + } + }; + // Cycle detection: identify this directory by its target's inode (resolved // via dereference=true) once, and reuse it for the backtrack below so we // don't stat the same directory twice. If it's already on the current path, @@ -554,22 +592,20 @@ impl Chmoder { if let Some(info) = &dir_info && !ancestors.insert(info.clone()) { - return r; + return; } // We buffer all paths in this dir to not keep too many fd's open during recursion let mut paths_in_this_dir = Vec::new(); - for dir_entry in file_path.read_dir()? { + for dir_entry in entries { match dir_entry { Ok(entry) => paths_in_this_dir.push(entry.path()), - Err(err) => r = r.and(Err(err.into())), + Err(err) => self.report_walk_failure(err.into()), } } for path in paths_in_this_dir { - r = self - .walk_dir_with_context(path.as_path(), false, ancestors) - .and(r); + self.walk_dir_with_context(path.as_path(), false, ancestors); } // Backtrack: remove this directory from ancestors so sibling subtrees @@ -578,7 +614,6 @@ impl Chmoder { ancestors.remove(&info); } } - r } #[cfg(not(target_os = "redox"))] @@ -587,15 +622,17 @@ impl Chmoder { file_path: &Path, is_command_line_arg: bool, ancestors: &mut HashSet, - ) -> UResult<()> { + ) { // Skip (and diagnose) an entry that is '/' (a symlink to it, or a bind // mount) before touching it. if self.descends_into_root(file_path) { show!(ChmodError::PreserveRootSameAs(file_path.into())); - return Ok(()); + return; } - let mut r = self.chmod_file(file_path); + if let Err(err) = self.chmod_file(file_path) { + self.report_walk_failure(err); + } // Determine whether to traverse symlinks based on context and traversal mode let should_follow_symlink = match self.traverse_symlinks { @@ -610,20 +647,11 @@ impl Chmoder { // swapped-in symlink. `-H`/`-L` still follow, which is what they ask for. if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { match DirFd::open(file_path, should_follow_symlink.into()) { - Ok(dir_fd) => { - r = self.safe_traverse_dir(&dir_fd, file_path, ancestors).and(r); - } - Err(err) => { - // Handle permission denied errors with proper file path context - if err.kind() == std::io::ErrorKind::PermissionDenied { - r = r.and(Err(ChmodError::PermissionDenied(file_path.into()).into())); - } else { - r = r.and(Err(err.into())); - } - } + Ok(dir_fd) => self.safe_traverse_dir(&dir_fd, file_path, ancestors), + // A directory that does not open cannot be read. + Err(err) => self.report_unreadable_dir(file_path, err), } } - r } #[cfg(not(target_os = "redox"))] @@ -632,8 +660,17 @@ impl Chmoder { dir_fd: &DirFd, dir_path: &Path, ancestors: &mut HashSet, - ) -> UResult<()> { - let mut r = Ok(()); + ) { + // A directory that opens but cannot be listed (read permission without + // search permission, on some platforms) is reported like one that does + // not open. + let entries = match dir_fd.read_dir() { + Ok(entries) => entries, + Err(err) => { + self.report_unreadable_dir(dir_path, err); + return; + } + }; // Cycle detection: identify this directory by (dev, ino) via the already-open // fd. Using the fd is TOCTOU-safe (no path re-resolution through symlinks) and @@ -643,53 +680,52 @@ impl Chmoder { .as_ref() .is_some_and(|info| !ancestors.insert(info.clone())) { - return r; // cycle: this directory is already an ancestor + return; // cycle: this directory is already an ancestor } - let entries = dir_fd.read_dir()?; - // Determine if we should follow symlinks (doesn't depend on entry_name) let should_follow_symlink = self.traverse_symlinks == TraverseSymlinks::All; for entry_name in entries { let entry_path = dir_path.join(&entry_name); - let dir_meta = dir_fd.metadata_at(&entry_name, should_follow_symlink.into()); - let Ok(meta) = dir_meta else { - // Handle permission denied with proper file path context - let e = dir_meta.unwrap_err(); - let error = if e.kind() == std::io::ErrorKind::PermissionDenied { - ChmodError::PermissionDenied(entry_path).into() - } else { - e.into() - }; - r = r.and(Err(error)); - continue; + let meta = match dir_fd.metadata_at(&entry_name, should_follow_symlink.into()) { + Ok(meta) => meta, + Err(err) => { + // Handle permission denied with proper file path context + let error = if err.kind() == std::io::ErrorKind::PermissionDenied { + ChmodError::PermissionDenied(entry_path).into() + } else { + err.into() + }; + self.report_walk_failure(error); + continue; + } }; if entry_path.is_symlink() { - r = self - .handle_symlink_during_safe_recursion( - &entry_path, - dir_fd, - &entry_name, - ancestors, - ) - .and(r); + if let Err(err) = self.handle_symlink_during_safe_recursion( + &entry_path, + dir_fd, + &entry_name, + ancestors, + ) { + self.report_walk_failure(err); + } } else { // For regular files and directories, chmod them. // Always use NoFollow: we already confirmed via stat that the entry // is not a symlink, so this prevents TOCTOU races where an attacker // replaces the entry with a symlink between stat and chmod. - r = self - .safe_chmod_file( - &entry_path, - dir_fd, - &entry_name, - meta.mode() & 0o7777, - SymlinkBehavior::NoFollow, - ) - .and(r); + if let Err(err) = self.safe_chmod_file( + &entry_path, + dir_fd, + &entry_name, + meta.mode() & 0o7777, + SymlinkBehavior::NoFollow, + ) { + self.report_walk_failure(err); + } // Recurse into subdirectories using the existing directory fd. // Open with NoFollow unless `-L` was requested: this prevents a @@ -698,18 +734,9 @@ impl Chmoder { if meta.is_dir() { match dir_fd.open_subdir(&entry_name, should_follow_symlink.into()) { Ok(child_dir_fd) => { - r = self - .safe_traverse_dir(&child_dir_fd, &entry_path, ancestors) - .and(r); - } - Err(err) => { - let error = if err.kind() == std::io::ErrorKind::PermissionDenied { - ChmodError::PermissionDenied(entry_path).into() - } else { - err.into() - }; - r = r.and(Err(error)); + self.safe_traverse_dir(&child_dir_fd, &entry_path, ancestors); } + Err(err) => self.report_unreadable_dir(&entry_path, err), } } } @@ -720,8 +747,6 @@ impl Chmoder { if let Some(info) = dir_info { ancestors.remove(&info); } - - r } #[cfg(not(target_os = "redox"))] @@ -742,7 +767,10 @@ impl Chmoder { // followed by Follow is the intended behavior and not a TOCTOU concern. // 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, ancestors), + Ok(meta) if meta.is_dir() => { + self.walk_dir_with_context(path, false, ancestors); + Ok(()) + } Ok(meta) => { // It's a file symlink, chmod it using safe traversal self.safe_chmod_file( diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index f4e07d9bdbf..d527ae8bc01 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -445,12 +445,8 @@ fn test_chmod_recursive_correct_exit_code() { perms.set_mode(0o000); set_permissions(at.plus_as_string("a"), perms).unwrap(); - // With safe_traversal enabled on all Unix platforms (except Redox), - // we get detailed error messages that include the file path - #[cfg(all(unix, not(target_os = "redox")))] - let err_msg = "chmod: cannot access 'a': Permission denied\n"; - #[cfg(not(all(unix, not(target_os = "redox"))))] - let err_msg = "chmod: Permission denied\n"; + // `a` is changed before it is read, so the failure is about reading it. + let err_msg = "chmod: cannot read directory 'a': Permission denied\n"; // order of command is a, a/b then c // command is expected to fail and not just take the last exit code @@ -461,7 +457,8 @@ fn test_chmod_recursive_correct_exit_code() { .arg("z") .umask(0) .fails() - .stderr_is(err_msg); + .stderr_is(err_msg) + .stdout_contains("'a' could not be accessed\n"); } #[test] @@ -491,9 +488,9 @@ fn test_chmod_recursive() { make_file(&at.plus_as_string("a/b/b"), 0o100444); make_file(&at.plus_as_string("a/b/c/c"), 0o100444); make_file(&at.plus_as_string("z/y"), 0o100444); - // With safe_traversal enabled on all Unix platforms, the error message - // now includes the file path consistently across platforms - let err_msg = "chmod: cannot access 'z': Permission denied\n"; + // Both operands become unreadable, and each failure is reported. + let err_msg = "chmod: cannot read directory 'a': Permission denied\n\ + chmod: cannot read directory 'z': Permission denied\n"; // only the permissions of folder `a` and `z` are changed // folder can't be read after read permission is removed @@ -515,6 +512,68 @@ fn test_chmod_recursive() { assert_eq!(at.metadata("z").permissions().mode(), z_perms_expected); } +#[test] +fn test_chmod_recursive_reports_every_unreadable_directory() { + let (at, mut ucmd) = at_and_ucmd!(); + let dirs = ["d/a", "d/b", "d/c"]; + for dir in dirs { + at.mkdir_all(&format!("{dir}/y")); + set_permissions(at.plus_as_string(dir), Permissions::from_mode(0o311)).unwrap(); + } + + let result = ucmd.args(&["-R", "o=r", "d"]).fails_with_code(1); + for dir in dirs { + result.stderr_contains(format!( + "chmod: cannot read directory '{dir}': Permission denied\n" + )); + // The directory itself is changed before its entries are listed. + assert_eq!(at.metadata(dir).permissions().mode() & 0o777, 0o314); + set_permissions(at.plus_as_string(dir), Permissions::from_mode(0o755)).unwrap(); + } +} + +#[test] +fn test_chmod_recursive_quiet_unreadable_directory() { + // -f hides the diagnostic; the failure still sets the exit status. + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir_all("d/a/y"); + set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o311)).unwrap(); + + ucmd.args(&["-f", "-R", "o=r", "d"]) + .fails_with_code(1) + .no_output(); + + set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o755)).unwrap(); +} + +#[test] +fn test_chmod_recursive_verbose_unreadable_directory() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir_all("d/a/y"); + set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o311)).unwrap(); + + // -v names the directory that was changed but could not be read. + scene + .ucmd() + .args(&["-R", "-v", "o=r", "d"]) + .fails_with_code(1) + .stderr_is("chmod: cannot read directory 'd/a': Permission denied\n") + .stdout_contains("mode of 'd/a' changed from 0311 (-wx--x--x) to 0314 (-wx--xr--)\n") + .stdout_contains("'d/a' could not be accessed\n"); + + // -c reports the change only. + scene + .ucmd() + .args(&["-R", "-c", "o=rw", "d"]) + .fails_with_code(1) + .stderr_is("chmod: cannot read directory 'd/a': Permission denied\n") + .stdout_contains("mode of 'd/a' changed from 0314 (-wx--xr--) to 0316 (-wx--xrw-)\n") + .stdout_does_not_contain("could not be accessed"); + + set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o755)).unwrap(); +} + #[test] #[allow(clippy::unreadable_literal)] fn test_chmod_recursive_read_permission() { From f620342e683a05302e47c5bc772ec2a096ff6ddb Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Fri, 9 Oct 2026 23:21:56 +0200 Subject: [PATCH 2/4] chmod: check the mode once, before any file is touched The mode was parsed again for every file, so under -R a bad one was reported once per entry, after the walk had started. Whether it parses does not depend on the file: check it up front, as GNU does. --- src/uu/chmod/src/chmod.rs | 5 +++++ tests/by-util/test_chmod.rs | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 606c0cffeec..e4c328ce4da 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -456,6 +456,11 @@ impl Chmoder { } fn chmod(&self, files: &[OsString]) -> UResult<()> { + // Whether the mode parses does not depend on the file, so check it + // once before any file is touched: a bad mode is reported once and + // changes nothing, as in GNU. + self.calculate_new_mode(0, false)?; + let mut r = Ok(()); for filename in files { diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index d527ae8bc01..3304a8f718b 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -972,6 +972,18 @@ fn test_gnu_invalid_mode() { scene.ucmd().arg("u+gr").arg("file").fails(); } +#[test] +fn test_chmod_invalid_mode_checked_before_any_file() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir_all("d/a"); + at.touch("d/a/f"); + + // The mode is checked once, up front: one message, and no walk. + ucmd.args(&["-R", "g+rw?x", "d"]) + .fails_with_code(1) + .stderr_only("chmod: invalid operator (expected +, -, or =, but found ?)\n"); +} + #[test] #[cfg(not(target_os = "android"))] fn test_gnu_options() { From d004ad1b925159cbb706cfdbc7d73b95ebe120f3 Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Fri, 9 Oct 2026 23:24:32 +0200 Subject: [PATCH 3/4] chmod: report every failure from one place change_file printed its own error and returned a bare exit code, which the recursive walk told apart from a real error by its empty message. Return the error instead and let one helper print it, unless -f, and set the exit status, for the operand loop as well as for the walk: every failing operand is reported, as GNU does. The umask diagnostic is shown where it is found, since GNU prints it even under -f. --- src/uu/chmod/src/chmod.rs | 62 +++++++++++++++++-------------------- tests/by-util/test_chmod.rs | 30 ++++++++++++++++++ 2 files changed, 58 insertions(+), 34 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index e4c328ce4da..e2d75466c2b 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -418,17 +418,15 @@ impl Chmoder { } } - /// Report a failure met while walking a tree under `-R`. + /// Report a failure met on one file. /// - /// The walk goes on after a failure, so each one is reported where it - /// happens rather than returned, which would keep only one of them. `-f` - /// hides the message; the failure still decides the exit status, as in - /// GNU. An error whose message was already printed (`change_file`) has - /// nothing left to show. - fn report_walk_failure(&self, error: Box) { - let message = error.to_string(); - if !self.quiet && !message.is_empty() { - show_error!("{message}"); + /// The remaining operands, and the rest of a tree under `-R`, are still + /// processed after a failure, so each one is reported where it happens + /// rather than returned, which would keep only one of them. `-f` hides + /// the message; the failure still decides the exit status, as in GNU. + fn report_failure(&self, error: Box) { + if !self.quiet { + show_error!("{error}"); } set_exit_code(error.code()); } @@ -437,7 +435,7 @@ impl Chmoder { /// changed before the attempt, so with `-v` GNU also says that the /// subtree was left alone. fn report_unreadable_dir(&self, path: &Path, err: std::io::Error) { - self.report_walk_failure(ChmodError::CannotReadDirectory(path.into(), err).into()); + self.report_failure(ChmodError::CannotReadDirectory(path.into(), err).into()); if self.verbose { println!( "{}", @@ -461,8 +459,6 @@ impl Chmoder { // changes nothing, as in GNU. self.calculate_new_mode(0, false)?; - let mut r = Ok(()); - for filename in files { let file = Path::new(filename); // `Path::exists()` returns `false` when the path's metadata cannot be @@ -533,11 +529,11 @@ impl Chmoder { if self.recursive { let mut ancestors = HashSet::new(); self.walk_dir_with_context(file, true, &mut ancestors); - } else { - r = self.chmod_file(file).and(r); + } else if let Err(err) = self.chmod_file(file) { + self.report_failure(err); } } - r + Ok(()) } /// Whether `file` is `/`, by `(st_dev, st_ino)` rather than by name, so a @@ -569,7 +565,7 @@ impl Chmoder { } if let Err(err) = self.chmod_file(file_path) { - self.report_walk_failure(err); + self.report_failure(err); } // Determine whether to traverse symlinks based on context and traversal mode @@ -606,7 +602,7 @@ impl Chmoder { for dir_entry in entries { match dir_entry { Ok(entry) => paths_in_this_dir.push(entry.path()), - Err(err) => self.report_walk_failure(err.into()), + Err(err) => self.report_failure(err.into()), } } for path in paths_in_this_dir { @@ -636,7 +632,7 @@ impl Chmoder { } if let Err(err) = self.chmod_file(file_path) { - self.report_walk_failure(err); + self.report_failure(err); } // Determine whether to traverse symlinks based on context and traversal mode @@ -703,7 +699,7 @@ impl Chmoder { } else { err.into() }; - self.report_walk_failure(error); + self.report_failure(error); continue; } }; @@ -715,7 +711,7 @@ impl Chmoder { &entry_name, ancestors, ) { - self.report_walk_failure(err); + self.report_failure(err); } } else { // For regular files and directories, chmod them. @@ -729,7 +725,7 @@ impl Chmoder { meta.mode() & 0o7777, SymlinkBehavior::NoFollow, ) { - self.report_walk_failure(err); + self.report_failure(err); } // Recurse into subdirectories using the existing directory fd. @@ -882,25 +878,23 @@ impl Chmoder { && self.option_like_mode && (new_mode & !naively_expected_new_mode) != 0 { - return Err(ChmodError::NewPermissions( + // Shown even under `-f`, as GNU does: it is about the mode as + // typed, not about this file. + show!(ChmodError::NewPermissions( file.into(), display_permissions_unix(new_mode, false), display_permissions_unix(naively_expected_new_mode, false), - ) - .into()); + )); } Ok(()) } - fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> Result<(), i32> { + fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> UResult<()> { // Always issue the chmod(2) call, even when the bits are unchanged: the // syscall can still fail (e.g. lacking permission on the file) and that // failure must be reported, matching GNU. if let Err(err) = fs::set_permissions(file, fs::Permissions::from_mode(mode)) { - if !self.quiet { - show_error!("{}", ChmodError::ChangingPermissions(file.into(), err)); - } if self.verbose { println!( "failed to change mode of file {} from {fperm:04o} ({}) to {mode:04o} ({})", @@ -909,12 +903,12 @@ impl Chmoder { display_permissions_unix(mode, false) ); } - Err(1) - } else { - // Use the helper method for consistent reporting - self.report_permission_change(file, fperm, mode); - Ok(()) + return Err(ChmodError::ChangingPermissions(file.into(), err).into()); } + + // Use the helper method for consistent reporting + self.report_permission_change(file, fperm, mode); + Ok(()) } } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 3304a8f718b..70f6ddd1f2e 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1139,6 +1139,8 @@ fn test_umask_conflict_reported_only_for_option_like_mode() { (&["-w", "--", "file"], 0o466, true), (&["file", "-w"], 0o466, true), (&["-w", "-w", "--", "file"], 0o466, true), + // `-f` hides most diagnostics, but not this one, as in GNU. + (&["-f", "-w", "file"], 0o466, true), (&["--", "-w", "file"], 0o466, false), (&["--", "-rw", "file"], 0o022, false), // What matters is whether the argument itself began with a hyphen, not whether the mode @@ -1699,6 +1701,34 @@ fn test_chmod_operator_only_still_calls_syscall() { } } +#[test] +fn test_chmod_reports_every_failing_operand() { + use rustix::process::geteuid; + + // Every operand is tried and every failure reported, as GNU does: '-f' + // hides the messages but keeps the exit status, and '-v' still says what + // could not be changed. As above, '/' is a file a non-root user cannot + // chmod. + if geteuid().is_root() || metadata("/").map_or(0, |m| m.uid()) != 0 { + return; + } + + let error = "chmod: changing permissions of '/': Operation not permitted\n"; + new_ucmd!() + .args(&["0", "/", "/"]) + .fails_with_code(1) + .stderr_only(format!("{error}{error}")); + new_ucmd!() + .args(&["-f", "0", "/", "/"]) + .fails_with_code(1) + .no_output(); + new_ucmd!() + .args(&["-v", "-f", "0", "/", "/"]) + .fails_with_code(1) + .no_stderr() + .stdout_contains("failed to change mode of file '/' from"); +} + #[test] fn test_chmod_colored_output() { // Test colored help message From 1c488d518861557baf6d970a5ca44778ef12305b Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Tue, 20 Oct 2026 21:28:25 +0200 Subject: [PATCH 4/4] tests: chmod: accept any errno for the root operand '/' is read-only on macOS, so the message ends differently there. --- tests/by-util/test_chmod.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 70f6ddd1f2e..99dc19a5e59 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1713,11 +1713,17 @@ fn test_chmod_reports_every_failing_operand() { return; } - let error = "chmod: changing permissions of '/': Operation not permitted\n"; - new_ucmd!() - .args(&["0", "/", "/"]) - .fails_with_code(1) - .stderr_only(format!("{error}{error}")); + // The errno text differs between systems: '/' is read-only on macOS. + let result = new_ucmd!().args(&["0", "/", "/"]).fails_with_code(1); + result.no_stdout(); + let lines: Vec<&str> = result.stderr_str().lines().collect(); + assert_eq!(lines.len(), 2, "{lines:?}"); + for line in lines { + assert!( + line.starts_with("chmod: changing permissions of '/': "), + "{line}" + ); + } new_ucmd!() .args(&["-f", "0", "/", "/"]) .fails_with_code(1)