From 2ea27e7733cc90189e0956eec53640948b9bf15f Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Mon, 29 Jun 2026 21:14:00 +0200 Subject: [PATCH] chmod: always call chmod(2) even when mode bits are unchanged Operator-only modes ('+', '-', '=') leave the bits unchanged but must still issue the syscall so permission failures are reported. Also align the failure message with GNU (changing permissions of FILE). Should make test tests/chmod/only-op.sh pass --- src/uu/chmod/locales/en-US.ftl | 1 + src/uu/chmod/locales/fr-FR.ftl | 1 + src/uu/chmod/src/chmod.rs | 17 ++++++++++------- tests/by-util/test_chmod.rs | 27 ++++++++++++++++++++++++++- 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/src/uu/chmod/locales/en-US.ftl b/src/uu/chmod/locales/en-US.ftl index 12df1e2b7a5..6404519dce7 100644 --- a/src/uu/chmod/locales/en-US.ftl +++ b/src/uu/chmod/locales/en-US.ftl @@ -11,6 +11,7 @@ chmod-error-preserve-root = it is dangerous to operate recursively on {$file} chmod: use --no-preserve-root to override this failsafe chmod-error-permission-denied = cannot access {$file}: Permission denied 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 # Help messages diff --git a/src/uu/chmod/locales/fr-FR.ftl b/src/uu/chmod/locales/fr-FR.ftl index f4e21b1b725..e2315c65e59 100644 --- a/src/uu/chmod/locales/fr-FR.ftl +++ b/src/uu/chmod/locales/fr-FR.ftl @@ -23,6 +23,7 @@ chmod-error-preserve-root = il est dangereux d'opérer récursivement sur {$file chmod: utiliser --no-preserve-root pour outrepasser cette protection chmod-error-permission-denied = impossible d'accéder à {$file} : Permission refusée 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 # Messages verbeux/de statut diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 4c6b3e0f76c..6e83b4b71ef 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -13,7 +13,9 @@ use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::{Path, PathBuf}; use thiserror::Error; use uucore::display::Quotable; -use uucore::error::{ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code}; +use uucore::error::{ + ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code, strip_errno, +}; use uucore::fs::{FileInformation, display_permissions_unix}; use uucore::mode; use uucore::perms::{TraverseSymlinks, configure_symlink_and_recursion}; @@ -38,6 +40,8 @@ enum ChmodError { PermissionDenied(PathBuf), #[error("{}", translate!("chmod-error-new-permissions", "file" => _0.maybe_quote(), "actual" => _1.clone(), "expected" => _2.clone()))] NewPermissions(PathBuf, String, String), + #[error("{}", translate!("chmod-error-changing-permissions", "file" => _0.quote(), "err" => strip_errno(_1)))] + ChangingPermissions(PathBuf, std::io::Error), } impl UError for ChmodError {} @@ -772,13 +776,12 @@ impl Chmoder { } fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> Result<(), i32> { - if fperm == mode { - // Use the helper method for consistent reporting - self.report_permission_change(file, fperm, mode); - Ok(()) - } else if let Err(err) = fs::set_permissions(file, fs::Permissions::from_mode(mode)) { + // 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!("{err}"); + show_error!("{}", ChmodError::ChangingPermissions(file.into(), err)); } if self.verbose { println!( diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 2541a9f5f0b..1aa2324e504 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -5,7 +5,7 @@ // spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr use std::fs::{OpenOptions, Permissions, metadata, set_permissions}; -use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; +use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; use uutests::at_and_ucmd; use uutests::util::{AtPath, TestScenario, UCommand}; @@ -1409,6 +1409,31 @@ fn test_chmod_recursive_uses_dirfd_for_subdirs() { ); } +#[test] +fn test_chmod_operator_only_still_calls_syscall() { + use uucore::process::geteuid; + + // An operator with no permission letters ('+', '-', '=') leaves the mode + // bits unchanged, yet chmod must still issue the chmod(2) call so that a + // lack of permission is reported instead of silently succeeding. As a + // non-root user, '/' (owned by root) is a file we cannot chmod. + if geteuid() == 0 { + return; + } + if metadata("/").map_or(0, |m| m.uid()) != 0 { + return; // '/' is not root-owned in this environment + } + + for op in ["+", "-", "="] { + new_ucmd!() + .arg(op) + .arg("/") + .fails() + .code_is(1) + .stderr_contains("changing permissions of '/'"); + } +} + #[test] fn test_chmod_colored_output() { // Test colored help message