Skip to content

chmod:fix file descriptor limits issue #9595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mattsu2020 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chmod:fix file descriptor limits issue #9595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b91ef7
perf(chmod): optimize directory traversal to avoid fd stacking on Linux
mattsu2020 Dec 6, 2025
4566b07
refactor: simplify FD limit handling in chmod recursive test
mattsu2020 Dec 6, 2025
7f19b0c
refactor(chmod): remove redundant std::path:: qualifier from PathBuf …
mattsu2020 Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 57 additions & 44 deletions src/uu/chmod/src/chmod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand Down
162 changes: 86 additions & 76 deletions src/uucore/src/lib/features/perms.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -451,112 +454,119 @@ 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)
);
}
continue;
}
};

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
}
}

Expand Down
30 changes: 29 additions & 1 deletion tests/by-util/test_chmod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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(&current);
for _ in 0..depth {
current.push("d");
at.mkdir(&current);
}

// 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
Expand Down
Loading