diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 15b608af6b2..7644dd1a586 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -10,6 +10,9 @@ use std::ffi::OsString; use std::fs; use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; + +#[cfg(target_os = "linux")] +use std::path::PathBuf; use thiserror::Error; use uucore::display::Quotable; use uucore::error::{ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code}; @@ -436,7 +439,11 @@ 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() { - for dir_entry in file_path.read_dir()? { + // `read_dir` keeps a file descriptor open for the directory while + // the iterator is alive. Collect the entries first so the iterator + // (and thus the FD) is dropped before we recurse into children. + let entries: Vec<_> = file_path.read_dir()?.collect(); + for dir_entry in entries { let path = match dir_entry { Ok(entry) => entry.path(), Err(err) => { @@ -469,7 +476,7 @@ impl Chmoder { if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() { match DirFd::open(file_path) { Ok(dir_fd) => { - r = self.safe_traverse_dir(&dir_fd, file_path).and(r); + r = self.safe_traverse_dir(dir_fd, file_path).and(r); } Err(err) => { // Handle permission denied errors with proper file path context @@ -488,60 +495,66 @@ impl Chmoder { } #[cfg(target_os = "linux")] - fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path) -> UResult<()> { + fn safe_traverse_dir(&self, dir_fd: DirFd, dir_path: &Path) -> UResult<()> { let mut r = Ok(()); - - let entries = dir_fd.read_dir()?; + // Depth-first traversal without recursive calls to avoid stacking FDs. + let mut stack: Vec<(DirFd, PathBuf)> = vec![(dir_fd, dir_path.to_path_buf())]; // 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); + while let Some((dir_fd, dir_path)) = stack.pop() { + let entries = dir_fd.read_dir()?; - let dir_meta = dir_fd.metadata_at(&entry_name, should_follow_symlink); - 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.to_string_lossy().to_string()).into() - } else { - e.into() + 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); + 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.to_string_lossy().to_string()) + .into() + } else { + e.into() + }; + r = r.and(Err(error)); + continue; }; - r = r.and(Err(error)); - continue; - }; - if entry_path.is_symlink() { - r = self - .handle_symlink_during_safe_recursion(&entry_path, dir_fd, &entry_name) - .and(r); - } else { - // For regular files and directories, chmod them - r = self - .safe_chmod_file(&entry_path, dir_fd, &entry_name, meta.mode() & 0o7777) - .and(r); - - // Recurse into subdirectories using the existing directory fd - if meta.is_dir() { - match dir_fd.open_subdir(&entry_name) { - Ok(child_dir_fd) => { - r = self.safe_traverse_dir(&child_dir_fd, &entry_path).and(r); - } - Err(err) => { - let error = if err.kind() == std::io::ErrorKind::PermissionDenied { - ChmodError::PermissionDenied( - entry_path.to_string_lossy().to_string(), - ) - .into() - } else { - err.into() - }; - r = r.and(Err(error)); + if entry_path.is_symlink() { + r = self + .handle_symlink_during_safe_recursion(&entry_path, &dir_fd, &entry_name) + .and(r); + } else { + // For regular files and directories, chmod them + r = self + .safe_chmod_file(&entry_path, &dir_fd, &entry_name, meta.mode() & 0o7777) + .and(r); + + // Queue subdirectories; parent dir_fd can be dropped before processing children. + if meta.is_dir() { + match dir_fd.open_subdir(&entry_name) { + Ok(child_dir_fd) => { + stack.push((child_dir_fd, entry_path.clone())); + } + Err(err) => { + let error = if err.kind() == std::io::ErrorKind::PermissionDenied { + ChmodError::PermissionDenied( + entry_path.to_string_lossy().to_string(), + ) + .into() + } else { + err.into() + }; + r = r.and(Err(error)); + } } } } } + // dir_fd is dropped here, releasing its file descriptor before the next depth step. } r } diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index f6a73cc965b..1c39a1255f6 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -33,6 +33,9 @@ use std::os::unix::fs::MetadataExt; use std::os::unix::ffi::OsStrExt; use std::path::{MAIN_SEPARATOR, Path}; +#[cfg(target_os = "linux")] +use std::path::PathBuf; + /// The various level of verbosity #[derive(PartialEq, Eq, Clone, Debug)] pub enum VerbosityLevel { @@ -451,42 +454,25 @@ impl ChownExecutor { }; let mut ret = 0; - self.safe_traverse_dir(&dir_fd, root, &mut ret); + self.safe_traverse_dir(dir_fd, root, &mut ret); ret } #[cfg(target_os = "linux")] - fn safe_traverse_dir(&self, dir_fd: &DirFd, dir_path: &Path, ret: &mut i32) { - // Read directory entries - let entries = match dir_fd.read_dir() { - Ok(entries) => entries, - Err(e) => { - *ret = 1; - if self.verbosity.level != VerbosityLevel::Silent { - show_error!( - "cannot read directory '{}': {}", - dir_path.display(), - strip_errno(&e) - ); - } - return; - } - }; - - for entry_name in entries { - let entry_path = dir_path.join(&entry_name); - - // Get metadata for the entry - let follow = self.traverse_symlinks == TraverseSymlinks::All; - - let meta = match dir_fd.metadata_at(&entry_name, follow) { - Ok(m) => m, + fn safe_traverse_dir(&self, dir_fd: DirFd, dir_path: &Path, ret: &mut i32) { + // Iterative depth-first traversal to avoid stacking directory FDs. + let mut stack: Vec<(DirFd, PathBuf)> = vec![(dir_fd, dir_path.to_path_buf())]; + + while let Some((dir_fd, dir_path)) = stack.pop() { + // Read directory entries + let entries = match dir_fd.read_dir() { + Ok(entries) => entries, Err(e) => { *ret = 1; if self.verbosity.level != VerbosityLevel::Silent { show_error!( - "cannot access '{}': {}", - entry_path.display(), + "cannot read directory '{}': {}", + dir_path.display(), strip_errno(&e) ); } @@ -494,69 +480,93 @@ impl ChownExecutor { } }; - if self.preserve_root - && is_root(&entry_path, self.traverse_symlinks == TraverseSymlinks::All) - { - *ret = 1; - return; - } + for entry_name in entries { + let entry_path = dir_path.join(&entry_name); - // Check if we should chown this entry - if self.matched(meta.uid(), meta.gid()) { - // Use fchownat for the actual ownership change - let follow_symlinks = - self.dereference || self.traverse_symlinks == TraverseSymlinks::All; + // Get metadata for the entry + let follow = self.traverse_symlinks == TraverseSymlinks::All; - // Only pass the IDs that should actually be changed - let chown_uid = self.dest_uid; - let chown_gid = self.dest_gid; + let meta = match dir_fd.metadata_at(&entry_name, follow) { + Ok(m) => m, + Err(e) => { + *ret = 1; + if self.verbosity.level != VerbosityLevel::Silent { + show_error!( + "cannot access '{}': {}", + entry_path.display(), + strip_errno(&e) + ); + } + continue; + } + }; - if let Err(e) = dir_fd.chown_at(&entry_name, chown_uid, chown_gid, follow_symlinks) + if self.preserve_root + && is_root(&entry_path, self.traverse_symlinks == TraverseSymlinks::All) { *ret = 1; - if self.verbosity.level != VerbosityLevel::Silent { - let msg = format!( - "changing {} of {}: {}", - if self.verbosity.groups_only { - "group" - } else { - "ownership" - }, - entry_path.quote(), - strip_errno(&e) - ); - show_error!("{}", msg); - } - } else { - // Report the successful ownership change using the shared helper - self.report_ownership_change_success(&entry_path, meta.uid(), meta.gid()); + return; } - } else { - self.print_verbose_ownership_retained_as( - &entry_path, - meta.uid(), - self.dest_gid.map(|_| meta.gid()), - ); - } - // Recurse into subdirectories - if meta.is_dir() && (follow || !meta.file_type().is_symlink()) { - match dir_fd.open_subdir(&entry_name) { - Ok(subdir_fd) => { - self.safe_traverse_dir(&subdir_fd, &entry_path, ret); - } - Err(e) => { + // Check if we should chown this entry + if self.matched(meta.uid(), meta.gid()) { + // Use fchownat for the actual ownership change + let follow_symlinks = + self.dereference || self.traverse_symlinks == TraverseSymlinks::All; + + // Only pass the IDs that should actually be changed + let chown_uid = self.dest_uid; + let chown_gid = self.dest_gid; + + if let Err(e) = + dir_fd.chown_at(&entry_name, chown_uid, chown_gid, follow_symlinks) + { *ret = 1; if self.verbosity.level != VerbosityLevel::Silent { - show_error!( - "cannot access '{}': {}", - entry_path.display(), + let msg = format!( + "changing {} of {}: {}", + if self.verbosity.groups_only { + "group" + } else { + "ownership" + }, + entry_path.quote(), strip_errno(&e) ); + show_error!("{}", msg); + } + } else { + // Report the successful ownership change using the shared helper + self.report_ownership_change_success(&entry_path, meta.uid(), meta.gid()); + } + } else { + self.print_verbose_ownership_retained_as( + &entry_path, + meta.uid(), + self.dest_gid.map(|_| meta.gid()), + ); + } + + // Recurse into subdirectories iteratively + if meta.is_dir() && (follow || !meta.file_type().is_symlink()) { + match dir_fd.open_subdir(&entry_name) { + Ok(subdir_fd) => { + stack.push((subdir_fd, entry_path.clone())); + } + Err(e) => { + *ret = 1; + if self.verbosity.level != VerbosityLevel::Silent { + show_error!( + "cannot access '{}': {}", + entry_path.display(), + strip_errno(&e) + ); + } } } } } + // dir_fd dropped here, releasing the descriptor before descending further } } diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index e4d4b028474..2b4f153bee5 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -2,7 +2,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 +// spell-checker:ignore (words) dirfd subdirs openat FDCWD NOFILE getrlimit setrlimit rlim use std::fs::{OpenOptions, Permissions, metadata, set_permissions}; use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; @@ -1326,6 +1326,34 @@ fn test_chmod_recursive_uses_dirfd_for_subdirs() { ); } +#[cfg(unix)] +#[test] +fn test_chmod_recursive_does_not_exhaust_fds() { + use rlimit::Resource; + use std::path::PathBuf; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // Build a deep single-branch directory tree + let depth = 256; + let mut current = PathBuf::from("deep"); + at.mkdir(¤t); + for _ in 0..depth { + current.push("d"); + at.mkdir(¤t); + } + + // Constrain NOFILE only for the child process under test + scene + .ucmd() + .limit(Resource::NOFILE, 64, 64) + .arg("-R") + .arg("777") + .arg("deep") + .succeeds(); +} + #[test] fn test_chmod_colored_output() { // Test colored help message