rm: fix segfault on very deeply nested directories

[sylvestre]

safe_remove_dir_recursive_impl recursed once per directory level, so rm -rf on a hierarchy tens of thousands of levels deep overflowed the stack and died with SIGSEGV, leaving the tree in place.

Walk the tree with an explicit stack instead. Only the deepest 16 levels keep their directory descriptor open; the others get theirs back through ".." on the way up, checked against the recorded device and inode so a directory swapped in mid-walk cannot redirect the removal. Depth is then bounded by memory rather than by the stack or the descriptor limit.

https://bugs.launchpad.net/bugs/2167206
#2949

[codspeed-hq]

Merging this PR will improve performance by 24.39%

⚡ 1 improved benchmark
✅ 7 untouched benchmarks
⏩ 410 skipped benchmarks¹

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
⚡	Simulation	rm_recursive_tree	26.4 ms	21.2 ms	+24.39%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing sylvestre:rm-deep-stack (61d46df) with main (b6726cc)}

Footnotes

1. 410 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[Copilot]

🟡 Changes recommended

Two moderate implementation and test-portability issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates Unix rm -rf to safely remove extremely deep directory trees without stack overflow.

Changes:

• Replaces recursive traversal with an explicit stack.
• Bounds open directory descriptors and validates reopened parents.
• Adds a 32K-level hierarchy regression test.

File summaries

File	Changes	Final findings
src/uu/rm/src/platform/unix.rs	Implements iterative safe traversal.	Moderate (2 votes): Avoid quadratic PathBuf cloning during unwind. Moderate (1 vote): Fix the descriptor-cap off-by-one.
tests/by-util/test_rm.rs	Adds deep-hierarchy coverage.	Moderate (3 votes): Exclude Redox, which uses an unsupported fallback. Nit (1 vote): Correct the descriptor-release comment.

Review details

Suppressed comments (2)

src/uu/rm/src/platform/unix.rs:580

• OPEN_DIR_FDS is 16, but the + 1 means the oldest frame is not closed until 17 frames are stacked. After that assignment, frames 1..16 still hold descriptors and cur_fd holds another, so 17 directory FDs remain open; use checked_sub(OPEN_DIR_FDS) (or adjust the constant and documentation) to enforce the stated cap.

            if let Some(closable) = stack.len().checked_sub(OPEN_DIR_FDS + 1) {
                stack[closable].dir_fd = None;

tests/by-util/test_rm.rs:1302

• This comment contradicts the next line: drop(fd) releases the deepest descriptor, so no directory is kept open while rm runs. Describe the release instead so the test setup rationale is accurate.

    // Keeping the deepest directory open makes its removal much slower.

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved traversal-safety, platform-compatibility, and regression-test issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

tests/by-util/test_rm.rs:1317

• depth > 1024 is not tied to the pre-fix failure depth. Because this test also accepts filesystems that stop at ENAMETOOLONG, it can pass at a depth where the old recursive implementation still fits on the stack, leaving the regression untested. Require a depth known to exhaust the old implementation, or run the traversal with a deliberately bounded stack while retaining a platform-specific skip when that depth cannot be created.

    assert!(
        depth > 1024,
        "hierarchy is too shallow to be a regression test"

tests/by-util/test_rm.rs:1284

• This test is enabled on AIX and Hurd, but uucore::safe_traversal is explicitly unavailable on those targets (src/uucore/src/lib/features.rs:88-93). Mirror that platform exclusion here; otherwise the test exercises the fallback implementation (or fails to compile) instead of the traversal fixed by this PR.

#[cfg(all(unix, not(target_os = "redox")))]

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Two moderate review issues remain regarding cleanup after ENAMETOOLONG and unsupported test targets.

Review details

Suppressed comments (2)

tests/by-util/test_rm.rs:1326

• If mkdirat succeeds but the following openat returns ENAMETOOLONG, the leaf remains created while depth is not incremented. This cleanup then runs the same traversal and can hit the same ENAMETOOLONG opening that leaf, so the intended skip path can fail and leave the partial tree behind. Unwind the partial tree with fd-relative operations (or otherwise make cleanup independent of rm) before returning.

    if depth < DEPTH {
        println!("this filesystem stops nesting at {depth} levels; skipping");
        // Still tear the tree down here: the harness cleanup recurses too.
        ts.ucmd().arg("-rf").arg("deep").succeeds();

tests/by-util/test_rm.rs:1284

• This test is enabled on AIX and Hurd even though uucore::safe_traversal is unavailable on those targets (src/uucore/src/lib/features.rs:88-93). Match the safe-traversal target exclusions here so the new nix setup is not built for unsupported platforms.

#[cfg(all(unix, not(target_os = "redox")))]

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Four unresolved moderate findings remain.

Review details

Suppressed comments (4)

src/uu/rm/src/platform/unix.rs:502

• This allocates and copies the entire accumulated path for every directory level. A 32K-deep rm -rf therefore does quadratic path-copying and allocator work even on the success path; keep the path as a push/pop buffer or materialize it only when a prompt or diagnostic needs it.

        let entry_path = path.join(&entry_name);

src/uu/rm/src/platform/unix.rs:405

• These new cfg guards still include AIX and Hurd, but uucore::safe_traversal is explicitly unavailable on those targets (src/uucore/src/lib/features.rs:88-93 and src/uucore/src/lib/lib.rs:106-111). Align the rm module/dependency and this traversal code with that shared exclusion (or provide a fallback), otherwise the Unix build cannot compile on those platforms.

#[cfg(not(target_os = "redox"))]

tests/by-util/test_rm.rs:1284

• This test's cfg is broader than the implementation it exercises: uucore::safe_traversal is not built on AIX or Hurd (see src/uucore/src/lib/features.rs:88-93), so the new fd-based test should exclude those targets as well; otherwise those Unix targets can fail to compile or run this test.

#[cfg(all(unix, not(target_os = "redox")))]

tests/by-util/test_rm.rs:1284

• This test is also compiled under wasi_runner, but the WASI binary uses the non-Unix fallback rather than this openat-based traversal, so the test either exercises a different implementation or fails on the 32K-level tree. Mark it ignored under WASI so this regression test only runs where it can validate the changed code.

#[cfg(all(unix, not(target_os = "redox")))]

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Fix the deep-tree interactive emptiness check and guard the regression test for wasi_runner.

Review details

Suppressed comments (2)

src/uu/rm/src/platform/unix.rs:576

• path_of(&path_buf) can exceed PATH_MAX in the deep-tree case, but is_dir_empty calls fs::read_dir on that pathname and treats any error as “non-empty” (src/uu/rm/src/rm.rs:621-627). As a result, rm -ri prompts to descend into empty directories at sufficient depth and can leave the tree when those prompts are declined; perform the emptiness check relative to the already-open directory fd before prompting.

                && !is_dir_empty(path_of(&path_buf))

tests/by-util/test_rm.rs:1284

• This Unix-gated test also runs in the repository's wasi_runner mode, where only the rm binary is WASI and cannot use the Unix fd-relative traversal needed for this 32K-deep tree. It will therefore fail instead of testing the intended implementation; add a #[cfg_attr(wasi_runner, ignore = ...)] guard (as other Unix syscall tests do).

#[cfg(all(unix, not(target_os = "redox")))]

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/tail/follow-name (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/retry (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout-group (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/rm/many-dir-entries-vs-OOM is now passing!

[Copilot]

🔵 Needs a closer look

Four unresolved moderate issues remain in traversal safety, descriptor limits, and test portability/cleanup.

Review details

Suppressed comments (4)

src/uu/rm/src/platform/unix.rs:601

• This remains a stat-then-open race: if an attacker replaces the entry with a different real directory (not a symlink), O_NOFOLLOW still succeeds and the walker will recurse into and delete that directory. Compare the opened fd's device/inode with entry_stat before pushing the frame, as the root is checked above at lines 349-353.

            let child_dir_fd = match cur_fd.open_subdir(&entry_name, SymlinkBehavior::NoFollow) {
                Ok(fd) => fd,

src/uu/rm/src/platform/unix.rs:645

• At stack.len() == OPEN_DIR_FDS, the current directory plus all 16 suspended frames are still open because checked_sub(OPEN_DIR_FDS) returns None; the next descent therefore reaches 17 persistent DirFds (before read_dir's temporary duplicate). Close the oldest suspended frame as soon as the stack reaches the configured bound, or adjust the bound to account for the current directory.

            if let Some(closable) = stack.len().checked_sub(OPEN_DIR_FDS) {
                stack[closable].dir_fd = None;

tests/by-util/test_rm.rs:1284

• Align this test guard with the supported traversal targets and skip it under the WASI runner. cfg(unix) && !redox includes AIX/Hurd, where safe_traversal is unavailable, and WASI cannot exercise this host-only openat/mkdirat 32K-directory stress test; otherwise this test is not portable across the repository's Unix test matrix.

#[cfg(all(unix, not(target_os = "redox")))]

tests/by-util/test_rm.rs:1316

• When openat returns ENAMETOOLONG, the preceding mkdirat has already created the final a, but this branch leaves it behind. The cleanup rm -rf deep hits the same failure before it can unlink that directory, so the intended skip path fails and can leave the fixture in place; remove the just-created child through the still-open fd before breaking.

            Err(Errno::ENAMETOOLONG) => break,

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Retain a fallback or descriptor for non-searchable directories and add a regression test.

Review details

Suppressed comments (1)

src/uu/rm/src/platform/unix.rs:458

• openat(child_fd, "..", ...) requires search/execute permission on the current child directory. Once this frame's parent descriptor is evicted, a readable but non-searchable directory at that level makes reopen_parent fail with EACCES, so rm -rf leaves an otherwise removable empty subtree; the previous recursive implementation retained the parent fd and did not have this regression. Keep a fallback/descriptor for this case and add a deep regression test.

    let parent_fd = child_fd.open_subdir(OsStr::new(".."), SymlinkBehavior::NoFollow)?;
    let info = parent_fd.metadata()?.file_info();

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite