diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index 971e99afcdb..a921c759070 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -118,6 +118,8 @@ EBUSY EEXIST EINVAL EISDIR +EMFILE +ENFILE ENODATA ENOENT ENOSPC diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index f586ef8a681..8907180e6bc 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's +// spell-checker:ignore (ToDO) Chmoder cmode fmode fperm fref ugoa RFILE RFILE's fchmod #![cfg(unix)] @@ -45,6 +45,12 @@ enum ChmodError { NewPermissions(PathBuf, String, String), #[error("{}", translate!("chmod-error-changing-permissions", "file" => _0.quote(), "err" => strip_errno(_1)))] ChangingPermissions(PathBuf, std::io::Error), + #[cfg(not(target_os = "redox"))] + #[error("{}", translate!("perms-cannot-access-replaced", "file" => _0.quote()))] + Replaced(PathBuf), + #[cfg(not(target_os = "redox"))] + #[error("{}", translate!("perms-cannot-access", "file" => _0.quote(), "error" => strip_errno(_1)))] + CannotAccess(PathBuf, std::io::Error), } impl UError for ChmodError {} @@ -595,8 +601,6 @@ impl Chmoder { return Ok(()); } - let mut r = self.chmod_file(file_path); - // Determine whether to traverse symlinks based on context and traversal mode let should_follow_symlink = match self.traverse_symlinks { TraverseSymlinks::All => true, @@ -604,22 +608,76 @@ impl Chmoder { TraverseSymlinks::None => false, }; - // Recurse via safe traversal, opening under the same symlink policy the checks - // above used: the pathname is resolved again here, so under `-P` (the `-R` - // default) O_NOFOLLOW fails the open rather than redirecting the descent into a - // 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); + let descend = (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir(); + if !descend { + return self.chmod_file(file_path); + } + + // Change the mode through a descriptor checked not to be "/", which a rename + // cannot re-point. Under `-h` with `-H`/`-L`, or if it does not open yet, go by + // path as GNU does; the descent below then checks for "/" itself. + let pinned = if should_follow_symlink == self.dereference { + match DirFd::open(file_path, should_follow_symlink.into()) + .and_then(|dir_fd| Ok((dir_fd.metadata()?, dir_fd))) + { + Ok(pinned) => Some(pinned), + Err(e) + if matches!( + e.raw_os_error(), + Some(uucore::libc::EACCES | uucore::libc::EMFILE | uucore::libc::ENFILE) + ) => + { + None + } + // Other errors, such as the name now leading to a file or a symlink, + // leave it alone rather than change whatever is there by path. + Err(e) => return Err(ChmodError::CannotAccess(file_path.into(), e).into()), + } + } else { + None + }; + // The descriptor is closed once the mode is changed, keeping only the + // directory's identity, so the walk holds no more descriptors than without it. + let (mut r, changed) = match pinned { + Some((meta, dir_fd)) => { + if self.preserve_root && uucore::fs::metadata_is_root_dir(&meta) { + show!(ChmodError::PreserveRootSameAs(file_path.into())); + return Ok(()); } - 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())); + let r = self.chmod_dir_fd(&dir_fd, meta.mode() & 0o7777, file_path); + (r, Some(meta.file_info())) + } + None => (self.chmod_file(file_path), None), + }; + + // Open again to descend, as GNU does, so the mode just set decides whether + // the directory can be read. + match DirFd::open(file_path, should_follow_symlink.into()) { + Ok(dir_fd) => { + if let Some(changed) = changed { + // Descend only into the directory changed above, known not to be "/". + if !dir_fd + .metadata() + .is_ok_and(|meta| meta.file_info() == changed) + { + return r.and(Err(ChmodError::Replaced(file_path.into()).into())); } + } else if self.preserve_root + && dir_fd + .metadata() + .is_ok_and(|meta| uucore::fs::metadata_is_root_dir(&meta)) + { + show!(ChmodError::PreserveRootSameAs(file_path.into())); + return r; + } + 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(ChmodError::CannotAccess(file_path.into(), err).into())); } } } @@ -838,8 +896,29 @@ impl Chmoder { return Ok(()); } - self.change_file(fperm, self.fmode.unwrap_or(new_mode), file)?; + let mode = self.fmode.unwrap_or(new_mode); + self.change_file(fperm, mode, file, || { + fs::set_permissions(file, fs::Permissions::from_mode(mode)) + })?; + self.check_umask_kept(file, new_mode, naively_expected_new_mode) + } + /// [`Self::chmod_file`] for a directory already open, which no rename can + /// re-point, whose mode bits are `fperm`. + #[cfg(not(target_os = "redox"))] + fn chmod_dir_fd(&self, dir_fd: &DirFd, fperm: u32, file: &Path) -> UResult<()> { + let (new_mode, naively_expected_new_mode) = self.calculate_new_mode(fperm, true)?; + let mode = self.fmode.unwrap_or(new_mode); + self.change_file(fperm, mode, file, || dir_fd.fchmod(mode))?; + self.check_umask_kept(file, new_mode, naively_expected_new_mode) + } + + fn check_umask_kept( + &self, + file: &Path, + new_mode: u32, + naively_expected_new_mode: u32, + ) -> UResult<()> { // A bare mode such as `-w` is umask-relative, so the umask can keep permissions that // the user asked to drop. GNU reports that as an error, but only when the mode was // written in the option-like form (`chmod -w f`), where it doubles as a hint that the @@ -860,11 +939,18 @@ impl Chmoder { Ok(()) } - fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> Result<(), i32> { + /// Report the outcome of `set`, which applies `mode` to `file`. + fn change_file( + &self, + fperm: u32, + mode: u32, + file: &Path, + set: impl FnOnce() -> std::io::Result<()>, + ) -> Result<(), i32> { // 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 let Err(err) = set() { if !self.quiet { show_error!("{}", ChmodError::ChangingPermissions(file.into(), err)); } diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index c17fa99d21d..46007586b79 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -638,6 +638,21 @@ pub fn path_is_root_dir>(path: P, dereference: bool) -> bool { } } +/// Whether the file that `meta`, a `stat` already taken, describes is `/`. +/// +/// Unlike [`path_is_root_dir`] this does not look the path up again, so the +/// answer describes the file the caller is about to act on even if the path +/// has been re-pointed since. Callers hold either a [`fs::Metadata`] or, from +/// an `fstat` of a descriptor, a `safe_traversal::Metadata`, hence the generic. +#[cfg(unix)] +pub fn metadata_is_root_dir(meta: &impl MetadataExt) -> bool { + // st_dev and st_ino have different types on different platforms + #[allow(clippy::unnecessary_cast)] + root_file_information().is_some_and(|root| { + root.0.st_dev as u64 == meta.dev() && root.0.st_ino as u64 == meta.ino() + }) +} + /// Check if two files are identical by comparing their contents. /// /// Returns `Ok(true)` if both files exist, are regular files, and have identical contents. @@ -1617,4 +1632,13 @@ mod tests { assert!(path_is_root_dir(&link, true)); assert!(!path_is_root_dir(&link, false)); } + + #[cfg(unix)] + #[test] + fn test_metadata_is_root_dir() { + assert!(metadata_is_root_dir(&fs::metadata("/").unwrap())); + + let dir = tempdir().unwrap(); + assert!(!metadata_is_root_dir(&fs::metadata(dir.path()).unwrap())); + } } diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 3eb3dca9fae..52f93d23fe3 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -25,7 +25,7 @@ use walkdir::WalkDir; #[cfg(target_os = "linux")] use crate::features::fs::FileInformation; -use crate::features::fs::path_is_root_dir; +use crate::features::fs::{metadata_is_root_dir, path_is_root_dir}; #[cfg(target_os = "linux")] use crate::features::safe_traversal::{DirFd, FileInfo, SymlinkBehavior}; @@ -244,7 +244,21 @@ fn is_root(path: &Path, would_traverse_symlink: bool) -> bool { if !path_is_root_dir(path, would_traverse_symlink) { return false; } + report_root(path); + true +} + +/// [`is_root`], judged on `meta` the caller already holds instead of on a new +/// lookup of `path`, which a concurrent rename could have re-pointed. +fn meta_is_root(path: &Path, meta: &Metadata) -> bool { + if !metadata_is_root_dir(meta) { + return false; + } + report_root(path); + true +} +fn report_root(path: &Path) { if path.as_os_str() == "/" { show_error!("it is dangerous to operate recursively on '/'"); } else { @@ -254,7 +268,6 @@ fn is_root(path: &Path, would_traverse_symlink: bool) -> bool { ); } show_error!("use --no-preserve-root to override this failsafe"); - true } /// Whether `dir_fd` refers to the very object `meta` describes. @@ -306,9 +319,13 @@ impl ChownExecutor { return 1; }; + // Also judge `meta`: the descent below is pinned to it, so a swap after the + // path lookup in `is_root` cannot slip "/" past, while that lookup still + // catches a symlink to "/" which `meta` did not follow. if self.recursive && self.preserve_root - && is_root(path, self.traverse_symlinks != TraverseSymlinks::None) + && (is_root(path, self.traverse_symlinks != TraverseSymlinks::None) + || meta_is_root(path, &meta)) { // Fail-fast, do not attempt to recurse. return 1; diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index f4e07d9bdbf..8aacfb34701 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr +// spell-checker:ignore (words) dirfd subdirs openat FDCWD rwxr Nofile use std::fs::{OpenOptions, Permissions, metadata, set_permissions}; use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; @@ -598,6 +598,207 @@ fn test_chmod_preserve_root_symlink_during_recursion() { ); } +#[test] +fn test_chmod_preserve_root_symlink_operand() { + // A symlink operand that resolves to '/' is '/' whenever it is followed: + // by default and with -H or -L, and with -P once a trailing slash makes the + // lookup resolve it. `u+u` changes nothing, should the check ever let it by. + let cases: [(&[&str], &str); 4] = [ + (&["-R"], "root"), + (&["-R", "-H"], "root"), + (&["-R", "-L"], "root"), + (&["-R", "-P"], "root/"), + ]; + for (flags, operand) in cases { + let (at, mut ucmd) = at_and_ucmd!(); + at.symlink_dir("/", "root"); + ucmd.args(flags) + .args(&["--preserve-root", "u+u", operand]) + .fails_with_code(1) + .stderr_is(format!( + "chmod: it is dangerous to operate recursively on '{operand}' (same as '/')\n\ + chmod: use --no-preserve-root to override this failsafe\n" + )); + } +} + +#[test] +fn test_chmod_recursive_operand_that_does_not_open() { + // An operand directory that cannot be opened yet still has its mode changed, + // and is descended into once that change makes it readable, also through a + // symlink operand, which -P leaves alone. + for flags in [&["-R"][..], &["-R", "-H"], &["-R", "-L"], &["-R", "-P"]] { + for operand in ["d", "link"] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("d"); + at.touch("d/f"); + at.set_mode("d/f", 0o644); + at.set_mode("d", 0o000); + at.relative_symlink_dir("d", "link"); + + ucmd.args(flags) + .args(&["u+rwx", operand]) + .succeeds() + .no_output(); + + let mode = |file| at.metadata(file).permissions().mode() & 0o7777; + if operand == "link" && flags.contains(&"-P") { + assert_eq!(mode("d"), 0o000, "{flags:?} {operand}"); + at.set_mode("d", 0o755); + continue; + } + assert_eq!(mode("d"), 0o700, "{flags:?} {operand}"); + assert_eq!(mode("d/f"), 0o744, "{flags:?} {operand}"); + } + } +} + +#[test] +fn test_chmod_recursive_operand_made_unreadable() { + // An operand that the mode change leaves unreadable keeps its new mode, and + // the failed descent names it. Root reads it regardless. + if rustix::process::geteuid().is_root() { + return; + } + for flags in [&["-R"][..], &["-R", "-H"], &["-R", "-L"], &["-R", "-P"]] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("d"); + at.touch("d/f"); + at.set_mode("d/f", 0o644); + at.set_mode("d", 0o755); + + ucmd.args(flags) + .args(&["a-r", "d"]) + .fails_with_code(1) + .stderr_is("chmod: cannot access 'd': Permission denied\n"); + + assert_eq!( + at.metadata("d").permissions().mode() & 0o7777, + 0o311, + "{flags:?}" + ); + at.set_mode("d", 0o755); + assert_eq!( + at.metadata("d/f").permissions().mode() & 0o7777, + 0o644, + "{flags:?}" + ); + } +} + +#[test] +fn test_chmod_recursive_no_dereference_operand() { + // With -h, a directory operand followed by -H or -L has its mode changed by + // path rather than through a descriptor; it is still changed and descended. + for flags in [ + &["-R", "-h"][..], + &["-R", "-H", "-h"], + &["-R", "-L", "-h"], + &["-R", "-P", "-h"], + ] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("d"); + at.touch("d/f"); + at.set_mode("d", 0o755); + at.set_mode("d/f", 0o644); + + ucmd.args(flags).args(&["700", "d"]).succeeds().no_output(); + + assert_eq!(at.metadata("d").permissions().mode() & 0o7777, 0o700); + assert_eq!(at.metadata("d/f").permissions().mode() & 0o7777, 0o700); + } +} + +/// `chmod -R` changes the operand's mode and then opens it again to descend: +/// it must only descend into the directory whose mode it changed, which is the +/// one checked for '/', whatever the operand's name points at by then. A racer +/// re-points the operand between two directories of the fixture, so that a +/// failure never reaches outside it, and a file, which fails the open. +#[test] +#[cfg(not(target_os = "redox"))] +fn test_chmod_recursive_descends_only_into_the_directory_it_changed() { + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + for dir in ["a", "b"] { + at.mkdir(dir); + at.touch(format!("{dir}/f")); + } + at.touch("x"); + at.relative_symlink_dir("a", "op"); + + let stop = Arc::new(AtomicBool::new(false)); + let (op, tmp) = (at.plus("op"), at.plus("op.tmp")); + let racer_stop = stop.clone(); + let racer = std::thread::spawn(move || { + for target in ["b", "x", "a", "x"].iter().cycle() { + if racer_stop.load(Ordering::Relaxed) { + break; + } + std::os::unix::fs::symlink(target, &tmp).unwrap(); + std::fs::rename(&tmp, &op).unwrap(); + } + }); + + for _ in 0..200 { + for dir in ["a", "b"] { + at.set_mode(dir, 0o755); + at.set_mode(&format!("{dir}/f"), 0o644); + } + + let result = scene.ucmd().args(&["-R", "700", "op"]).run(); + + for dir in ["a", "b"] { + let mode = |file: &str| at.metadata(file).permissions().mode() & 0o7777; + assert!( + mode(&format!("{dir}/f")) == 0o644 || mode(dir) == 0o700, + "descended into {dir} without changing its mode" + ); + } + if !result.succeeded() { + // One error about the operand. Its cause varies with the timing: + // macOS can also fail a lookup through a symlink being replaced. + let stderr = result.code_is(1).stderr_str(); + assert!( + stderr.starts_with("chmod: ") + && stderr.contains("'op'") + && stderr.lines().count() == 1, + "{stderr}" + ); + } + } + + stop.store(true, Ordering::Relaxed); + racer.join().unwrap(); +} + +/// Each directory reached through a symlink under -L takes one descriptor +/// while the walk is below it, not more. +#[test] +#[cfg(target_os = "linux")] +fn test_chmod_recursive_dereference_one_descriptor_per_level() { + use rustix::process::Resource; + + let (at, mut ucmd) = at_and_ucmd!(); + for i in 0..=10 { + at.mkdir(format!("d{i}")); + } + for i in 0..10 { + at.relative_symlink_dir(&format!("../d{}", i + 1), &format!("d{i}/next")); + } + at.touch("d10/f"); + at.set_mode("d10/f", 0o644); + + ucmd.args(&["-R", "-L", "700", "d0"]) + .limit(Resource::Nofile, 20, 20) + .succeeds() + .no_output(); + + assert_eq!(at.metadata("d10/f").permissions().mode() & 0o7777, 0o700); +} + #[test] fn test_chmod_recursive_reference_does_not_follow_inner_symlink() { // A symlink met inside the tree during `chmod -R` must not have its referent