From 3b91ef7740913827636ec73868ff2588d40be17e Mon Sep 17 00:00:00 2001 From: mattsu Date: Sat, 6 Dec 2025 19:49:12 +0900 Subject: [PATCH 1/3] perf(chmod): optimize directory traversal to avoid fd stacking on Linux Replace recursive safe_traverse_dir with iterative depth-first search using a stack to prevent file descriptor exhaustion during deep directory hierarchies. Also collect read_dir entries upfront to release the directory iterator early, reducing resource usage. --- src/uu/chmod/src/chmod.rs | 101 +++++++++-------- src/uucore/src/lib/features/perms.rs | 162 ++++++++++++++------------- tests/by-util/test_chmod.rs | 45 ++++++++ 3 files changed, 188 insertions(+), 120 deletions(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 15b608af6b2..edd8b219463 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, std::path::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..ceff8efb4b6 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, std::path::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..7874d09ec7c 100644 --- a/tests/by-util/test_chmod.rs +++ b/tests/by-util/test_chmod.rs @@ -1326,6 +1326,51 @@ fn test_chmod_recursive_uses_dirfd_for_subdirs() { ); } +#[cfg(unix)] +#[test] +fn test_chmod_recursive_does_not_exhaust_fds() { + use libc::{RLIMIT_NOFILE, getrlimit, rlimit, setrlimit}; + use std::path::PathBuf; + + // Lower soft limit so deep recursion would fail if descriptors stack up + let mut old = rlimit { + rlim_cur: 0, + rlim_max: 0, + }; + unsafe { assert_eq!(0, getrlimit(RLIMIT_NOFILE, &mut old)) }; + + let new_soft = old.rlim_max.min(64); + struct Restore(rlimit); + impl Drop for Restore { + fn drop(&mut self) { + unsafe { + setrlimit(RLIMIT_NOFILE, &self.0); + } + } + } + let _restore = Restore(old); + + let lowered = rlimit { + rlim_cur: new_soft, + rlim_max: old.rlim_max, + }; + unsafe { assert_eq!(0, setrlimit(RLIMIT_NOFILE, &lowered)) }; + + 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); + } + + scene.ucmd().arg("-R").arg("777").arg("deep").succeeds(); +} + #[test] fn test_chmod_colored_output() { // Test colored help message From 4566b072b497cb3cb84f58ed44b6a15a421c73f2 Mon Sep 17 00:00:00 2001 From: mattsu Date: Sat, 6 Dec 2025 20:57:09 +0900 Subject: [PATCH 2/3] refactor: simplify FD limit handling in chmod recursive test Use rlimit crate's .limit() method instead of manual libc rlimit calls for constraining NOFILE. This cleans up the test code and leverages existing utilities in the rlimit crate. Additionally, optimize import by using PathBuf directly from std::path. --- src/uucore/src/lib/features/perms.rs | 2 +- tests/by-util/test_chmod.rs | 37 ++++++++-------------------- 2 files changed, 11 insertions(+), 28 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index ceff8efb4b6..1c39a1255f6 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -461,7 +461,7 @@ impl ChownExecutor { #[cfg(target_os = "linux")] 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, std::path::PathBuf)> = vec![(dir_fd, dir_path.to_path_buf())]; + 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 diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index 7874d09ec7c..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}; @@ -1329,33 +1329,9 @@ fn test_chmod_recursive_uses_dirfd_for_subdirs() { #[cfg(unix)] #[test] fn test_chmod_recursive_does_not_exhaust_fds() { - use libc::{RLIMIT_NOFILE, getrlimit, rlimit, setrlimit}; + use rlimit::Resource; use std::path::PathBuf; - // Lower soft limit so deep recursion would fail if descriptors stack up - let mut old = rlimit { - rlim_cur: 0, - rlim_max: 0, - }; - unsafe { assert_eq!(0, getrlimit(RLIMIT_NOFILE, &mut old)) }; - - let new_soft = old.rlim_max.min(64); - struct Restore(rlimit); - impl Drop for Restore { - fn drop(&mut self) { - unsafe { - setrlimit(RLIMIT_NOFILE, &self.0); - } - } - } - let _restore = Restore(old); - - let lowered = rlimit { - rlim_cur: new_soft, - rlim_max: old.rlim_max, - }; - unsafe { assert_eq!(0, setrlimit(RLIMIT_NOFILE, &lowered)) }; - let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; @@ -1368,7 +1344,14 @@ fn test_chmod_recursive_does_not_exhaust_fds() { at.mkdir(¤t); } - scene.ucmd().arg("-R").arg("777").arg("deep").succeeds(); + // Constrain NOFILE only for the child process under test + scene + .ucmd() + .limit(Resource::NOFILE, 64, 64) + .arg("-R") + .arg("777") + .arg("deep") + .succeeds(); } #[test] From 7f19b0cc7da4915048a2bfd426a95af9298c4846 Mon Sep 17 00:00:00 2001 From: mattsu Date: Tue, 9 Dec 2025 18:45:00 +0900 Subject: [PATCH 3/3] refactor(chmod): remove redundant std::path:: qualifier from PathBuf type Use implicit import for PathBuf to simplify code readability. --- src/uu/chmod/src/chmod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index edd8b219463..7644dd1a586 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -498,7 +498,7 @@ impl Chmoder { fn safe_traverse_dir(&self, dir_fd: DirFd, dir_path: &Path) -> UResult<()> { let mut r = Ok(()); // Depth-first traversal without recursive calls to avoid stacking FDs. - let mut stack: Vec<(DirFd, std::path::PathBuf)> = vec![(dir_fd, dir_path.to_path_buf())]; + 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;