From a5af326dbd9eab596dfd2e5cd8bfc97b56f7c295 Mon Sep 17 00:00:00 2001 From: Eylul Seyitoglu Date: Wed, 28 Oct 2026 02:51:09 +0300 Subject: [PATCH 1/2] chmod: report the real error for files below the -R operand When chmod could not change the mode of a file found while recursing, every error except EROFS was reported as "cannot access ... Permission denied", even EPERM for a file the user doesn't own. Now the error from chmod is shown as is, the same way change_file() already does for the operand. --- src/uu/chmod/src/chmod.rs | 8 +------- tests/by-util/test_chmod.rs | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 46c49dbc7cb..507556795f0 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -845,13 +845,7 @@ impl Chmoder { ); } - return Err(match e.kind() { - std::io::ErrorKind::ReadOnlyFilesystem => { - ChmodError::ChangingPermissions(file_path.into(), e) - } - _ => ChmodError::PermissionDenied(file_path.into()), - } - .into()); + return Err(ChmodError::ChangingPermissions(file_path.into(), e).into()); } // Report the change using the helper method diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 11f8ac65f52..3d7b738efa0 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -596,6 +596,28 @@ fn test_chmod_readonly_filesystem() { scene.umount_temp_fs(); } +#[test] +fn test_chmod_recursive_reports_real_error_below_operand() { + use rustix::process::geteuid; + + // A file found while recursing that the user does not own must be + // reported with the real error (EPERM), like the operand itself, not as + // "cannot access ... Permission denied". '-L' makes the walk chmod the + // root-owned target of the symlink. 'u+r' keeps the mode the same. + let target = "/etc/passwd"; + if geteuid().is_root() || metadata(target).map_or(1, |m| m.uid()) != 0 { + return; + } + + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("tree"); + at.symlink_file(target, "tree/link"); + + ucmd.args(&["-R", "-L", "u+r", "tree"]) + .fails_with_code(1) + .stderr_is("chmod: changing permissions of 'tree/link': Operation not permitted\n"); +} + #[test] #[allow(clippy::unreadable_literal)] fn test_chmod_recursive_correct_exit_code() { From d63ef55fd25df721646d40859922513b6cbd729a Mon Sep 17 00:00:00 2001 From: Eylul Seyitoglu Date: Tue, 3 Nov 2026 04:18:32 +0300 Subject: [PATCH 2/2] chmod: print the -v failure line like GNU Show the old and new mode, drop the extra 'file' word for the operand, and print the line after the error, as GNU does. --- src/uu/chmod/src/chmod.rs | 45 +++++++++++++++++++------------------ tests/by-util/test_chmod.rs | 28 ++++++++++++++--------- 2 files changed, 41 insertions(+), 32 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 507556795f0..87c244a72ce 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -838,14 +838,8 @@ impl Chmoder { // Use safe traversal to change the mode if let Err(e) = dir_fd.chmod_at(entry_name, new_mode, symlink_behavior) { - if self.verbose { - println!( - "failed to change mode of {} to {new_mode:o}", - file_path.quote(), - ); - } - - return Err(ChmodError::ChangingPermissions(file_path.into(), e).into()); + self.report_failed_change(file_path, e, current_mode, new_mode); + return Ok(()); } // Report the change using the helper method @@ -901,32 +895,39 @@ impl Chmoder { return Ok(()); } - self.change_file(fperm, self.fmode.unwrap_or(new_mode), file)?; - - self.report_umask_conflict(file, new_mode, naively_expected_new_mode); + if self.change_file(fperm, self.fmode.unwrap_or(new_mode), file) { + self.report_umask_conflict(file, new_mode, naively_expected_new_mode); + } Ok(()) } - fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> UResult<()> { + /// Returns false if the mode could not be changed (already reported). + fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> bool { // 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.verbose { - println!( - "failed to change mode of file {} from {fperm:04o} ({}) to {mode:04o} ({})", - file.quote(), - display_permissions_unix(fperm, false), - display_permissions_unix(mode, false) - ); - } - return Err(ChmodError::ChangingPermissions(file.into(), err).into()); + self.report_failed_change(file, err, fperm, mode); + return false; } // Use the helper method for consistent reporting self.report_permission_change(file, fperm, mode); - Ok(()) + true + } + + /// Report a failed chmod: the error first, then the `-v` line, like GNU. + fn report_failed_change(&self, file: &Path, err: std::io::Error, old_mode: u32, new_mode: u32) { + self.report_failure(ChmodError::ChangingPermissions(file.into(), err).into()); + if self.verbose { + println!( + "failed to change mode of {} from {old_mode:04o} ({}) to {new_mode:04o} ({})", + file.quote(), + display_permissions_unix(old_mode, false), + display_permissions_unix(new_mode, false) + ); + } } } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 3d7b738efa0..8f43cf447c5 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -600,22 +600,30 @@ fn test_chmod_readonly_filesystem() { fn test_chmod_recursive_reports_real_error_below_operand() { use rustix::process::geteuid; - // A file found while recursing that the user does not own must be - // reported with the real error (EPERM), like the operand itself, not as - // "cannot access ... Permission denied". '-L' makes the walk chmod the - // root-owned target of the symlink. 'u+r' keeps the mode the same. + // '-L' makes chmod follow the link to a root-owned file: EPERM let target = "/etc/passwd"; - if geteuid().is_root() || metadata(target).map_or(1, |m| m.uid()) != 0 { + let Ok(meta) = metadata(target) else { return }; + if geteuid().is_root() || meta.uid() != 0 { return; } + let mode = meta.mode() & 0o7777; - let (at, mut ucmd) = at_and_ucmd!(); - at.mkdir("tree"); - at.symlink_file(target, "tree/link"); + let ts = TestScenario::new(util_name!()); + ts.fixtures.mkdir("tree"); + ts.fixtures.symlink_file(target, "tree/link"); - ucmd.args(&["-R", "-L", "u+r", "tree"]) + ts.ucmd() + .args(&["-R", "-L", "u+r", "tree"]) .fails_with_code(1) .stderr_is("chmod: changing permissions of 'tree/link': Operation not permitted\n"); + + ts.ucmd() + .args(&["-v", "-R", "-L", "u+r", "tree"]) + .fails_with_code(1) + .stdout_contains(format!( + "failed to change mode of 'tree/link' from {mode:04o} (" + )) + .stdout_contains(format!(") to {:04o} (", mode | 0o400)); } #[test] @@ -1916,7 +1924,7 @@ fn test_chmod_reports_every_failing_operand() { .args(&["-v", "-f", "0", "/", "/"]) .fails_with_code(1) .no_stderr() - .stdout_contains("failed to change mode of file '/' from"); + .stdout_contains("failed to change mode of '/' from"); } #[test]