From 7c0246729f8e14afd15c9551d94c9097b17ca9a6 Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Wed, 7 Oct 2026 23:20:58 +0200 Subject: [PATCH 1/3] chown, chgrp: say "cannot read directory" for a directory that does not open A directory that could not be opened for reading under -R was reported as "cannot access", where GNU says "cannot read directory". The walkdir path used off Linux also printed its diagnostics with -f, which is meant to hide them. Report a directory that does not open, or that opens but cannot be listed, as "cannot read directory", on both traversal paths, and keep -f quiet on both. --- src/uucore/locales/en-US.ftl | 1 + src/uucore/src/lib/features/perms.rs | 25 +++++++++++++----- tests/by-util/test_chgrp.rs | 39 ++++++++++++++++++++++++++-- tests/by-util/test_chown.rs | 31 ++++++++++++++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/src/uucore/locales/en-US.ftl b/src/uucore/locales/en-US.ftl index 6afc1ab7647..33487d7a47f 100644 --- a/src/uucore/locales/en-US.ftl +++ b/src/uucore/locales/en-US.ftl @@ -99,6 +99,7 @@ mode-diag-help-syntax = a mode is either octal, as in 644, or clauses such as u+ # Shared recursive chown/chgrp/chmod diagnostics (uucore::perms) perms-cannot-access = cannot access { $file }: { $error } +perms-cannot-read-directory = cannot read directory { $file }: { $error } perms-cannot-access-replaced = cannot access { $file }: replaced while it was being processed # Encoding/decoding error messages (base32, base64, basenc) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 3eb3dca9fae..7bb69ad6537 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -541,9 +541,8 @@ impl ChownExecutor { *ret = 1; if self.verbosity.level != VerbosityLevel::Silent { show_error!( - "cannot read directory {}: {}", - dir_path.quote(), - strip_errno(&e) + "{}", + translate!("perms-cannot-read-directory", "file" => dir_path.quote(), "error" => strip_errno(&e)) ); } return; @@ -627,12 +626,13 @@ impl ChownExecutor { Ok(subdir_fd) => { self.safe_traverse_dir(&subdir_fd, &entry_path, ret, ancestors); } + // A directory that does not open cannot be read. Err(e) => { *ret = 1; if self.verbosity.level != VerbosityLevel::Silent { show_error!( "{}", - translate!("perms-cannot-access", "file" => entry_path.quote(), "error" => strip_errno(&e)) + translate!("perms-cannot-read-directory", "file" => entry_path.quote(), "error" => strip_errno(&e)) ); } } @@ -667,11 +667,21 @@ impl ChownExecutor { let entry = match entry { Err(e) => { ret = 1; + if self.verbosity.level == VerbosityLevel::Silent { + continue; + } if let Some(path) = e.path() { + // A directory that can be reached but not listed is + // reported as unreadable, like GNU does. + let message = if e.io_error().is_some() && path.is_dir() { + "perms-cannot-read-directory" + } else { + "perms-cannot-access" + }; show_error!( "{}", translate!( - "perms-cannot-access", + message, "file" => path.quote(), "error" => if let Some(error) = e.io_error() { strip_errno(error) @@ -799,7 +809,8 @@ impl ChownExecutor { 0 } - /// Try to open directory with error reporting + /// Try to open directory with error reporting: a directory that does + /// not open cannot be read. #[cfg(target_os = "linux")] fn try_open_dir(&self, path: &Path) -> Option { DirFd::open(path, SymlinkBehavior::Follow) @@ -807,7 +818,7 @@ impl ChownExecutor { if self.verbosity.level != VerbosityLevel::Silent { show_error!( "{}", - translate!("perms-cannot-access", "file" => path.quote(), "error" => strip_errno(&e)) + translate!("perms-cannot-read-directory", "file" => path.quote(), "error" => strip_errno(&e)) ); } }) diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index 3f29e130a69..31e50ce92ce 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -89,6 +89,41 @@ fn test_fail_silently() { } } +#[test] +#[cfg(unix)] +fn test_chgrp_recursive_unreadable_directory() { + use std::fs::{Permissions, set_permissions}; + use std::os::unix::fs::PermissionsExt; + + if getegid().is_root() { + // root can read any directory + return; + } + let dirs = ["d/a", "d/b"]; + + let (at, mut ucmd) = at_and_ucmd!(); + 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", "--reference=d", "d"]).fails_with_code(1); + for dir in dirs { + result.stderr_contains(format!( + "chgrp: cannot read directory '{dir}': Permission denied\n" + )); + set_permissions(at.plus_as_string(dir), Permissions::from_mode(0o755)).unwrap(); + } + + // -f hides the message; 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", "--reference=d", "d"]) + .fails_with_code(1) + .no_output(); + set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o755)).unwrap(); +} + #[test] fn test_preserve_root() { // It's weird that on OS X, `realpath /etc/..` returns '/private' @@ -336,7 +371,7 @@ fn test_permission_denied() { .arg(group.as_raw().to_string()) .arg("dir") .fails() - .stderr_only("chgrp: cannot access 'dir': Permission denied\n"); + .stderr_only("chgrp: cannot read directory 'dir': Permission denied\n"); } } @@ -355,7 +390,7 @@ fn test_subdir_permission_denied() { .arg(group.as_raw().to_string()) .arg("dir") .fails() - .stderr_only("chgrp: cannot access 'dir/subdir': Permission denied\n"); + .stderr_only("chgrp: cannot read directory 'dir/subdir': Permission denied\n"); } } diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index 4d77788792a..20615967632 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -766,6 +766,37 @@ fn test_chown_owner_group_mix() { .stderr_contains("failed to change"); } +#[test] +#[cfg(unix)] +fn test_chown_recursive_unreadable_directory() { + use std::fs::{Permissions, set_permissions}; + use std::os::unix::fs::PermissionsExt; + + if rustix::process::geteuid().is_root() { + // root can read any directory + return; + } + 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(); + + scene + .ucmd() + .args(&["-R", "--reference=d", "d"]) + .fails_with_code(1) + .stderr_is("chown: cannot read directory 'd/a': Permission denied\n"); + + // -f hides the message; the failure still sets the exit status. + scene + .ucmd() + .args(&["-f", "-R", "--reference=d", "d"]) + .fails_with_code(1) + .no_output(); + + set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o755)).unwrap(); +} + #[test] fn test_chown_recursive() { let scene = TestScenario::new(util_name!()); From 17ae8382cfa2cdce3f1dad468c3099bbb3f35ebb Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Wed, 7 Oct 2026 23:57:24 +0200 Subject: [PATCH 2/3] chown, chgrp: tell an unreadable directory from walkdir's own ordering, trim the comments and the chgrp test --- src/uucore/src/lib/features/perms.rs | 14 +++++++------- tests/by-util/test_chgrp.rs | 19 ++++++------------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 7bb69ad6537..9072c1c249c 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -36,7 +36,7 @@ use std::io::Result as IOResult; use std::os::unix::fs::MetadataExt; use std::os::unix::ffi::OsStrExt; -use std::path::Path; +use std::path::{Path, PathBuf}; #[derive(Debug, thiserror::Error)] enum PermsError { @@ -626,7 +626,6 @@ impl ChownExecutor { Ok(subdir_fd) => { self.safe_traverse_dir(&subdir_fd, &entry_path, ret, ancestors); } - // A directory that does not open cannot be read. Err(e) => { *ret = 1; if self.verbosity.level != VerbosityLevel::Silent { @@ -662,6 +661,9 @@ impl ChownExecutor { .follow_links(self.traverse_symlinks == TraverseSymlinks::All) .min_depth(1) .into_iter(); + // The directory whose entry came last: walkdir reports a directory it + // cannot read right after that entry, under the same path. + let mut last_dir: Option = None; // We can't use a for loop because we need to manipulate the iterator inside the loop. while let Some(entry) = iterator.next() { let entry = match entry { @@ -671,9 +673,7 @@ impl ChownExecutor { continue; } if let Some(path) = e.path() { - // A directory that can be reached but not listed is - // reported as unreadable, like GNU does. - let message = if e.io_error().is_some() && path.is_dir() { + let message = if last_dir.as_deref() == Some(path) { "perms-cannot-read-directory" } else { "perms-cannot-access" @@ -698,6 +698,7 @@ impl ChownExecutor { Ok(entry) => entry, }; let path = entry.path(); + last_dir = entry.file_type().is_dir().then(|| path.to_path_buf()); let Some(meta) = self.obtain_meta(path, self.dereference) else { ret = 1; @@ -809,8 +810,7 @@ impl ChownExecutor { 0 } - /// Try to open directory with error reporting: a directory that does - /// not open cannot be read. + /// Try to open directory with error reporting #[cfg(target_os = "linux")] fn try_open_dir(&self, path: &Path) -> Option { DirFd::open(path, SymlinkBehavior::Follow) diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index 31e50ce92ce..c0de8c28c76 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -99,20 +99,13 @@ fn test_chgrp_recursive_unreadable_directory() { // root can read any directory return; } - let dirs = ["d/a", "d/b"]; - let (at, mut ucmd) = at_and_ucmd!(); - 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", "--reference=d", "d"]).fails_with_code(1); - for dir in dirs { - result.stderr_contains(format!( - "chgrp: cannot read directory '{dir}': Permission denied\n" - )); - set_permissions(at.plus_as_string(dir), Permissions::from_mode(0o755)).unwrap(); - } + at.mkdir_all("d/a/y"); + set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o311)).unwrap(); + + ucmd.args(&["-R", "--reference=d", "d"]) + .fails_with_code(1) + .stderr_is("chgrp: cannot read directory 'd/a': Permission denied\n"); // -f hides the message; the failure still sets the exit status. let (at, mut ucmd) = at_and_ucmd!(); From 3649b1e1be6cdbe057ac13f27ea19091b20a0e78 Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Fri, 9 Oct 2026 23:19:41 +0200 Subject: [PATCH 3/3] chown, chgrp: name the operand too when walkdir cannot read it walkdir never yields the root under min_depth(1), so a read error on the operand itself still came out as "cannot access" on the walkdir path (macOS, the BSDs). Start last_dir from the root, and drop the PathBuf import that the Linux build did not use. --- src/uucore/src/lib/features/perms.rs | 7 ++++--- tests/by-util/test_chgrp.rs | 9 +++++++++ tests/by-util/test_chown.rs | 9 +++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 9072c1c249c..b42d371c716 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -36,7 +36,7 @@ use std::io::Result as IOResult; use std::os::unix::fs::MetadataExt; use std::os::unix::ffi::OsStrExt; -use std::path::{Path, PathBuf}; +use std::path::Path; #[derive(Debug, thiserror::Error)] enum PermsError { @@ -662,8 +662,9 @@ impl ChownExecutor { .min_depth(1) .into_iter(); // The directory whose entry came last: walkdir reports a directory it - // cannot read right after that entry, under the same path. - let mut last_dir: Option = None; + // cannot read right after that entry, under the same path. The root is + // below min_depth, so it never comes out as an entry: start from it. + let mut last_dir = Some(root.to_path_buf()); // We can't use a for loop because we need to manipulate the iterator inside the loop. while let Some(entry) = iterator.next() { let entry = match entry { diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index c0de8c28c76..f74c0573707 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -115,6 +115,15 @@ fn test_chgrp_recursive_unreadable_directory() { .fails_with_code(1) .no_output(); set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o755)).unwrap(); + + // The operand itself. + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("d"); + set_permissions(at.plus_as_string("d"), Permissions::from_mode(0o311)).unwrap(); + ucmd.args(&["-R", "--reference=d", "d"]) + .fails_with_code(1) + .stderr_is("chgrp: cannot read directory 'd': Permission denied\n"); + set_permissions(at.plus_as_string("d"), Permissions::from_mode(0o755)).unwrap(); } #[test] diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index 20615967632..353899c6781 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -795,6 +795,15 @@ fn test_chown_recursive_unreadable_directory() { .no_output(); set_permissions(at.plus_as_string("d/a"), Permissions::from_mode(0o755)).unwrap(); + + // The operand itself. + set_permissions(at.plus_as_string("d"), Permissions::from_mode(0o311)).unwrap(); + scene + .ucmd() + .args(&["-R", "--reference=d", "d"]) + .fails_with_code(1) + .stderr_is("chown: cannot read directory 'd': Permission denied\n"); + set_permissions(at.plus_as_string("d"), Permissions::from_mode(0o755)).unwrap(); } #[test]