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..e2d75466c2b 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,32 @@ impl Chmoder { } } + /// Report a failure met on one file. + /// + /// 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()); + } + + /// 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_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!( @@ -426,7 +454,10 @@ impl Chmoder { } fn chmod(&self, files: &[OsString]) -> UResult<()> { - let mut r = Ok(()); + // 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)?; for filename in files { let file = Path::new(filename); @@ -497,14 +528,12 @@ impl Chmoder { } if self.recursive { let mut ancestors = HashSet::new(); - r = self - .walk_dir_with_context(file, true, &mut ancestors) - .and(r); - } else { - r = self.chmod_file(file).and(r); + self.walk_dir_with_context(file, true, &mut ancestors); + } 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 @@ -527,15 +556,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_failure(err); + } // Determine whether to traverse symlinks based on context and traversal mode let should_follow_symlink = match self.traverse_symlinks { @@ -546,6 +577,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 +593,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_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 +615,6 @@ impl Chmoder { ancestors.remove(&info); } } - r } #[cfg(not(target_os = "redox"))] @@ -587,15 +623,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_failure(err); + } // Determine whether to traverse symlinks based on context and traversal mode let should_follow_symlink = match self.traverse_symlinks { @@ -610,20 +648,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 +661,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 +681,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_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_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_failure(err); + } // Recurse into subdirectories using the existing directory fd. // Open with NoFollow unless `-L` was requested: this prevents a @@ -698,18 +735,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 +748,6 @@ impl Chmoder { if let Some(info) = dir_info { ancestors.remove(&info); } - - r } #[cfg(not(target_os = "redox"))] @@ -742,7 +768,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( @@ -849,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} ({})", @@ -876,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 f4e07d9bdbf..99dc19a5e59 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() { @@ -913,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() { @@ -1068,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 @@ -1628,6 +1701,40 @@ 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; + } + + // 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) + .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