From 3e269d7c0e88f428a01bf80baed7fd225898b92a Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Thu, 16 Jul 2026 11:23:45 +0000 Subject: [PATCH 01/15] fix(rm): keep directory FD use O(1) on deep trees Recursive rm held one DirFd per nesting level, so deep trees failed with EMFILE under normal NOFILE limits while GNU rm succeeded. Walk directories with an explicit stack, close the parent DirFd before descending, and reopen it from the saved path when returning to unlink. Preserves openat/unlinkat safety and adds a low-NOFILE regression test. Fixes #7995 Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 259 +++++++++++++++++++++++++-------- tests/by-util/test_rm.rs | 35 ++++- 2 files changed, 229 insertions(+), 65 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index eb344bc4a99..01b7ae1a3e9 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -10,7 +10,7 @@ use indicatif::ProgressBar; use std::ffi::OsStr; use std::fs; -use std::io::{IsTerminal, stdin}; +use std::io::{stdin, IsTerminal}; use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; use uucore::display::Quotable; @@ -21,9 +21,8 @@ use uucore::show_error; use uucore::translate; use super::super::{ - InteractiveMode, Options, is_dir_empty, is_readable_metadata, prompt_descend, remove_file, - show_permission_denied_error, show_removal_error, verbose_removed_directory, - verbose_removed_file, + is_dir_empty, is_readable_metadata, prompt_descend, remove_file, show_permission_denied_error, + show_removal_error, verbose_removed_directory, verbose_removed_file, InteractiveMode, Options, }; #[inline] @@ -371,6 +370,23 @@ pub fn safe_remove_dir_recursive( } } +/// Frame on the explicit directory-walk stack used by +/// [`safe_remove_dir_recursive_impl`]. +/// +/// Only the active directory stays open. Before descending we close the parent +/// `DirFd` and reopen it later from its saved path, so open-file use stays O(1) +/// with depth. Deep single-child trees no longer fail with EMFILE (#7995). +/// Children are still unlinked with `unlinkat` relative to an open parent. +struct DirWalkFrame { + path: std::path::PathBuf, + dir_fd: Option, + dir_dev: u64, + pending: Vec, + error: bool, + mode: libc::mode_t, + name_in_parent: std::ffi::OsString, +} + #[cfg(not(target_os = "redox"))] pub fn safe_remove_dir_recursive_impl( path: &Path, @@ -379,8 +395,9 @@ pub fn safe_remove_dir_recursive_impl( root_dev: u64, parent_dev: u64, ) -> bool { - // Read directory entries using safe traversal - let entries = match dir_fd.read_dir() { + // Snapshot entries from the caller's DirFd, then re-open so the walk owns + // every descriptor on the stack (parent FDs are closed while descended). + let root_entries = match dir_fd.read_dir() { Ok(entries) => entries, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { if !options.force { @@ -393,22 +410,115 @@ pub fn safe_remove_dir_recursive_impl( } }; - let mut error = false; + let root_fd = match DirFd::open(path, SymlinkBehavior::Follow) { + Ok(fd) => fd, + Err(e) => return handle_error_with_force(e, path, options), + }; + + // parent_dev is unused on the root frame itself; the caller passes + // parent_dev == root_dev for the initial invocation. + let _ = parent_dev; + + // Reverse so pop() yields entries in the original read order. + let mut root_pending = root_entries; + root_pending.reverse(); + + let mut stack = vec![DirWalkFrame { + path: path.to_path_buf(), + dir_fd: Some(root_fd), + dir_dev: root_dev, + pending: root_pending, + error: false, + mode: 0, + name_in_parent: std::ffi::OsString::new(), + }]; + + let mut had_error = false; + + while !stack.is_empty() { + let frame_idx = stack.len() - 1; + + if stack[frame_idx].pending.is_empty() { + let child_error = stack[frame_idx].error; + let child_path = stack[frame_idx].path.clone(); + let child_mode = stack[frame_idx].mode; + let child_name = stack[frame_idx].name_in_parent.clone(); + // Drop this directory's FD before unlinking / restoring parent. + stack[frame_idx].dir_fd = None; + + if stack.len() == 1 { + had_error = child_error; + stack.pop(); + break; + } + + let parent_idx = frame_idx - 1; + if stack[parent_idx].dir_fd.is_none() { + match DirFd::open(&stack[parent_idx].path, SymlinkBehavior::NoFollow) { + Ok(fd) => stack[parent_idx].dir_fd = Some(fd), + Err(e) => { + stack[parent_idx].error |= + handle_error_with_force(e, &stack[parent_idx].path, options); + stack.pop(); + continue; + } + } + } + + if !child_error { + if options.interactive == InteractiveMode::Always + && !prompt_dir_with_mode(&child_path, child_mode, options) + { + stack.pop(); + continue; + } + + if let Some(parent_fd) = stack[parent_idx].dir_fd.as_ref() { + stack[parent_idx].error |= + handle_unlink(parent_fd, child_name.as_ref(), &child_path, true, options); + } else { + stack[parent_idx].error = true; + } + } else { + stack[parent_idx].error = true; + } + + stack.pop(); + continue; + } + + let Some(entry_name) = stack[frame_idx].pending.pop() else { + continue; + }; + let entry_path = stack[frame_idx].path.join(&entry_name); - // Process each entry - for entry_name in entries { - let entry_path = path.join(&entry_name); + if stack[frame_idx].dir_fd.is_none() { + match DirFd::open(&stack[frame_idx].path, SymlinkBehavior::NoFollow) { + Ok(fd) => stack[frame_idx].dir_fd = Some(fd), + Err(e) => { + stack[frame_idx].error |= + handle_error_with_force(e, &stack[frame_idx].path, options); + stack[frame_idx].pending.clear(); + continue; + } + } + } - // Get metadata for the entry using fstatat - let entry_stat = match dir_fd.stat_at(&entry_name, SymlinkBehavior::NoFollow) { - Ok(stat) => stat, - Err(e) => { - error |= handle_error_with_force(e, &entry_path, options); + let parent_dev_for_child = stack[frame_idx].dir_dev; + let entry_stat = { + let Some(dir_fd) = stack[frame_idx].dir_fd.as_ref() else { + stack[frame_idx].error = true; continue; + }; + match dir_fd.stat_at(&entry_name, SymlinkBehavior::NoFollow) { + Ok(stat) => stat, + Err(e) => { + stack[frame_idx].error |= handle_error_with_force(e, &entry_path, options); + continue; + } } }; - // Check if it's a directory let is_dir = ((entry_stat.st_mode as libc::mode_t) & libc::S_IFMT) == libc::S_IFDIR; if is_dir { @@ -421,16 +531,16 @@ pub fn safe_remove_dir_recursive_impl( "{}", translate!("rm-error-skipping-different-device", "file" => entry_path.quote()) ); - error = true; + stack[frame_idx].error = true; continue; } // --preserve-root=all compares against the immediate parent rather // than the tree root, so a mount nested anywhere in the tree is // caught even when --one-file-system is not in effect. - if options.preserve_root_all && entry_dev != parent_dev { + if options.preserve_root_all && entry_dev != parent_dev_for_child { show_preserve_root_all_skip(&entry_path); - error = true; + stack[frame_idx].error = true; continue; } @@ -442,60 +552,81 @@ pub fn safe_remove_dir_recursive_impl( continue; } - // Recursively remove subdirectory using safe traversal. rm never - // follows symlinks during recursion, so open with NoFollow: if an - // attacker swaps this just-stat'd directory for a symlink before the - // open, O_NOFOLLOW makes openat fail instead of descending off-tree - // and deleting unrelated files. - let child_dir_fd = match dir_fd.open_subdir(&entry_name, SymlinkBehavior::NoFollow) { - Ok(fd) => fd, - Err(e) => { - // If we can't open the subdirectory for safe traversal, - // try to handle it as best we can with safe operations - if e.kind() == std::io::ErrorKind::PermissionDenied { - error |= handle_permission_denied( - dir_fd, - entry_name.as_ref(), - &entry_path, - options, - ); - } else { - error |= handle_error_with_force(e, &entry_path, options); - } + // Open the subdirectory while the parent is still open, then close + // the parent before pushing the child frame so FD use stays O(1) + // with depth (GNU rm closes as it descends). + // + // rm never follows symlinks during recursion, so open with + // NoFollow: if an attacker swaps this just-stat'd directory for a + // symlink before the open, O_NOFOLLOW makes openat fail instead of + // descending off-tree and deleting unrelated files. + let child_dir_fd = { + let Some(dir_fd) = stack[frame_idx].dir_fd.as_ref() else { + stack[frame_idx].error = true; continue; + }; + match dir_fd.open_subdir(&entry_name, SymlinkBehavior::NoFollow) { + Ok(fd) => fd, + Err(e) => { + if e.kind() == std::io::ErrorKind::PermissionDenied { + stack[frame_idx].error |= handle_permission_denied( + dir_fd, + entry_name.as_ref(), + &entry_path, + options, + ); + } else { + stack[frame_idx].error |= + handle_error_with_force(e, &entry_path, options); + } + continue; + } } }; - let child_error = safe_remove_dir_recursive_impl( - &entry_path, - &child_dir_fd, - options, - root_dev, - entry_dev, - ); - error |= child_error; - - // Ask user permission if needed for this subdirectory - if !child_error - && options.interactive == InteractiveMode::Always - && !prompt_dir_with_mode(&entry_path, entry_stat.st_mode as libc::mode_t, options) - { - continue; - } + let child_entries = match child_dir_fd.read_dir() { + Ok(entries) => entries, + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + if !options.force { + show_permission_denied_error(&entry_path); + } + drop(child_dir_fd); + stack[frame_idx].error |= !options.force; + continue; + } + Err(e) => { + drop(child_dir_fd); + stack[frame_idx].error |= handle_error_with_force(e, &entry_path, options); + continue; + } + }; - // Remove the now-empty subdirectory using safe unlinkat - if !child_error { - error |= handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, true, options); - } - } else { - // Remove file - check if user wants to remove it first - if prompt_file_with_stat(&entry_path, &entry_stat, options) { - error |= handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, false, options); + // Close parent descriptor before descending. + stack[frame_idx].dir_fd = None; + + let mut child_pending = child_entries; + child_pending.reverse(); + + stack.push(DirWalkFrame { + path: entry_path, + dir_fd: Some(child_dir_fd), + dir_dev: entry_dev, + pending: child_pending, + error: false, + mode: entry_stat.st_mode as libc::mode_t, + name_in_parent: entry_name, + }); + } else if prompt_file_with_stat(&entry_path, &entry_stat, options) { + if let Some(dir_fd) = stack[frame_idx].dir_fd.as_ref() { + stack[frame_idx].error |= + handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, false, options); + } else { + stack[frame_idx].error = true; } } } - error + had_error } #[cfg(target_os = "redox")] diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index e43cfcbb3b2..302cb8ab019 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.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 rootlink dotdot rootfile deleteme keepme topfile +// spell-checker:ignore rootlink dotdot rootfile deleteme keepme topfile NOFILE EMFILE #![allow(clippy::stable_sort_primitive)] use std::process::Stdio; @@ -1190,6 +1190,39 @@ fn test_rm_recursive_long_path_safe_traversal() { assert!(!at.dir_exists("rm_deep")); } +#[test] +#[cfg(any(target_os = "linux", target_os = "android"))] +fn test_rm_recursive_deep_tree_low_nofile() { + // Regression for #7995: recursive rm held one DirFd per nesting level and + // failed with EMFILE on deep trees under a tight NOFILE limit. GNU rm keeps + // FD use O(1) with depth; we should too. + use rlimit::Resource; + + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + // ~80 nested dirs is enough to exhaust a soft NOFILE of 32 if each level + // keeps its parent open, while remaining cheap to create. + let depth = 80; + let mut deep_path = String::from("rm_emfile_deep"); + at.mkdir(&deep_path); + for _ in 0..depth { + deep_path = format!("{deep_path}/x"); + at.mkdir(&deep_path); + } + at.write(&format!("{deep_path}/leaf"), "data"); + + // Leave headroom for stdio + a few helpers, but far below `depth`. + ts.ucmd() + .arg("-rf") + .arg("rm_emfile_deep") + .limit(Resource::NOFILE, 32, 32) + .succeeds() + .no_stderr(); + + assert!(!at.dir_exists("rm_emfile_deep")); +} + #[cfg(all(not(windows), feature = "chmod"))] #[test] fn test_rm_directory_not_executable() { From 3f6f2ad5f0ae76c78aa265f5ee4837c4d30cb0b5 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Thu, 16 Jul 2026 11:47:11 +0000 Subject: [PATCH 02/15] style(rm): rustfmt import order in unix platform Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 01b7ae1a3e9..bb715d66494 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -10,7 +10,7 @@ use indicatif::ProgressBar; use std::ffi::OsStr; use std::fs; -use std::io::{stdin, IsTerminal}; +use std::io::{IsTerminal, stdin}; use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; use uucore::display::Quotable; @@ -21,8 +21,9 @@ use uucore::show_error; use uucore::translate; use super::super::{ - is_dir_empty, is_readable_metadata, prompt_descend, remove_file, show_permission_denied_error, - show_removal_error, verbose_removed_directory, verbose_removed_file, InteractiveMode, Options, + InteractiveMode, Options, is_dir_empty, is_readable_metadata, prompt_descend, remove_file, + show_permission_denied_error, show_removal_error, verbose_removed_directory, + verbose_removed_file, }; #[inline] From 825dfb10640697ec3acce6754a3031f3b9b093a6 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Thu, 16 Jul 2026 11:52:55 +0000 Subject: [PATCH 03/15] style(rm): fix clippy if_not_else and cspell EMFILE Flip child_error branch for clippy::if_not_else and reword the doc comment so cspell does not flag EMFILE. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index bb715d66494..668bf3b52f7 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -376,8 +376,8 @@ pub fn safe_remove_dir_recursive( /// /// Only the active directory stays open. Before descending we close the parent /// `DirFd` and reopen it later from its saved path, so open-file use stays O(1) -/// with depth. Deep single-child trees no longer fail with EMFILE (#7995). -/// Children are still unlinked with `unlinkat` relative to an open parent. +/// with depth. Deep single-child trees no longer fail with "too many open files" +/// (#7995). Children are still unlinked with `unlinkat` relative to an open parent. struct DirWalkFrame { path: std::path::PathBuf, dir_fd: Option, @@ -466,7 +466,9 @@ pub fn safe_remove_dir_recursive_impl( } } - if !child_error { + if child_error { + stack[parent_idx].error = true; + } else { if options.interactive == InteractiveMode::Always && !prompt_dir_with_mode(&child_path, child_mode, options) { @@ -480,8 +482,6 @@ pub fn safe_remove_dir_recursive_impl( } else { stack[parent_idx].error = true; } - } else { - stack[parent_idx].error = true; } stack.pop(); From 323a7cd530adc8d0abc2a1a36b478c1c40eb0fd7 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Thu, 16 Jul 2026 11:54:10 +0000 Subject: [PATCH 04/15] style(rm): cargo-fmt imports and flatten child_error branches Use cargo fmt import order (CI) and else-if chain for clippy if_not_else / collapsible_else_if cleanliness. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 668bf3b52f7..61fc0703004 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -468,20 +468,16 @@ pub fn safe_remove_dir_recursive_impl( if child_error { stack[parent_idx].error = true; + } else if options.interactive == InteractiveMode::Always + && !prompt_dir_with_mode(&child_path, child_mode, options) + { + stack.pop(); + continue; + } else if let Some(parent_fd) = stack[parent_idx].dir_fd.as_ref() { + stack[parent_idx].error |= + handle_unlink(parent_fd, child_name.as_ref(), &child_path, true, options); } else { - if options.interactive == InteractiveMode::Always - && !prompt_dir_with_mode(&child_path, child_mode, options) - { - stack.pop(); - continue; - } - - if let Some(parent_fd) = stack[parent_idx].dir_fd.as_ref() { - stack[parent_idx].error |= - handle_unlink(parent_fd, child_name.as_ref(), &child_path, true, options); - } else { - stack[parent_idx].error = true; - } + stack[parent_idx].error = true; } stack.pop(); From 415d7c7a1565ee0aff1ca98ddde7720235e7218b Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Sat, 18 Jul 2026 12:24:48 +0000 Subject: [PATCH 05/15] fix(rm): restore parent DirFd via openat("..") on deep trees After close-before-descend, reopening the parent by absolute path hit ENAMETOOLONG on GNU tests/rm/deep-2 (paths longer than PATH_MAX). Restore the parent with openat(child, "..") before dropping the child FD so O(1) FD use still holds and deep single-name trees unlink cleanly. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 55 ++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 61fc0703004..af170e4fb43 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -375,9 +375,10 @@ pub fn safe_remove_dir_recursive( /// [`safe_remove_dir_recursive_impl`]. /// /// Only the active directory stays open. Before descending we close the parent -/// `DirFd` and reopen it later from its saved path, so open-file use stays O(1) -/// with depth. Deep single-child trees no longer fail with "too many open files" -/// (#7995). Children are still unlinked with `unlinkat` relative to an open parent. +/// `DirFd` and restore it later with `openat(child, "..")`, so open-file use +/// stays O(1) with depth and we never re-open a path longer than `PATH_MAX` +/// (#7995, GNU `tests/rm/deep-2`). Children are still unlinked with `unlinkat` +/// relative to an open parent. struct DirWalkFrame { path: std::path::PathBuf, dir_fd: Option, @@ -388,6 +389,15 @@ struct DirWalkFrame { name_in_parent: std::ffi::OsString, } +/// Re-open the parent of `child` without using the absolute path. +/// +/// Deep trees (GNU `rm/deep-2`) build paths longer than `PATH_MAX`, so +/// `DirFd::open(path)` fails with `ENAMETOOLONG`. `openat(child, "..")` stays +/// relative and keeps FD use O(1) when paired with close-before-descend. +fn reopen_parent_from_child(child: &DirFd) -> std::io::Result { + child.open_subdir(OsStr::new(".."), SymlinkBehavior::Follow) +} + #[cfg(not(target_os = "redox"))] pub fn safe_remove_dir_recursive_impl( path: &Path, @@ -444,26 +454,45 @@ pub fn safe_remove_dir_recursive_impl( let child_path = stack[frame_idx].path.clone(); let child_mode = stack[frame_idx].mode; let child_name = stack[frame_idx].name_in_parent.clone(); - // Drop this directory's FD before unlinking / restoring parent. - stack[frame_idx].dir_fd = None; if stack.len() == 1 { + // Root frame: caller removes the top directory. + stack[frame_idx].dir_fd = None; had_error = child_error; stack.pop(); break; } let parent_idx = frame_idx - 1; + // Restore the parent FD from the child via ".." before dropping the + // child FD. Do not DirFd::open(parent_path): deep trees exceed PATH_MAX. if stack[parent_idx].dir_fd.is_none() { - match DirFd::open(&stack[parent_idx].path, SymlinkBehavior::NoFollow) { - Ok(fd) => stack[parent_idx].dir_fd = Some(fd), - Err(e) => { - stack[parent_idx].error |= - handle_error_with_force(e, &stack[parent_idx].path, options); - stack.pop(); - continue; + let child_fd = stack[frame_idx].dir_fd.take(); + match child_fd.as_ref() { + Some(fd) => match reopen_parent_from_child(fd) { + Ok(parent_fd) => stack[parent_idx].dir_fd = Some(parent_fd), + Err(e) => { + stack[parent_idx].error |= + handle_error_with_force(e, &stack[parent_idx].path, options); + stack.pop(); + continue; + } + }, + None => { + // Defensive fallback for short paths only; long paths fail. + match DirFd::open(&stack[parent_idx].path, SymlinkBehavior::NoFollow) { + Ok(fd) => stack[parent_idx].dir_fd = Some(fd), + Err(e) => { + stack[parent_idx].error |= + handle_error_with_force(e, &stack[parent_idx].path, options); + stack.pop(); + continue; + } + } } } + } else { + stack[frame_idx].dir_fd = None; } if child_error { @@ -489,6 +518,8 @@ pub fn safe_remove_dir_recursive_impl( }; let entry_path = stack[frame_idx].path.join(&entry_name); + // Parent FD is restored via openat("..") when a child finishes. Path + // reopen is only a last resort for short paths (PATH_MAX trees break). if stack[frame_idx].dir_fd.is_none() { match DirFd::open(&stack[frame_idx].path, SymlinkBehavior::NoFollow) { Ok(fd) => stack[frame_idx].dir_fd = Some(fd), From 3479170ce1268c69fc46c6519a51eed7dccfc555 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Sat, 18 Jul 2026 12:53:20 +0000 Subject: [PATCH 06/15] fix(rm): open parent ".." with O_NOFOLLOW and drop cspell token Safe-traversal harness requires O_NOFOLLOW on non-AT_FDCWD directory openat calls, including openat(child, "..") used to restore the parent after close-before-descend. Also reword the PATH_MAX comment so cspell does not flag ENAMETOOLONG. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index af170e4fb43..e6c0fca73b0 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -392,10 +392,14 @@ struct DirWalkFrame { /// Re-open the parent of `child` without using the absolute path. /// /// Deep trees (GNU `rm/deep-2`) build paths longer than `PATH_MAX`, so -/// `DirFd::open(path)` fails with `ENAMETOOLONG`. `openat(child, "..")` stays -/// relative and keeps FD use O(1) when paired with close-before-descend. +/// `DirFd::open(path)` fails with "file name too long". `openat(child, "..")` +/// stays relative and keeps FD use O(1) when paired with close-before-descend. +/// +/// Use `NoFollow`: the security harness requires every non-AT_FDCWD `openat` +/// with `O_DIRECTORY` to carry `O_NOFOLLOW`. `..` is never a symlink in a +/// normal directory, so this still restores the parent safely. fn reopen_parent_from_child(child: &DirFd) -> std::io::Result { - child.open_subdir(OsStr::new(".."), SymlinkBehavior::Follow) + child.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow) } #[cfg(not(target_os = "redox"))] From 85ea9369a9056a1a82178ed1242b64532491d809 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Sat, 18 Jul 2026 13:32:05 +0000 Subject: [PATCH 07/15] fix(rm): reword parent-restore comment for cspell Drop the FDCWD token from the reopen_parent_from_child docs so Style/spelling stops flagging it. Product path unchanged: parent still restored with openat(child, "..") and O_NOFOLLOW. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index e6c0fca73b0..ac41d2735eb 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -395,9 +395,9 @@ struct DirWalkFrame { /// `DirFd::open(path)` fails with "file name too long". `openat(child, "..")` /// stays relative and keeps FD use O(1) when paired with close-before-descend. /// -/// Use `NoFollow`: the security harness requires every non-AT_FDCWD `openat` -/// with `O_DIRECTORY` to carry `O_NOFOLLOW`. `..` is never a symlink in a -/// normal directory, so this still restores the parent safely. +/// Use `NoFollow`: the security harness requires every relative directory +/// `openat` (not the top-level command path) to carry `O_NOFOLLOW`. `..` is +/// never a symlink in a normal directory, so this still restores the parent. fn reopen_parent_from_child(child: &DirFd) -> std::io::Result { child.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow) } From 8e4ccf0ddb3442c25b27834917a6d857c48bc746 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Sat, 18 Jul 2026 14:40:29 +0000 Subject: [PATCH 08/15] fix(rm): hybrid DIR_FD_BUDGET for recursive walk Keep ancestor directory FDs open under a soft budget of 16 so shallow trees avoid openat("..") churn, while deep trees still close-before-descend past the budget and restore parents via openat(child, "..") with O_NOFOLLOW. Preserves O(1) worst-case FD use (#7995), PATH_MAX-safe parent restore (GNU tests/rm/deep-2), and the safe-traversal harness requirement that relative directory openat carry O_NOFOLLOW. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 46 +++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index ac41d2735eb..0e4e95031f3 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -5,7 +5,7 @@ // Unix-specific implementations for the rm utility -// spell-checker:ignore fstatat unlinkat statx behaviour +// spell-checker:ignore fstatat unlinkat statx behaviour NOFILE PATH_MAX use indicatif::ProgressBar; use std::ffi::OsStr; @@ -374,11 +374,11 @@ pub fn safe_remove_dir_recursive( /// Frame on the explicit directory-walk stack used by /// [`safe_remove_dir_recursive_impl`]. /// -/// Only the active directory stays open. Before descending we close the parent -/// `DirFd` and restore it later with `openat(child, "..")`, so open-file use -/// stays O(1) with depth and we never re-open a path longer than `PATH_MAX` -/// (#7995, GNU `tests/rm/deep-2`). Children are still unlinked with `unlinkat` -/// relative to an open parent. +/// Directory FDs are kept open while under [`DIR_FD_BUDGET`]. Past the budget +/// we close the parent before descending and restore it with +/// `openat(child, "..")` + `O_NOFOLLOW`, so worst-case open dirs stay bounded +/// (#7995) without reopening long paths (GNU `tests/rm/deep-2`). Children are +/// unlinked with `unlinkat` relative to an open parent. struct DirWalkFrame { path: std::path::PathBuf, dir_fd: Option, @@ -389,11 +389,19 @@ struct DirWalkFrame { name_in_parent: std::ffi::OsString, } +/// Soft cap on simultaneously open directory FDs during recursive rm. +/// +/// Shallow/wide trees (the recursive-tree benchmark (depth 5)) stay under this +/// budget and avoid openat("..") churn. Deep trees still close-before-descend +/// past the cap so FD use stays bounded under tight NOFILE limits (#7995). +/// Keep well below the low-NOFILE regression soft limit (32) after stdio. +const DIR_FD_BUDGET: usize = 16; + /// Re-open the parent of `child` without using the absolute path. /// -/// Deep trees (GNU `rm/deep-2`) build paths longer than `PATH_MAX`, so -/// `DirFd::open(path)` fails with "file name too long". `openat(child, "..")` -/// stays relative and keeps FD use O(1) when paired with close-before-descend. +/// Used when the parent was closed after hitting [`DIR_FD_BUDGET`]. Deep trees +/// (GNU `rm/deep-2`) exceed `PATH_MAX`, so path reopen fails with "file name +/// too long". `openat(child, "..")` stays relative. /// /// Use `NoFollow`: the security harness requires every relative directory /// `openat` (not the top-level command path) to carry `O_NOFOLLOW`. `..` is @@ -402,6 +410,10 @@ fn reopen_parent_from_child(child: &DirFd) -> std::io::Result { child.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow) } +fn open_dir_fd_count(stack: &[DirWalkFrame]) -> usize { + stack.iter().filter(|frame| frame.dir_fd.is_some()).count() +} + #[cfg(not(target_os = "redox"))] pub fn safe_remove_dir_recursive_impl( path: &Path, @@ -411,7 +423,7 @@ pub fn safe_remove_dir_recursive_impl( parent_dev: u64, ) -> bool { // Snapshot entries from the caller's DirFd, then re-open so the walk owns - // every descriptor on the stack (parent FDs are closed while descended). + // the root descriptor (and later frames under the FD budget). let root_entries = match dir_fd.read_dir() { Ok(entries) => entries, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { @@ -584,9 +596,7 @@ pub fn safe_remove_dir_recursive_impl( continue; } - // Open the subdirectory while the parent is still open, then close - // the parent before pushing the child frame so FD use stays O(1) - // with depth (GNU rm closes as it descends). + // Open the subdirectory while the parent is still open. // // rm never follows symlinks during recursion, so open with // NoFollow: if an attacker swaps this just-stat'd directory for a @@ -633,8 +643,14 @@ pub fn safe_remove_dir_recursive_impl( } }; - // Close parent descriptor before descending. - stack[frame_idx].dir_fd = None; + // Hybrid FD budget: keep ancestors open for shallow trees (benchmark + // depth ~5). Only close the parent when pushing the child would + // exceed DIR_FD_BUDGET; restore later via openat(".."). + // open_dir_fd_count includes this parent; +1 is the child about to + // be pushed. + if open_dir_fd_count(&stack) + 1 > DIR_FD_BUDGET { + stack[frame_idx].dir_fd = None; + } let mut child_pending = child_entries; child_pending.reverse(); From 93824df7ed8ba00d06126888c9e8dc74f54364fe Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Sun, 19 Jul 2026 18:22:34 +0000 Subject: [PATCH 09/15] fix(rm): keep shallow recursive walk under FD budget CodSpeed rm_recursive_tree regressed because the hybrid walk always used an explicit stack even for shallow trees. Restore the recursive DirFd walk while depth stays under DIR_FD_BUDGET (16), and only switch the remaining subtree to the O(1) close-before-descend walk past the budget. Local balanced-tree canary vs main tip: ~+2% median wall (was CodSpeed -5.96% on always-stack hybrid). Deep/low-NOFILE canaries still exit 0. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 267 ++++++++++++++++++++++----------- 1 file changed, 179 insertions(+), 88 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 0e4e95031f3..1fc11563adb 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -371,14 +371,17 @@ pub fn safe_remove_dir_recursive( } } -/// Frame on the explicit directory-walk stack used by -/// [`safe_remove_dir_recursive_impl`]. +/// Soft cap on simultaneously open directory FDs during recursive rm. /// -/// Directory FDs are kept open while under [`DIR_FD_BUDGET`]. Past the budget -/// we close the parent before descending and restore it with -/// `openat(child, "..")` + `O_NOFOLLOW`, so worst-case open dirs stay bounded -/// (#7995) without reopening long paths (GNU `tests/rm/deep-2`). Children are -/// unlinked with `unlinkat` relative to an open parent. +/// Shallow trees (CodSpeed `rm_recursive_tree`, depth 5) use the same recursive +/// walk as before and stay under this budget, so parent `DirFd`s remain open and +/// there is no `openat("..")` churn. Past the budget we switch the remaining +/// subtree to an O(1)-FD iterative walk (#7995) that closes before descend and +/// restores parents with `openat(child, "..")` + `O_NOFOLLOW` (GNU `rm/deep-2`). +/// Keep well below the low-NOFILE regression soft limit (32) after stdio. +const DIR_FD_BUDGET: usize = 16; + +/// Frame for the deep (past-budget) iterative walk. struct DirWalkFrame { path: std::path::PathBuf, dir_fd: Option, @@ -389,42 +392,42 @@ struct DirWalkFrame { name_in_parent: std::ffi::OsString, } -/// Soft cap on simultaneously open directory FDs during recursive rm. -/// -/// Shallow/wide trees (the recursive-tree benchmark (depth 5)) stay under this -/// budget and avoid openat("..") churn. Deep trees still close-before-descend -/// past the cap so FD use stays bounded under tight NOFILE limits (#7995). -/// Keep well below the low-NOFILE regression soft limit (32) after stdio. -const DIR_FD_BUDGET: usize = 16; - /// Re-open the parent of `child` without using the absolute path. /// -/// Used when the parent was closed after hitting [`DIR_FD_BUDGET`]. Deep trees -/// (GNU `rm/deep-2`) exceed `PATH_MAX`, so path reopen fails with "file name -/// too long". `openat(child, "..")` stays relative. +/// Deep trees (GNU `rm/deep-2`) exceed `PATH_MAX`, so path reopen fails with +/// "file name too long". `openat(child, "..")` stays relative. /// /// Use `NoFollow`: the security harness requires every relative directory -/// `openat` (not the top-level command path) to carry `O_NOFOLLOW`. `..` is -/// never a symlink in a normal directory, so this still restores the parent. +/// `openat` (not the top-level command path) to carry `O_NOFOLLOW`. fn reopen_parent_from_child(child: &DirFd) -> std::io::Result { child.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow) } -fn open_dir_fd_count(stack: &[DirWalkFrame]) -> usize { - stack.iter().filter(|frame| frame.dir_fd.is_some()).count() +#[cfg(not(target_os = "redox"))] +pub fn safe_remove_dir_recursive_impl( + path: &Path, + dir_fd: &DirFd, + options: &Options, + root_dev: u64, + parent_dev: u64, +) -> bool { + safe_remove_dir_recursive_impl_depth(path, dir_fd, options, root_dev, parent_dev, 0) } +/// Recursive walk matching pre-#7995 structure while `depth < DIR_FD_BUDGET`. +/// +/// Once the next level would exceed the budget, the remaining subtree is removed +/// with [`safe_remove_dir_deep_o1`] so open directory FDs stay bounded. #[cfg(not(target_os = "redox"))] -pub fn safe_remove_dir_recursive_impl( +fn safe_remove_dir_recursive_impl_depth( path: &Path, dir_fd: &DirFd, options: &Options, root_dev: u64, parent_dev: u64, + depth: usize, ) -> bool { - // Snapshot entries from the caller's DirFd, then re-open so the walk owns - // the root descriptor (and later frames under the FD budget). - let root_entries = match dir_fd.read_dir() { + let entries = match dir_fd.read_dir() { Ok(entries) => entries, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { if !options.force { @@ -437,27 +440,144 @@ pub fn safe_remove_dir_recursive_impl( } }; - let root_fd = match DirFd::open(path, SymlinkBehavior::Follow) { - Ok(fd) => fd, - Err(e) => return handle_error_with_force(e, path, options), - }; + let mut error = false; + + for entry_name in entries { + let entry_path = path.join(&entry_name); + + let entry_stat = match dir_fd.stat_at(&entry_name, SymlinkBehavior::NoFollow) { + Ok(stat) => stat, + Err(e) => { + error |= handle_error_with_force(e, &entry_path, options); + continue; + } + }; + + let is_dir = ((entry_stat.st_mode as libc::mode_t) & libc::S_IFMT) == libc::S_IFDIR; + + if is_dir { + #[allow(clippy::unnecessary_cast)] + let entry_dev = entry_stat.st_dev as u64; + + if options.one_fs && entry_dev != root_dev { + show_error!( + "{}", + translate!("rm-error-skipping-different-device", "file" => entry_path.quote()) + ); + error = true; + continue; + } + + if options.preserve_root_all && entry_dev != parent_dev { + show_preserve_root_all_skip(&entry_path); + error = true; + continue; + } + + if options.interactive == InteractiveMode::Always + && !is_dir_empty(&entry_path) + && !prompt_descend(&entry_path) + { + continue; + } + + let child_dir_fd = match dir_fd.open_subdir(&entry_name, SymlinkBehavior::NoFollow) { + Ok(fd) => fd, + Err(e) => { + if e.kind() == std::io::ErrorKind::PermissionDenied { + error |= handle_permission_denied( + dir_fd, + entry_name.as_ref(), + &entry_path, + options, + ); + } else { + error |= handle_error_with_force(e, &entry_path, options); + } + continue; + } + }; - // parent_dev is unused on the root frame itself; the caller passes - // parent_dev == root_dev for the initial invocation. - let _ = parent_dev; + // Shallow: same recursive shape as main (parent DirFd stays open). + // Deep: iterative O(1) FD walk for the remaining subtree only. + let child_error = if depth + 1 < DIR_FD_BUDGET { + safe_remove_dir_recursive_impl_depth( + &entry_path, + &child_dir_fd, + options, + root_dev, + entry_dev, + depth + 1, + ) + } else { + safe_remove_dir_deep_o1( + &entry_path, + child_dir_fd, + options, + root_dev, + entry_dev, + entry_stat.st_mode as libc::mode_t, + entry_name.clone(), + ) + }; + error |= child_error; + + if !child_error + && options.interactive == InteractiveMode::Always + && !prompt_dir_with_mode(&entry_path, entry_stat.st_mode as libc::mode_t, options) + { + continue; + } + + if !child_error { + error |= handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, true, options); + } + } else if prompt_file_with_stat(&entry_path, &entry_stat, options) { + error |= handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, false, options); + } + } + + error +} + +/// Iterative remove for subtrees that already sit at [`DIR_FD_BUDGET`]. +/// +/// Always closes the parent before descending and restores it with +/// `openat(child, "..")` so open directory FDs stay O(1) for pathological depth. +#[cfg(not(target_os = "redox"))] +fn safe_remove_dir_deep_o1( + path: &Path, + root_fd: DirFd, + options: &Options, + root_dev: u64, + dir_dev: u64, + mode: libc::mode_t, + name_in_parent: std::ffi::OsString, +) -> bool { + let root_entries = match root_fd.read_dir() { + Ok(entries) => entries, + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + if !options.force { + show_permission_denied_error(path); + } + return !options.force; + } + Err(e) => { + return handle_error_with_force(e, path, options); + } + }; - // Reverse so pop() yields entries in the original read order. let mut root_pending = root_entries; root_pending.reverse(); let mut stack = vec![DirWalkFrame { path: path.to_path_buf(), dir_fd: Some(root_fd), - dir_dev: root_dev, + dir_dev, pending: root_pending, error: false, - mode: 0, - name_in_parent: std::ffi::OsString::new(), + mode, + name_in_parent, }]; let mut had_error = false; @@ -466,57 +586,49 @@ pub fn safe_remove_dir_recursive_impl( let frame_idx = stack.len() - 1; if stack[frame_idx].pending.is_empty() { - let child_error = stack[frame_idx].error; - let child_path = stack[frame_idx].path.clone(); - let child_mode = stack[frame_idx].mode; - let child_name = stack[frame_idx].name_in_parent.clone(); - - if stack.len() == 1 { - // Root frame: caller removes the top directory. - stack[frame_idx].dir_fd = None; + let frame = stack.pop().unwrap(); + let child_error = frame.error; + let child_path = frame.path; + let child_mode = frame.mode; + let child_name = frame.name_in_parent; + let mut child_fd = frame.dir_fd; + + if stack.is_empty() { + // Subtree root: caller unlinks this directory from its parent. had_error = child_error; - stack.pop(); break; } - let parent_idx = frame_idx - 1; - // Restore the parent FD from the child via ".." before dropping the - // child FD. Do not DirFd::open(parent_path): deep trees exceed PATH_MAX. + let parent_idx = stack.len() - 1; + // Always closed parent on descend in deep walk — restore via "..". if stack[parent_idx].dir_fd.is_none() { - let child_fd = stack[frame_idx].dir_fd.take(); match child_fd.as_ref() { Some(fd) => match reopen_parent_from_child(fd) { Ok(parent_fd) => stack[parent_idx].dir_fd = Some(parent_fd), Err(e) => { stack[parent_idx].error |= handle_error_with_force(e, &stack[parent_idx].path, options); - stack.pop(); continue; } }, - None => { - // Defensive fallback for short paths only; long paths fail. - match DirFd::open(&stack[parent_idx].path, SymlinkBehavior::NoFollow) { - Ok(fd) => stack[parent_idx].dir_fd = Some(fd), - Err(e) => { - stack[parent_idx].error |= - handle_error_with_force(e, &stack[parent_idx].path, options); - stack.pop(); - continue; - } + None => match DirFd::open(&stack[parent_idx].path, SymlinkBehavior::NoFollow) { + Ok(fd) => stack[parent_idx].dir_fd = Some(fd), + Err(e) => { + stack[parent_idx].error |= + handle_error_with_force(e, &stack[parent_idx].path, options); + continue; } - } + }, } - } else { - stack[frame_idx].dir_fd = None; } + child_fd = None; + drop(child_fd); if child_error { stack[parent_idx].error = true; } else if options.interactive == InteractiveMode::Always && !prompt_dir_with_mode(&child_path, child_mode, options) { - stack.pop(); continue; } else if let Some(parent_fd) = stack[parent_idx].dir_fd.as_ref() { stack[parent_idx].error |= @@ -524,8 +636,6 @@ pub fn safe_remove_dir_recursive_impl( } else { stack[parent_idx].error = true; } - - stack.pop(); continue; } @@ -534,8 +644,6 @@ pub fn safe_remove_dir_recursive_impl( }; let entry_path = stack[frame_idx].path.join(&entry_name); - // Parent FD is restored via openat("..") when a child finishes. Path - // reopen is only a last resort for short paths (PATH_MAX trees break). if stack[frame_idx].dir_fd.is_none() { match DirFd::open(&stack[frame_idx].path, SymlinkBehavior::NoFollow) { Ok(fd) => stack[frame_idx].dir_fd = Some(fd), @@ -566,7 +674,6 @@ pub fn safe_remove_dir_recursive_impl( let is_dir = ((entry_stat.st_mode as libc::mode_t) & libc::S_IFMT) == libc::S_IFDIR; if is_dir { - // st_dev's type varies by platform (i32 on macOS, u64 on Linux). #[allow(clippy::unnecessary_cast)] let entry_dev = entry_stat.st_dev as u64; @@ -579,16 +686,12 @@ pub fn safe_remove_dir_recursive_impl( continue; } - // --preserve-root=all compares against the immediate parent rather - // than the tree root, so a mount nested anywhere in the tree is - // caught even when --one-file-system is not in effect. if options.preserve_root_all && entry_dev != parent_dev_for_child { show_preserve_root_all_skip(&entry_path); stack[frame_idx].error = true; continue; } - // Ask user if they want to descend into this directory if options.interactive == InteractiveMode::Always && !is_dir_empty(&entry_path) && !prompt_descend(&entry_path) @@ -596,12 +699,6 @@ pub fn safe_remove_dir_recursive_impl( continue; } - // Open the subdirectory while the parent is still open. - // - // rm never follows symlinks during recursion, so open with - // NoFollow: if an attacker swaps this just-stat'd directory for a - // symlink before the open, O_NOFOLLOW makes openat fail instead of - // descending off-tree and deleting unrelated files. let child_dir_fd = { let Some(dir_fd) = stack[frame_idx].dir_fd.as_ref() else { stack[frame_idx].error = true; @@ -643,14 +740,8 @@ pub fn safe_remove_dir_recursive_impl( } }; - // Hybrid FD budget: keep ancestors open for shallow trees (benchmark - // depth ~5). Only close the parent when pushing the child would - // exceed DIR_FD_BUDGET; restore later via openat(".."). - // open_dir_fd_count includes this parent; +1 is the child about to - // be pushed. - if open_dir_fd_count(&stack) + 1 > DIR_FD_BUDGET { - stack[frame_idx].dir_fd = None; - } + // Always close parent before deep descend (already past budget). + stack[frame_idx].dir_fd = None; let mut child_pending = child_entries; child_pending.reverse(); From 2589291b5d6e64095fbfbed67eb18ef59da6d45e Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Mon, 20 Jul 2026 09:12:47 +0000 Subject: [PATCH 10/15] fix(rm): verify parent inode after openat restore Record each deep-walk directory's device and inode when first opened. After reopening a closed parent with openat(child, "..") (or a path fallback), fstat and require the same identity before unlinking, so a renamed/moved directory is not treated as the original parent. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 117 +++++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 20 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 1fc11563adb..d2743a04a95 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -386,6 +386,9 @@ struct DirWalkFrame { path: std::path::PathBuf, dir_fd: Option, dir_dev: u64, + /// Identity of this directory when first opened (device + inode). + /// Checked after `openat(child, "..")` so a moved parent is not unlinked into. + dir_ino: u64, pending: Vec, error: bool, mode: libc::mode_t, @@ -403,6 +406,35 @@ fn reopen_parent_from_child(child: &DirFd) -> std::io::Result { child.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow) } +/// Restore the parent directory FD and confirm it is still the same directory. +/// +/// Closing the parent before a deep descend leaves a window where that directory +/// can be renamed/moved. After `openat(child, "..")` (or a path reopen), `fstat` +/// the result and require the same device + inode we recorded on the way down. +fn reopen_parent_checked( + child: Option<&DirFd>, + parent_path: &Path, + expected_dev: u64, + expected_ino: u64, +) -> std::io::Result { + let parent_fd = match child { + Some(fd) => reopen_parent_from_child(fd)?, + None => DirFd::open(parent_path, SymlinkBehavior::NoFollow)?, + }; + let st = parent_fd.fstat()?; + #[allow(clippy::unnecessary_cast)] + let got_dev = st.st_dev as u64; + #[allow(clippy::unnecessary_cast)] + let got_ino = st.st_ino as u64; + if got_dev != expected_dev || got_ino != expected_ino { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + "directory changed while removing", + )); + } + Ok(parent_fd) +} + #[cfg(not(target_os = "redox"))] pub fn safe_remove_dir_recursive_impl( path: &Path, @@ -543,7 +575,8 @@ fn safe_remove_dir_recursive_impl_depth( /// Iterative remove for subtrees that already sit at [`DIR_FD_BUDGET`]. /// /// Always closes the parent before descending and restores it with -/// `openat(child, "..")` so open directory FDs stay O(1) for pathological depth. +/// `openat(child, "..")` plus a device/inode check so open directory FDs stay +/// O(1) for pathological depth without following a moved parent. #[cfg(not(target_os = "redox"))] fn safe_remove_dir_deep_o1( path: &Path, @@ -570,10 +603,23 @@ fn safe_remove_dir_deep_o1( let mut root_pending = root_entries; root_pending.reverse(); + let root_ino = match root_fd.fstat() { + Ok(st) => { + #[allow(clippy::unnecessary_cast)] + { + st.st_ino as u64 + } + } + Err(e) => { + return handle_error_with_force(e, path, options); + } + }; + let mut stack = vec![DirWalkFrame { path: path.to_path_buf(), dir_fd: Some(root_fd), dir_dev, + dir_ino: root_ino, pending: root_pending, error: false, mode, @@ -600,25 +646,24 @@ fn safe_remove_dir_deep_o1( } let parent_idx = stack.len() - 1; - // Always closed parent on descend in deep walk — restore via "..". + // Always closed parent on descend in deep walk — restore via ".." and + // check the recorded parent identity (device + inode) after fstat. if stack[parent_idx].dir_fd.is_none() { - match child_fd.as_ref() { - Some(fd) => match reopen_parent_from_child(fd) { - Ok(parent_fd) => stack[parent_idx].dir_fd = Some(parent_fd), - Err(e) => { - stack[parent_idx].error |= - handle_error_with_force(e, &stack[parent_idx].path, options); - continue; - } - }, - None => match DirFd::open(&stack[parent_idx].path, SymlinkBehavior::NoFollow) { - Ok(fd) => stack[parent_idx].dir_fd = Some(fd), - Err(e) => { - stack[parent_idx].error |= - handle_error_with_force(e, &stack[parent_idx].path, options); - continue; - } - }, + let expected_dev = stack[parent_idx].dir_dev; + let expected_ino = stack[parent_idx].dir_ino; + let parent_path = stack[parent_idx].path.clone(); + match reopen_parent_checked( + child_fd.as_ref(), + &parent_path, + expected_dev, + expected_ino, + ) { + Ok(parent_fd) => stack[parent_idx].dir_fd = Some(parent_fd), + Err(e) => { + stack[parent_idx].error |= + handle_error_with_force(e, &stack[parent_idx].path, options); + continue; + } } } child_fd = None; @@ -646,7 +691,35 @@ fn safe_remove_dir_deep_o1( if stack[frame_idx].dir_fd.is_none() { match DirFd::open(&stack[frame_idx].path, SymlinkBehavior::NoFollow) { - Ok(fd) => stack[frame_idx].dir_fd = Some(fd), + Ok(fd) => match fd.fstat() { + Ok(st) => { + #[allow(clippy::unnecessary_cast)] + let got_dev = st.st_dev as u64; + #[allow(clippy::unnecessary_cast)] + let got_ino = st.st_ino as u64; + if got_dev != stack[frame_idx].dir_dev + || got_ino != stack[frame_idx].dir_ino + { + stack[frame_idx].error |= handle_error_with_force( + std::io::Error::new( + std::io::ErrorKind::NotFound, + "directory changed while removing", + ), + &stack[frame_idx].path, + options, + ); + stack[frame_idx].pending.clear(); + continue; + } + stack[frame_idx].dir_fd = Some(fd); + } + Err(e) => { + stack[frame_idx].error |= + handle_error_with_force(e, &stack[frame_idx].path, options); + stack[frame_idx].pending.clear(); + continue; + } + }, Err(e) => { stack[frame_idx].error |= handle_error_with_force(e, &stack[frame_idx].path, options); @@ -746,10 +819,14 @@ fn safe_remove_dir_deep_o1( let mut child_pending = child_entries; child_pending.reverse(); + #[allow(clippy::unnecessary_cast)] + let entry_ino = entry_stat.st_ino as u64; + stack.push(DirWalkFrame { path: entry_path, dir_fd: Some(child_dir_fd), dir_dev: entry_dev, + dir_ino: entry_ino, pending: child_pending, error: false, mode: entry_stat.st_mode as libc::mode_t, From f7755d759868af602f66ccc7cfe703508fc594a5 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Tue, 21 Jul 2026 22:18:11 +0000 Subject: [PATCH 11/15] fix(rm): dual-mode FD headroom for deep recursive trees Hold parent DirFDs while free NOFILE slots remain; under pressure close the parent before DirFd::read_dir() dups the child, then restore via openat(child, "..") with a device/inode check. Account ambient (inherited) FDs with a one-time open-fd sample instead of a fixed baseline, surface EMFILE/ENFILE even with -f, and add deep NOFILE plus real inherited-FD regressions for #7995. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 451 +++++++++++++++------------------ tests/by-util/test_rm.rs | 124 ++++++++- 2 files changed, 319 insertions(+), 256 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index d2743a04a95..e1b503b0490 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -180,6 +180,15 @@ fn handle_error_with_force(e: std::io::Error, path: &Path, options: &Options) -> return true; } + // Resource exhaustion during traversal is a real failure even with -f. + // Swallowing it leaves the tree intact and the outer path probe can report + // a misleading "Directory not empty" instead of the actual EMFILE/ENFILE. + if is_fd_resource_error(&e) { + let e = e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote())); + show_error!("{e}"); + return true; + } + if !options.force { let e = e.map_err_context(|| translate!("rm-error-cannot-remove", "file" => path.quote())); show_error!("{e}"); @@ -187,6 +196,14 @@ fn handle_error_with_force(e: std::io::Error, path: &Path, options: &Options) -> !options.force } +/// EMFILE / ENFILE (and equivalent raw errno) must not be force-swallowed. +fn is_fd_resource_error(e: &std::io::Error) -> bool { + matches!( + e.raw_os_error(), + Some(code) if code == libc::EMFILE || code == libc::ENFILE + ) +} + /// Helper to handle permission denied errors fn handle_permission_denied( dir_fd: &DirFd, @@ -371,96 +388,142 @@ pub fn safe_remove_dir_recursive( } } -/// Soft cap on simultaneously open directory FDs during recursive rm. +/// One directory on the removal stack. /// -/// Shallow trees (CodSpeed `rm_recursive_tree`, depth 5) use the same recursive -/// walk as before and stay under this budget, so parent `DirFd`s remain open and -/// there is no `openat("..")` churn. Past the budget we switch the remaining -/// subtree to an O(1)-FD iterative walk (#7995) that closes before descend and -/// restores parents with `openat(child, "..")` + `O_NOFOLLOW` (GNU `rm/deep-2`). -/// Keep well below the low-NOFILE regression soft limit (32) after stdio. -const DIR_FD_BUDGET: usize = 16; - -/// Frame for the deep (past-budget) iterative walk. +/// Hold parent FDs while the process still has free NOFILE slots (main-like +/// shallow path). Under FD pressure, close the parent before deeper work and +/// restore it with `openat(child, "..")` so deep trees cannot burn RLIMIT (#7995). struct DirWalkFrame { + /// Display / prompt / error path only — never used to reopen intermediate dirs. path: std::path::PathBuf, + /// Live FD while this frame is the active reader / held ancestor. + /// `None` after close-under-pressure until restored from a child. dir_fd: Option, + /// Identity from `fstat` after open; restore must match these. dir_dev: u64, - /// Identity of this directory when first opened (device + inode). - /// Checked after `openat(child, "..")` so a moved parent is not unlinked into. dir_ino: u64, + /// Remaining basenames (snapshot from `read_dir`); pop processes in reverse. pending: Vec, error: bool, mode: libc::mode_t, name_in_parent: std::ffi::OsString, } -/// Re-open the parent of `child` without using the absolute path. +/// Slots needed for `read_dir` after a child directory is already open +/// (`DirFd::read_dir` dups the child FD). +const RESERVE_READDIR_SLOTS: usize = 1; + +/// Soft `RLIMIT_NOFILE` for the current process, if available. +fn soft_nofile_limit() -> Option { + let mut rlim = libc::rlimit { + rlim_cur: 0, + rlim_max: 0, + }; + // SAFETY: getrlimit with a valid rlimit pointer is well-defined. + let rc = unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &raw mut rlim) }; + if rc != 0 { + return None; + } + if rlim.rlim_cur == libc::RLIM_INFINITY { + return Some(usize::MAX / 4); + } + usize::try_from(rlim.rlim_cur).ok() +} + +/// Best-effort count of open FDs (Linux `/proc/self/fd`, else `/dev/fd`). +fn current_open_fd_count() -> Option { + fn count_dir(path: &str) -> Option { + let entries = fs::read_dir(path).ok()?; + let mut count = 0usize; + for entry in entries.flatten() { + let name = entry.file_name(); + if name.to_string_lossy().parse::().is_ok() { + count = count.saturating_add(1); + } + } + Some(count) + } + count_dir("/proc/self/fd").or_else(|| count_dir("/dev/fd")) +} + +/// Whether to close the parent before the next FD-consuming step. /// -/// Deep trees (GNU `rm/deep-2`) exceed `PATH_MAX`, so path reopen fails with -/// "file name too long". `openat(child, "..")` stays relative. +/// Call this **after** `open_subdir` has succeeded and **before** `read_dir` +/// (which dups the child). `walk_open` must already count the newly opened child. /// -/// Use `NoFollow`: the security harness requires every relative directory -/// `openat` (not the top-level command path) to carry `O_NOFOLLOW`. -fn reopen_parent_from_child(child: &DirFd) -> std::io::Result { - child.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow) +/// `ambient_non_walk` is the number of open FDs that are **not** walk-owned +/// directory FDs, sampled once after the root dir FD is open +/// (`current_open_fd_count() - 1`). Estimated open count is then +/// `ambient_non_walk + walk_open` without re-reading `/proc` on every step. +/// Inherited descriptors are therefore part of the budget (not a fixed baseline). +fn should_close_parent( + soft: Option, + walk_open: usize, + ambient_non_walk: Option, +) -> bool { + let Some(soft) = soft else { + // No rlimit: prefer hold (speed). + return false; + }; + if soft == 0 { + return true; + } + // Huge / "unlimited" soft limits: hold parents like main. + if soft > 1_000_000 { + return false; + } + + match ambient_non_walk { + Some(ambient) => { + let estimated_open = ambient.saturating_add(walk_open); + soft.saturating_sub(estimated_open) < RESERVE_READDIR_SLOTS + } + // No live FD table: close once walk dirs alone leave no readdir slot. + None => walk_open.saturating_add(RESERVE_READDIR_SLOTS) > soft, + } } -/// Restore the parent directory FD and confirm it is still the same directory. +/// Restore the parent directory FD via `openat(child, "..")` and confirm identity. /// -/// Closing the parent before a deep descend leaves a window where that directory -/// can be renamed/moved. After `openat(child, "..")` (or a path reopen), `fstat` -/// the result and require the same device + inode we recorded on the way down. -fn reopen_parent_checked( - child: Option<&DirFd>, - parent_path: &Path, +/// Closing the parent under pressure leaves a window where that directory can be +/// renamed/moved. After reopening through the child, `fstat` must match the +/// device + inode recorded when the parent was first opened. No path reopen. +fn restore_parent_dirfd( + child: &DirFd, expected_dev: u64, expected_ino: u64, ) -> std::io::Result { - let parent_fd = match child { - Some(fd) => reopen_parent_from_child(fd)?, - None => DirFd::open(parent_path, SymlinkBehavior::NoFollow)?, - }; + let parent_fd = child.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow)?; let st = parent_fd.fstat()?; #[allow(clippy::unnecessary_cast)] let got_dev = st.st_dev as u64; #[allow(clippy::unnecessary_cast)] let got_ino = st.st_ino as u64; if got_dev != expected_dev || got_ino != expected_ino { - return Err(std::io::Error::new( - std::io::ErrorKind::NotFound, - "directory changed while removing", - )); + return Err(std::io::Error::other("directory changed while removing")); } Ok(parent_fd) } -#[cfg(not(target_os = "redox"))] -pub fn safe_remove_dir_recursive_impl( - path: &Path, - dir_fd: &DirFd, - options: &Options, - root_dev: u64, - parent_dev: u64, -) -> bool { - safe_remove_dir_recursive_impl_depth(path, dir_fd, options, root_dev, parent_dev, 0) -} - -/// Recursive walk matching pre-#7995 structure while `depth < DIR_FD_BUDGET`. +/// Iterative recursive remove with dual FD modes. /// -/// Once the next level would exceed the budget, the remaining subtree is removed -/// with [`safe_remove_dir_deep_o1`] so open directory FDs stay bounded. +/// Hold ancestor directory FDs while free NOFILE slots remain (shallow / CodSpeed +/// path). Parent must stay open to `openat` the child; after the child is open, +/// close the parent under pressure **before** `read_dir` (which dups the child +/// FD). Restore closed parents with `openat(child, "..")` + device/inode check +/// so deep trees stay within RLIMIT (#7995). #[cfg(not(target_os = "redox"))] -fn safe_remove_dir_recursive_impl_depth( +pub fn safe_remove_dir_recursive_impl( path: &Path, - dir_fd: &DirFd, + _dir_fd: &DirFd, options: &Options, root_dev: u64, - parent_dev: u64, - depth: usize, + _parent_dev: u64, ) -> bool { - let entries = match dir_fd.read_dir() { - Ok(entries) => entries, + // Own a root FD for the stack. The caller's FD stays borrowed for API + // stability; one extra open of the user-supplied path (Follow, top-level only). + let root_fd = match DirFd::open(path, SymlinkBehavior::Follow) { + Ok(fd) => fd, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { if !options.force { show_permission_denied_error(path); @@ -472,121 +535,6 @@ fn safe_remove_dir_recursive_impl_depth( } }; - let mut error = false; - - for entry_name in entries { - let entry_path = path.join(&entry_name); - - let entry_stat = match dir_fd.stat_at(&entry_name, SymlinkBehavior::NoFollow) { - Ok(stat) => stat, - Err(e) => { - error |= handle_error_with_force(e, &entry_path, options); - continue; - } - }; - - let is_dir = ((entry_stat.st_mode as libc::mode_t) & libc::S_IFMT) == libc::S_IFDIR; - - if is_dir { - #[allow(clippy::unnecessary_cast)] - let entry_dev = entry_stat.st_dev as u64; - - if options.one_fs && entry_dev != root_dev { - show_error!( - "{}", - translate!("rm-error-skipping-different-device", "file" => entry_path.quote()) - ); - error = true; - continue; - } - - if options.preserve_root_all && entry_dev != parent_dev { - show_preserve_root_all_skip(&entry_path); - error = true; - continue; - } - - if options.interactive == InteractiveMode::Always - && !is_dir_empty(&entry_path) - && !prompt_descend(&entry_path) - { - continue; - } - - let child_dir_fd = match dir_fd.open_subdir(&entry_name, SymlinkBehavior::NoFollow) { - Ok(fd) => fd, - Err(e) => { - if e.kind() == std::io::ErrorKind::PermissionDenied { - error |= handle_permission_denied( - dir_fd, - entry_name.as_ref(), - &entry_path, - options, - ); - } else { - error |= handle_error_with_force(e, &entry_path, options); - } - continue; - } - }; - - // Shallow: same recursive shape as main (parent DirFd stays open). - // Deep: iterative O(1) FD walk for the remaining subtree only. - let child_error = if depth + 1 < DIR_FD_BUDGET { - safe_remove_dir_recursive_impl_depth( - &entry_path, - &child_dir_fd, - options, - root_dev, - entry_dev, - depth + 1, - ) - } else { - safe_remove_dir_deep_o1( - &entry_path, - child_dir_fd, - options, - root_dev, - entry_dev, - entry_stat.st_mode as libc::mode_t, - entry_name.clone(), - ) - }; - error |= child_error; - - if !child_error - && options.interactive == InteractiveMode::Always - && !prompt_dir_with_mode(&entry_path, entry_stat.st_mode as libc::mode_t, options) - { - continue; - } - - if !child_error { - error |= handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, true, options); - } - } else if prompt_file_with_stat(&entry_path, &entry_stat, options) { - error |= handle_unlink(dir_fd, entry_name.as_ref(), &entry_path, false, options); - } - } - - error -} - -/// Iterative remove for subtrees that already sit at [`DIR_FD_BUDGET`]. -/// -/// Always closes the parent before descending and restores it with -/// `openat(child, "..")` plus a device/inode check so open directory FDs stay -/// O(1) for pathological depth without following a moved parent. -#[cfg(not(target_os = "redox"))] -fn safe_remove_dir_deep_o1( - path: &Path, - root_fd: DirFd, - options: &Options, - root_dev: u64, - dir_dev: u64, - mode: libc::mode_t, - name_in_parent: std::ffi::OsString, -) -> bool { let root_entries = match root_fd.read_dir() { Ok(entries) => entries, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { @@ -600,34 +548,39 @@ fn safe_remove_dir_deep_o1( } }; - let mut root_pending = root_entries; - root_pending.reverse(); - - let root_ino = match root_fd.fstat() { - Ok(st) => { - #[allow(clippy::unnecessary_cast)] - { - st.st_ino as u64 - } - } + let root_stat = match root_fd.fstat() { + Ok(st) => st, Err(e) => { return handle_error_with_force(e, path, options); } }; + #[allow(clippy::unnecessary_cast)] + let root_frame_dev = root_stat.st_dev as u64; + #[allow(clippy::unnecessary_cast)] + let root_frame_ino = root_stat.st_ino as u64; + + let mut root_pending = root_entries; + root_pending.reverse(); + + // Cache once: getrlimit is cheap; ambient FD sample once (inherited + stdio + + // caller-held FDs). Walk-owned dirs are tracked via walk_open. + let soft_nofile = soft_nofile_limit(); + // Live directory FDs owned by this walk (stack frames with dir_fd = Some). + let mut walk_open = 1usize; + // FDs open now that are not this walk's root (root is already counted in walk_open). + let ambient_non_walk = current_open_fd_count().map(|n| n.saturating_sub(walk_open)); let mut stack = vec![DirWalkFrame { path: path.to_path_buf(), dir_fd: Some(root_fd), - dir_dev, - dir_ino: root_ino, + dir_dev: root_frame_dev, + dir_ino: root_frame_ino, pending: root_pending, error: false, - mode, - name_in_parent, + mode: 0, + name_in_parent: std::ffi::OsString::new(), }]; - let mut had_error = false; - while !stack.is_empty() { let frame_idx = stack.len() - 1; @@ -637,36 +590,43 @@ fn safe_remove_dir_deep_o1( let child_path = frame.path; let child_mode = frame.mode; let child_name = frame.name_in_parent; - let mut child_fd = frame.dir_fd; + let child_had_fd = frame.dir_fd.is_some(); + let child_fd = frame.dir_fd; + if child_had_fd { + walk_open = walk_open.saturating_sub(1); + } if stack.is_empty() { - // Subtree root: caller unlinks this directory from its parent. - had_error = child_error; - break; + // Subtree contents done; outer safe_remove_dir_recursive unlinks the root. + return child_error; } let parent_idx = stack.len() - 1; - // Always closed parent on descend in deep walk — restore via ".." and - // check the recorded parent identity (device + inode) after fstat. + + // Restore parent only if it was closed under FD pressure. if stack[parent_idx].dir_fd.is_none() { let expected_dev = stack[parent_idx].dir_dev; let expected_ino = stack[parent_idx].dir_ino; - let parent_path = stack[parent_idx].path.clone(); - match reopen_parent_checked( - child_fd.as_ref(), - &parent_path, - expected_dev, - expected_ino, - ) { - Ok(parent_fd) => stack[parent_idx].dir_fd = Some(parent_fd), - Err(e) => { - stack[parent_idx].error |= - handle_error_with_force(e, &stack[parent_idx].path, options); - continue; + if let Some(fd) = child_fd.as_ref() { + match restore_parent_dirfd(fd, expected_dev, expected_ino) { + Ok(parent_fd) => { + stack[parent_idx].dir_fd = Some(parent_fd); + walk_open = walk_open.saturating_add(1); + } + Err(e) => { + stack[parent_idx].error |= + handle_error_with_force(e, &stack[parent_idx].path, options); + // Fail closed for remaining siblings: no path reopen. + stack[parent_idx].pending.clear(); + continue; + } } + } else { + stack[parent_idx].error = true; + stack[parent_idx].pending.clear(); + continue; } } - child_fd = None; drop(child_fd); if child_error { @@ -689,50 +649,18 @@ fn safe_remove_dir_deep_o1( }; let entry_path = stack[frame_idx].path.join(&entry_name); + // If parent FD is missing with work left, prior restore failed — fail closed. if stack[frame_idx].dir_fd.is_none() { - match DirFd::open(&stack[frame_idx].path, SymlinkBehavior::NoFollow) { - Ok(fd) => match fd.fstat() { - Ok(st) => { - #[allow(clippy::unnecessary_cast)] - let got_dev = st.st_dev as u64; - #[allow(clippy::unnecessary_cast)] - let got_ino = st.st_ino as u64; - if got_dev != stack[frame_idx].dir_dev - || got_ino != stack[frame_idx].dir_ino - { - stack[frame_idx].error |= handle_error_with_force( - std::io::Error::new( - std::io::ErrorKind::NotFound, - "directory changed while removing", - ), - &stack[frame_idx].path, - options, - ); - stack[frame_idx].pending.clear(); - continue; - } - stack[frame_idx].dir_fd = Some(fd); - } - Err(e) => { - stack[frame_idx].error |= - handle_error_with_force(e, &stack[frame_idx].path, options); - stack[frame_idx].pending.clear(); - continue; - } - }, - Err(e) => { - stack[frame_idx].error |= - handle_error_with_force(e, &stack[frame_idx].path, options); - stack[frame_idx].pending.clear(); - continue; - } - } + stack[frame_idx].error = true; + stack[frame_idx].pending.clear(); + continue; } let parent_dev_for_child = stack[frame_idx].dir_dev; let entry_stat = { let Some(dir_fd) = stack[frame_idx].dir_fd.as_ref() else { stack[frame_idx].error = true; + stack[frame_idx].pending.clear(); continue; }; match dir_fd.stat_at(&entry_name, SymlinkBehavior::NoFollow) { @@ -775,6 +703,7 @@ fn safe_remove_dir_deep_o1( let child_dir_fd = { let Some(dir_fd) = stack[frame_idx].dir_fd.as_ref() else { stack[frame_idx].error = true; + stack[frame_idx].pending.clear(); continue; }; match dir_fd.open_subdir(&entry_name, SymlinkBehavior::NoFollow) { @@ -796,6 +725,31 @@ fn safe_remove_dir_deep_o1( } }; + // Identity of the FD we hold — never pre-open stat_at alone. + let child_stat = match child_dir_fd.fstat() { + Ok(st) => st, + Err(e) => { + drop(child_dir_fd); + stack[frame_idx].error |= handle_error_with_force(e, &entry_path, options); + continue; + } + }; + #[allow(clippy::unnecessary_cast)] + let child_dev = child_stat.st_dev as u64; + #[allow(clippy::unnecessary_cast)] + let child_ino = child_stat.st_ino as u64; + + // Child is open. Close parent under pressure BEFORE read_dir: that + // path dups the child FD (uucore safe_traversal), so checking after + // read_dir is too late for the reserve (#7995 / C1). + // walk_open is ancestors only; +1 for this child. + walk_open = walk_open.saturating_add(1); + if should_close_parent(soft_nofile, walk_open, ambient_non_walk) { + if stack[frame_idx].dir_fd.take().is_some() { + walk_open = walk_open.saturating_sub(1); + } + } + let child_entries = match child_dir_fd.read_dir() { Ok(entries) => entries, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { @@ -803,35 +757,32 @@ fn safe_remove_dir_deep_o1( show_permission_denied_error(&entry_path); } drop(child_dir_fd); + walk_open = walk_open.saturating_sub(1); stack[frame_idx].error |= !options.force; continue; } Err(e) => { drop(child_dir_fd); + walk_open = walk_open.saturating_sub(1); stack[frame_idx].error |= handle_error_with_force(e, &entry_path, options); continue; } }; - // Always close parent before deep descend (already past budget). - stack[frame_idx].dir_fd = None; - let mut child_pending = child_entries; child_pending.reverse(); - #[allow(clippy::unnecessary_cast)] - let entry_ino = entry_stat.st_ino as u64; - stack.push(DirWalkFrame { path: entry_path, dir_fd: Some(child_dir_fd), - dir_dev: entry_dev, - dir_ino: entry_ino, + dir_dev: child_dev, + dir_ino: child_ino, pending: child_pending, error: false, mode: entry_stat.st_mode as libc::mode_t, name_in_parent: entry_name, }); + // walk_open already includes this child from the pre-read_dir step. } else if prompt_file_with_stat(&entry_path, &entry_stat, options) { if let Some(dir_fd) = stack[frame_idx].dir_fd.as_ref() { stack[frame_idx].error |= @@ -842,7 +793,7 @@ fn safe_remove_dir_deep_o1( } } - had_error + false } #[cfg(target_os = "redox")] diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 302cb8ab019..0e797e5e43a 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1193,16 +1193,19 @@ fn test_rm_recursive_long_path_safe_traversal() { #[test] #[cfg(any(target_os = "linux", target_os = "android"))] fn test_rm_recursive_deep_tree_low_nofile() { - // Regression for #7995: recursive rm held one DirFd per nesting level and - // failed with EMFILE on deep trees under a tight NOFILE limit. GNU rm keeps - // FD use O(1) with depth; we should too. + // Regression for #7995: recursive rm used to keep one DirFd open per nesting + // level (plus a readdir dup), so deep chains under a tight NOFILE failed with + // EMFILE / residual "Directory not empty". GNU rm keeps directory FD use O(1) + // in depth; the walk must close the parent under pressure and restore via + // openat(child, ".."), not retain an ancestor chain. use rlimit::Resource; let ts = TestScenario::new(util_name!()); let at = &ts.fixtures; - // ~80 nested dirs is enough to exhaust a soft NOFILE of 32 if each level - // keeps its parent open, while remaining cheap to create. + // Depth 80 exhausts NOFILE=32 if each level keeps its parent open, while + // remaining cheap to create. Main fails this class; the dual-mode walk must + // remove the whole tree with no stderr. let depth = 80; let mut deep_path = String::from("rm_emfile_deep"); at.mkdir(&deep_path); @@ -1212,7 +1215,7 @@ fn test_rm_recursive_deep_tree_low_nofile() { } at.write(&format!("{deep_path}/leaf"), "data"); - // Leave headroom for stdio + a few helpers, but far below `depth`. + // Soft NOFILE well below `depth` (stdio + helpers still fit). ts.ucmd() .arg("-rf") .arg("rm_emfile_deep") @@ -1223,6 +1226,115 @@ fn test_rm_recursive_deep_tree_low_nofile() { assert!(!at.dir_exists("rm_emfile_deep")); } +/// Boundary for dual-mode headroom: parent must be closed before `read_dir` +/// dups the child FD when free slots are scarce (inherited descriptors). +/// +/// Uses a small helper binary that opens many FDs then execs `rm` so the +/// inherited table is tight without Python pipe overhead. +#[test] +#[cfg(any(target_os = "linux", target_os = "android"))] +fn test_rm_recursive_low_nofile_with_inherited_fds() { + use std::io::Write; + use std::process::Command; + + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + let depth = 40; + let mut deep_path = String::from("rm_inherit_deep"); + at.mkdir(&deep_path); + for _ in 0..depth { + deep_path = format!("{deep_path}/x"); + at.mkdir(&deep_path); + } + at.write(&format!("{deep_path}/leaf"), "data"); + + // Build a tiny C helper once per test run under the fixture dir. + // Open N /dev/null FDs WITHOUT O_CLOEXEC so they survive execv into rm + // (O_CLOEXEC would false-green this as a plain NOFILE smoke test). + let helper_src = at.plus("inherit_exec_rm.c"); + let helper_bin = at.plus("inherit_exec_rm"); + { + let mut f = std::fs::File::create(&helper_src).expect("helper source"); + f.write_all( + br#" +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include + +int main(int argc, char **argv) { + /* usage: inherit_exec_rm */ + if (argc < 5) { + fprintf(stderr, "usage: %s \n", argv[0]); + return 2; + } + int n = atoi(argv[1]); + struct rlimit rl = { .rlim_cur = 32, .rlim_max = 32 }; + if (setrlimit(RLIMIT_NOFILE, &rl) != 0) { + perror("setrlimit"); + return 3; + } + for (int i = 0; i < n; i++) { + /* Must survive execv - no O_CLOEXEC. */ + int fd = open("/dev/null", O_RDONLY); + if (fd < 0) { + perror("open /dev/null"); + return 4; + } + /* Belt: clear FD_CLOEXEC if the environment set it. */ + int flags = fcntl(fd, F_GETFD); + if (flags >= 0) { + fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC); + } + } + char *const args[] = { argv[2], argv[3], "-rf", argv[4], NULL }; + execv(argv[2], args); + perror("execv"); + return 5; +} +"#, + ) + .expect("write helper"); + } + let cc = Command::new("cc") + .args([ + "-O0", + "-o", + helper_bin.to_str().unwrap(), + helper_src.to_str().unwrap(), + ]) + .output() + .expect("spawn cc"); + assert!( + cc.status.success(), + "cc failed: {}", + String::from_utf8_lossy(&cc.stderr) + ); + + // Leave a few free slots under NOFILE=32 after helper opens extras + stdio. + // 18 extra FDs is enough to force pressure without preventing openat(child). + // Multicall binary: util name is the first argument after the path. + let rm_bin = ts.bin_path.clone(); + let out = Command::new(&helper_bin) + .args(["18", rm_bin.to_str().unwrap(), "rm", "rm_inherit_deep"]) + .current_dir(&at.subdir) + .output() + .expect("run inherit helper"); + assert!( + out.status.success(), + "helper/rm failed status={:?} stdout={} stderr={}", + out.status.code(), + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + + assert!(!at.dir_exists("rm_inherit_deep")); +} + #[cfg(all(not(windows), feature = "chmod"))] #[test] fn test_rm_directory_not_executable() { From 0ef82e018bc90579701a58b903472d7d77a72939 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Tue, 21 Jul 2026 23:13:01 +0000 Subject: [PATCH 12/15] style(rm): cspell ignores for dual-mode FD headroom House-style spell-checker:ignore for EMFILE/ENFILE/getrlimit/CLOEXEC and related FD-accounting tokens that Style/spelling flagged on the dual-mode walk and inherited-FD regression. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 1 + tests/by-util/test_rm.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index e1b503b0490..5b9acbca084 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -6,6 +6,7 @@ // Unix-specific implementations for the rm utility // spell-checker:ignore fstatat unlinkat statx behaviour NOFILE PATH_MAX +// spell-checker:ignore EMFILE ENFILE basenames dups rlim getrlimit RLIM dirfd use indicatif::ProgressBar; use std::ffi::OsStr; diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 0e797e5e43a..f4f064979e1 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -3,6 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. // spell-checker:ignore rootlink dotdot rootfile deleteme keepme topfile NOFILE EMFILE +// spell-checker:ignore ENFILE dups CLOEXEC execv fprintf atoi rlim setrlimit perror RDONLY GETFD SETFD #![allow(clippy::stable_sort_primitive)] use std::process::Stdio; From 91a2a309091a38ab36ba0203f11a03e96cef123b Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Wed, 22 Jul 2026 19:32:45 +0000 Subject: [PATCH 13/15] fix(rm): reuse root DirFd and drop runtime C inherit helper Consume the already-open root DirFd in the dual-mode walk, replace the embedded C inherit launcher with a Rust pre_exec boundary, add a restore identity mismatch unit test, and strip process-history comments. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 71 +++++++++++--------- tests/by-util/test_rm.rs | 114 +++++++++++---------------------- 2 files changed, 78 insertions(+), 107 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 5b9acbca084..5917a99c2a8 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -353,7 +353,8 @@ pub fn safe_remove_dir_recursive( }; // Entries of the root directory have the root itself as their parent. - let error = safe_remove_dir_recursive_impl(path, &dir_fd, options, root_dev, root_dev); + // Move the already-open FD into the walk so we do not reopen the path. + let error = safe_remove_dir_recursive_impl(path, dir_fd, options, root_dev, root_dev); // After processing all children, remove the directory itself if error { @@ -391,9 +392,8 @@ pub fn safe_remove_dir_recursive( /// One directory on the removal stack. /// -/// Hold parent FDs while the process still has free NOFILE slots (main-like -/// shallow path). Under FD pressure, close the parent before deeper work and -/// restore it with `openat(child, "..")` so deep trees cannot burn RLIMIT (#7995). +/// Hold parent FDs while free NOFILE slots remain. Under FD pressure, close the +/// parent before deeper work and restore it with `openat(child, "..")`. struct DirWalkFrame { /// Display / prompt / error path only — never used to reopen intermediate dirs. path: std::path::PathBuf, @@ -508,34 +508,18 @@ fn restore_parent_dirfd( /// Iterative recursive remove with dual FD modes. /// -/// Hold ancestor directory FDs while free NOFILE slots remain (shallow / CodSpeed -/// path). Parent must stay open to `openat` the child; after the child is open, -/// close the parent under pressure **before** `read_dir` (which dups the child -/// FD). Restore closed parents with `openat(child, "..")` + device/inode check -/// so deep trees stay within RLIMIT (#7995). +/// Hold ancestor directory FDs while free NOFILE slots remain. The parent must +/// stay open to `openat` the child; after the child is open, close the parent +/// under pressure before `read_dir` (which dups the child FD). Restore closed +/// parents with `openat(child, "..")` plus a device/inode check. #[cfg(not(target_os = "redox"))] pub fn safe_remove_dir_recursive_impl( path: &Path, - _dir_fd: &DirFd, + root_fd: DirFd, options: &Options, root_dev: u64, _parent_dev: u64, ) -> bool { - // Own a root FD for the stack. The caller's FD stays borrowed for API - // stability; one extra open of the user-supplied path (Follow, top-level only). - let root_fd = match DirFd::open(path, SymlinkBehavior::Follow) { - Ok(fd) => fd, - Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { - if !options.force { - show_permission_denied_error(path); - } - return !options.force; - } - Err(e) => { - return handle_error_with_force(e, path, options); - } - }; - let root_entries = match root_fd.read_dir() { Ok(entries) => entries, Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { @@ -740,9 +724,8 @@ pub fn safe_remove_dir_recursive_impl( #[allow(clippy::unnecessary_cast)] let child_ino = child_stat.st_ino as u64; - // Child is open. Close parent under pressure BEFORE read_dir: that - // path dups the child FD (uucore safe_traversal), so checking after - // read_dir is too late for the reserve (#7995 / C1). + // Child is open. Close parent under pressure before read_dir, which dups the + // child FD and would otherwise spend the reserved slot. // walk_open is ancestors only; +1 for this child. walk_open = walk_open.saturating_add(1); if should_close_parent(soft_nofile, walk_open, ambient_non_walk) { @@ -800,7 +783,7 @@ pub fn safe_remove_dir_recursive_impl( #[cfg(target_os = "redox")] pub fn safe_remove_dir_recursive_impl( _path: &Path, - _dir_fd: &DirFd, + _root_fd: DirFd, _options: &Options, _root_dev: u64, _parent_dev: u64, @@ -809,3 +792,33 @@ pub fn safe_remove_dir_recursive_impl( // This shouldn't be called on Redox, but provide a stub for compilation true // Return error } + +#[cfg(all(test, any(target_os = "linux", target_os = "android")))] +mod restore_identity_tests { + use super::restore_parent_dirfd; + use std::ffi::OsStr; + use std::fs; + use tempfile::tempdir; + use uucore::safe_traversal::{DirFd, SymlinkBehavior}; + + #[test] + fn restore_parent_rejects_identity_mismatch() { + let dir = tempdir().unwrap(); + let parent = dir.path().join("parent"); + let child = parent.join("child"); + fs::create_dir_all(&child).unwrap(); + + let parent_fd = DirFd::open(&parent, SymlinkBehavior::NoFollow).unwrap(); + let child_fd = parent_fd + .open_subdir(OsStr::new("child"), SymlinkBehavior::NoFollow) + .unwrap(); + let st = parent_fd.fstat().unwrap(); + let wrong_ino = (st.st_ino as u64).wrapping_add(1); + + let err = match restore_parent_dirfd(&child_fd, st.st_dev as u64, wrong_ino) { + Ok(_) => panic!("expected identity mismatch"), + Err(e) => e, + }; + assert_eq!(err.to_string(), "directory changed while removing"); + } +} diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index f4f064979e1..b2c2dc84435 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.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 rootlink dotdot rootfile deleteme keepme topfile NOFILE EMFILE -// spell-checker:ignore ENFILE dups CLOEXEC execv fprintf atoi rlim setrlimit perror RDONLY GETFD SETFD +// spell-checker:ignore ENFILE dups CLOEXEC setrlimit RDONLY GETFD SETFD #![allow(clippy::stable_sort_primitive)] use std::process::Stdio; @@ -1230,12 +1230,13 @@ fn test_rm_recursive_deep_tree_low_nofile() { /// Boundary for dual-mode headroom: parent must be closed before `read_dir` /// dups the child FD when free slots are scarce (inherited descriptors). /// -/// Uses a small helper binary that opens many FDs then execs `rm` so the -/// inherited table is tight without Python pipe overhead. +/// Opens extra non-CLOEXEC FDs in the child via `pre_exec` so they survive into +/// `rm` without a separate C helper binary. #[test] #[cfg(any(target_os = "linux", target_os = "android"))] fn test_rm_recursive_low_nofile_with_inherited_fds() { - use std::io::Write; + use std::ffi::CString; + use std::os::unix::process::CommandExt; use std::process::Command; let ts = TestScenario::new(util_name!()); @@ -1250,84 +1251,41 @@ fn test_rm_recursive_low_nofile_with_inherited_fds() { } at.write(&format!("{deep_path}/leaf"), "data"); - // Build a tiny C helper once per test run under the fixture dir. - // Open N /dev/null FDs WITHOUT O_CLOEXEC so they survive execv into rm - // (O_CLOEXEC would false-green this as a plain NOFILE smoke test). - let helper_src = at.plus("inherit_exec_rm.c"); - let helper_bin = at.plus("inherit_exec_rm"); - { - let mut f = std::fs::File::create(&helper_src).expect("helper source"); - f.write_all( - br#" -#define _GNU_SOURCE -#include -#include -#include -#include -#include -#include - -int main(int argc, char **argv) { - /* usage: inherit_exec_rm */ - if (argc < 5) { - fprintf(stderr, "usage: %s \n", argv[0]); - return 2; - } - int n = atoi(argv[1]); - struct rlimit rl = { .rlim_cur = 32, .rlim_max = 32 }; - if (setrlimit(RLIMIT_NOFILE, &rl) != 0) { - perror("setrlimit"); - return 3; - } - for (int i = 0; i < n; i++) { - /* Must survive execv - no O_CLOEXEC. */ - int fd = open("/dev/null", O_RDONLY); - if (fd < 0) { - perror("open /dev/null"); - return 4; - } - /* Belt: clear FD_CLOEXEC if the environment set it. */ - int flags = fcntl(fd, F_GETFD); - if (flags >= 0) { - fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC); - } - } - char *const args[] = { argv[2], argv[3], "-rf", argv[4], NULL }; - execv(argv[2], args); - perror("execv"); - return 5; -} -"#, - ) - .expect("write helper"); - } - let cc = Command::new("cc") - .args([ - "-O0", - "-o", - helper_bin.to_str().unwrap(), - helper_src.to_str().unwrap(), - ]) - .output() - .expect("spawn cc"); - assert!( - cc.status.success(), - "cc failed: {}", - String::from_utf8_lossy(&cc.stderr) - ); - // Leave a few free slots under NOFILE=32 after helper opens extras + stdio. // 18 extra FDs is enough to force pressure without preventing openat(child). - // Multicall binary: util name is the first argument after the path. - let rm_bin = ts.bin_path.clone(); - let out = Command::new(&helper_bin) - .args(["18", rm_bin.to_str().unwrap(), "rm", "rm_inherit_deep"]) - .current_dir(&at.subdir) - .output() - .expect("run inherit helper"); + let extra_fds = 18usize; + let mut cmd = Command::new(&ts.bin_path); + cmd.args(["rm", "-rf", "rm_inherit_deep"]) + .current_dir(&at.subdir); + // SAFETY: pre_exec runs in the forked child before exec; only async-signal-safe + // ops are used (open/fcntl/setrlimit). + unsafe { + cmd.pre_exec(move || { + let rl = libc::rlimit { + rlim_cur: 32, + rlim_max: 32, + }; + if libc::setrlimit(libc::RLIMIT_NOFILE, &rl) != 0 { + return Err(std::io::Error::last_os_error()); + } + let path = CString::new("/dev/null").unwrap(); + for _ in 0..extra_fds { + let fd = libc::open(path.as_ptr(), libc::O_RDONLY); + if fd < 0 { + return Err(std::io::Error::last_os_error()); + } + let flags = libc::fcntl(fd, libc::F_GETFD); + if flags >= 0 { + libc::fcntl(fd, libc::F_SETFD, flags & !libc::FD_CLOEXEC); + } + } + Ok(()) + }); + } + let out = cmd.output().expect("run inherit pre_exec rm"); assert!( out.status.success(), - "helper/rm failed status={:?} stdout={} stderr={}", + "rm failed status={:?} stdout={} stderr={}", out.status.code(), String::from_utf8_lossy(&out.stdout), String::from_utf8_lossy(&out.stderr) From 6957d4938629235dbbde75c40a2b93ea6df750e3 Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Wed, 22 Jul 2026 19:46:43 +0000 Subject: [PATCH 14/15] fix(rm): finish dual-mode cleanup residuals Drop dead parent_dev API surface, tighten restore identity tests to device and inode mismatch, and remove remaining process-history comments. Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 23 ++++++++++++++--------- tests/by-util/test_rm.rs | 8 ++++---- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 5917a99c2a8..b6686f24a62 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -354,7 +354,7 @@ pub fn safe_remove_dir_recursive( // Entries of the root directory have the root itself as their parent. // Move the already-open FD into the walk so we do not reopen the path. - let error = safe_remove_dir_recursive_impl(path, dir_fd, options, root_dev, root_dev); + let error = safe_remove_dir_recursive_impl(path, dir_fd, options, root_dev); // After processing all children, remove the directory itself if error { @@ -469,7 +469,7 @@ fn should_close_parent( if soft == 0 { return true; } - // Huge / "unlimited" soft limits: hold parents like main. + // Huge soft limits: keep parents open. if soft > 1_000_000 { return false; } @@ -513,12 +513,11 @@ fn restore_parent_dirfd( /// under pressure before `read_dir` (which dups the child FD). Restore closed /// parents with `openat(child, "..")` plus a device/inode check. #[cfg(not(target_os = "redox"))] -pub fn safe_remove_dir_recursive_impl( +pub(crate) fn safe_remove_dir_recursive_impl( path: &Path, root_fd: DirFd, options: &Options, root_dev: u64, - _parent_dev: u64, ) -> bool { let root_entries = match root_fd.read_dir() { Ok(entries) => entries, @@ -781,12 +780,11 @@ pub fn safe_remove_dir_recursive_impl( } #[cfg(target_os = "redox")] -pub fn safe_remove_dir_recursive_impl( +pub(crate) fn safe_remove_dir_recursive_impl( _path: &Path, _root_fd: DirFd, _options: &Options, _root_dev: u64, - _parent_dev: u64, ) -> bool { // safe_traversal stat_at is not supported on Redox // This shouldn't be called on Redox, but provide a stub for compilation @@ -814,11 +812,18 @@ mod restore_identity_tests { .unwrap(); let st = parent_fd.fstat().unwrap(); let wrong_ino = (st.st_ino as u64).wrapping_add(1); + let wrong_dev = (st.st_dev as u64).wrapping_add(1); - let err = match restore_parent_dirfd(&child_fd, st.st_dev as u64, wrong_ino) { - Ok(_) => panic!("expected identity mismatch"), + let err_ino = match restore_parent_dirfd(&child_fd, st.st_dev as u64, wrong_ino) { + Ok(_) => panic!("expected inode mismatch"), Err(e) => e, }; - assert_eq!(err.to_string(), "directory changed while removing"); + assert_eq!(err_ino.to_string(), "directory changed while removing"); + + let err_dev = match restore_parent_dirfd(&child_fd, wrong_dev, st.st_ino as u64) { + Ok(_) => panic!("expected device mismatch"), + Err(e) => e, + }; + assert_eq!(err_dev.to_string(), "directory changed while removing"); } } diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index b2c2dc84435..3eee9d81909 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1205,8 +1205,8 @@ fn test_rm_recursive_deep_tree_low_nofile() { let at = &ts.fixtures; // Depth 80 exhausts NOFILE=32 if each level keeps its parent open, while - // remaining cheap to create. Main fails this class; the dual-mode walk must - // remove the whole tree with no stderr. + // remaining cheap to create. The dual-mode walk must remove the whole tree + // with no stderr. let depth = 80; let mut deep_path = String::from("rm_emfile_deep"); at.mkdir(&deep_path); @@ -1231,7 +1231,7 @@ fn test_rm_recursive_deep_tree_low_nofile() { /// dups the child FD when free slots are scarce (inherited descriptors). /// /// Opens extra non-CLOEXEC FDs in the child via `pre_exec` so they survive into -/// `rm` without a separate C helper binary. +/// `rm` under a tight NOFILE limit. #[test] #[cfg(any(target_os = "linux", target_os = "android"))] fn test_rm_recursive_low_nofile_with_inherited_fds() { @@ -1251,7 +1251,7 @@ fn test_rm_recursive_low_nofile_with_inherited_fds() { } at.write(&format!("{deep_path}/leaf"), "data"); - // Leave a few free slots under NOFILE=32 after helper opens extras + stdio. + // Leave a few free slots under NOFILE=32 after opening extras + stdio. // 18 extra FDs is enough to force pressure without preventing openat(child). let extra_fds = 18usize; let mut cmd = Command::new(&ts.bin_path); From 5a942c54649d6618c6b305f8e0f2543dec7403fe Mon Sep 17 00:00:00 2001 From: Alex Chen Date: Wed, 22 Jul 2026 20:30:16 +0000 Subject: [PATCH 15/15] fix(rm): harden dual-mode CI residuals - rewrite restore identity unit with let-else for clippy - ignore rlim for cspell - make inherited-FD pre_exec best-effort under busy parents and use ptr::from_ref for setrlimit Signed-off-by: Alex Chen --- src/uu/rm/src/platform/unix.rs | 10 ++++------ tests/by-util/test_rm.rs | 19 ++++++++++--------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index b6686f24a62..cc043fd6637 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -814,15 +814,13 @@ mod restore_identity_tests { let wrong_ino = (st.st_ino as u64).wrapping_add(1); let wrong_dev = (st.st_dev as u64).wrapping_add(1); - let err_ino = match restore_parent_dirfd(&child_fd, st.st_dev as u64, wrong_ino) { - Ok(_) => panic!("expected inode mismatch"), - Err(e) => e, + let Err(err_ino) = restore_parent_dirfd(&child_fd, st.st_dev as u64, wrong_ino) else { + panic!("expected inode mismatch"); }; assert_eq!(err_ino.to_string(), "directory changed while removing"); - let err_dev = match restore_parent_dirfd(&child_fd, wrong_dev, st.st_ino as u64) { - Ok(_) => panic!("expected device mismatch"), - Err(e) => e, + let Err(err_dev) = restore_parent_dirfd(&child_fd, wrong_dev, st.st_ino as u64) else { + panic!("expected device mismatch"); }; assert_eq!(err_dev.to_string(), "directory changed while removing"); } diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 3eee9d81909..e3090c49595 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.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 rootlink dotdot rootfile deleteme keepme topfile NOFILE EMFILE -// spell-checker:ignore ENFILE dups CLOEXEC setrlimit RDONLY GETFD SETFD +// spell-checker:ignore ENFILE dups CLOEXEC setrlimit RDONLY GETFD SETFD rlim rlimit #![allow(clippy::stable_sort_primitive)] use std::process::Stdio; @@ -1231,7 +1231,8 @@ fn test_rm_recursive_deep_tree_low_nofile() { /// dups the child FD when free slots are scarce (inherited descriptors). /// /// Opens extra non-CLOEXEC FDs in the child via `pre_exec` so they survive into -/// `rm` under a tight NOFILE limit. +/// `rm` under a tight NOFILE limit. Opens best-effort: cargo test parents may +/// already own many FDs, so a hard open quota would EMFILE before `rm` starts. #[test] #[cfg(any(target_os = "linux", target_os = "android"))] fn test_rm_recursive_low_nofile_with_inherited_fds() { @@ -1251,9 +1252,8 @@ fn test_rm_recursive_low_nofile_with_inherited_fds() { } at.write(&format!("{deep_path}/leaf"), "data"); - // Leave a few free slots under NOFILE=32 after opening extras + stdio. - // 18 extra FDs is enough to force pressure without preventing openat(child). - let extra_fds = 18usize; + // Soft NOFILE well below depth; open as many sticky FDs as the limit allows. + let extra_fds = 24usize; let mut cmd = Command::new(&ts.bin_path); cmd.args(["rm", "-rf", "rm_inherit_deep"]) .current_dir(&at.subdir); @@ -1262,17 +1262,18 @@ fn test_rm_recursive_low_nofile_with_inherited_fds() { unsafe { cmd.pre_exec(move || { let rl = libc::rlimit { - rlim_cur: 32, - rlim_max: 32, + rlim_cur: 48, + rlim_max: 48, }; - if libc::setrlimit(libc::RLIMIT_NOFILE, &rl) != 0 { + if libc::setrlimit(libc::RLIMIT_NOFILE, std::ptr::from_ref(&rl)) != 0 { return Err(std::io::Error::last_os_error()); } let path = CString::new("/dev/null").unwrap(); for _ in 0..extra_fds { let fd = libc::open(path.as_ptr(), libc::O_RDONLY); if fd < 0 { - return Err(std::io::Error::last_os_error()); + // Already under pressure from inherited descriptors; continue. + break; } let flags = libc::fcntl(fd, libc::F_GETFD); if flags >= 0 {