From eb24f90a4d123645f528e520b3b61eabfca18345 Mon Sep 17 00:00:00 2001 From: Dongkeun Lee Date: Sat, 25 Jul 2026 00:35:20 -0400 Subject: [PATCH] fix(checkout): cap Treehouse return descriptors --- bin/fm-checkout-lock-lib.sh | 77 +++++++++++++---------- tests/fm-checkout-return-boundary.test.sh | 68 +++++++++++++++++--- tests/fm-teardown.test.sh | 73 +++++++++++++++++---- 3 files changed, 164 insertions(+), 54 deletions(-) diff --git a/bin/fm-checkout-lock-lib.sh b/bin/fm-checkout-lock-lib.sh index fb8369de73..28e3ad5cf4 100644 --- a/bin/fm-checkout-lock-lib.sh +++ b/bin/fm-checkout-lock-lib.sh @@ -372,42 +372,51 @@ parent_metadata = os.stat(parent) if opened.st_dev != parent_metadata.st_dev or os.path.ismount(target): raise SystemExit(74) root_device = opened.st_dev -descriptors = [descriptor] -pending = [(descriptor, target)] +# Descendant validation is point-in-time. Reopen each queued relative path +# from the retained root, validating every component descriptor-relatively, +# then release its descriptor immediately after listing it. Only the root +# remains bound across exec. Start with the root so every descendant is queued +# by this one descriptor-relative validation path. +pending = [()] while pending: - directory_fd, path = pending.pop() - for name in sorted(os.listdir(directory_fd)): - item = os.stat(name, dir_fd=directory_fd, follow_symlinks=False) - # A symlink INSIDE the tree is skipped, never refused and never - # descended. Refusing outright made this boundary reject any repository - # whose own committed layout uses symlinks - relvino puts 177 in every - # worktree (its CLAUDE.md -> AGENTS.md convention and symlinked skills), - # so no crew there could ever be reaped. What this walk exists to prove - # is that the destructive return cannot ESCAPE the tree, and a symlink - # entry cannot cause that here: it is inspected with - # follow_symlinks=False, it is not a directory so it is never queued, - # and every descent below - # opens with O_NOFOLLOW and re-proves dev/ino, single-device, and - # non-mount. The path-component loop above still refuses a symlinked - # ANCESTOR, which is the real redirection vector. - if stat.S_ISLNK(item.st_mode) or not stat.S_ISDIR(item.st_mode): - continue - child_path = os.path.join(path, name) - child = os.open(name, flags, dir_fd=directory_fd) - child_opened = os.fstat(child) - if ( - (item.st_dev, item.st_ino) != (child_opened.st_dev, child_opened.st_ino) - or child_opened.st_dev != root_device - or os.path.ismount(child_path) - ): - os.close(child) - raise SystemExit(74) - descriptors.append(child) - pending.append((child, child_path)) -for retained in descriptors: - os.set_inheritable(retained, True) + relative = pending.pop() + directory_fd = descriptor + path = target + try: + for name in relative: + item = os.stat(name, dir_fd=directory_fd, follow_symlinks=False) + if not stat.S_ISDIR(item.st_mode): + raise SystemExit(74) + parent_fd = directory_fd + child = os.open(name, flags, dir_fd=directory_fd) + directory_fd = child + os.set_inheritable(directory_fd, False) + child_opened = os.fstat(directory_fd) + path = os.path.join(path, name) + if ( + (item.st_dev, item.st_ino) + != (child_opened.st_dev, child_opened.st_ino) + or child_opened.st_dev != root_device + or os.path.ismount(path) + ): + raise SystemExit(74) + if parent_fd != descriptor: + os.close(parent_fd) + for name in sorted(os.listdir(directory_fd), reverse=True): + item = os.stat( + name, dir_fd=directory_fd, follow_symlinks=False + ) + # A symlink INSIDE the tree is skipped, never refused and never + # descended. Every directory is reopened with O_NOFOLLOW and + # re-proves dev/ino, single-device, and non-mount before use. + if stat.S_ISDIR(item.st_mode): + pending.append(relative + (name,)) + finally: + if directory_fd != descriptor: + os.close(directory_fd) +os.set_inheritable(descriptor, True) os.environ["FM_TREEHOUSE_RETURN_ROOT_FD"] = str(descriptor) -os.environ["FM_TREEHOUSE_RETURN_BOUNDARY_FDS"] = ",".join(map(str, descriptors)) +os.environ["FM_TREEHOUSE_RETURN_BOUNDARY_FDS"] = str(descriptor) os.environ["FM_TREEHOUSE_RETURN_PROJECT"] = project os.fchdir(descriptor) bound = os.stat(".") diff --git a/tests/fm-checkout-return-boundary.test.sh b/tests/fm-checkout-return-boundary.test.sh index a5a3bbb4a4..53825ea87f 100755 --- a/tests/fm-checkout-return-boundary.test.sh +++ b/tests/fm-checkout-return-boundary.test.sh @@ -2,17 +2,20 @@ # Tests for the destructive Treehouse-return boundary walk in # bin/fm-checkout-lock-lib.sh (fm_checkout_treehouse_return_locked). # -# The walk exists to prove that `treehouse return --force` cannot ESCAPE the -# worktree it is pointed at. It must refuse a redirected target - a symlinked -# path component, a non-directory, a mount point, a cross-device child - and it -# must NOT refuse a tree merely because ordinary symlinks live inside it. +# The walk validates that `treehouse return --force` starts from an acceptable +# worktree boundary. It must refuse a redirected target - a symlinked path +# component, a non-directory, a mount point, a cross-device child - and it must +# NOT refuse a tree merely because ordinary symlinks live inside it. The root +# descriptor remains open across exec so the return stays bound to that root. +# Descendant validation is point-in-time: descendant descriptors are released +# after their subtrees are checked instead of being inherited by Treehouse. # # That distinction is the regression this file pins. The walk originally # refused on ANY symlink entry anywhere in the tree, which made every repo # whose own committed layout uses symlinks permanently un-reapable: relvino # carries 177 of them in every worktree (its CLAUDE.md -> AGENTS.md convention # and its symlinked skills), so no crew there could ever be torn down. A -# symlink ENTRY cannot redirect this operation - it is inspected with +# symlink ENTRY cannot redirect this validation walk - it is inspected with # follow_symlinks=False, it is not a directory so it is never queued for # descent, and each descent opens with O_NOFOLLOW and re-proves identity, # single-device, and non-mount. Only a symlinked ANCESTOR redirects, and that @@ -67,11 +70,14 @@ make_case() { # -> prints " " printf '%s %s %s' "$case_dir" "$project" "$worktree" } -run_boundary() { # [cd-dir] -> exit status - local project=$1 target=$2 cd_dir=${3:-$1} fakebin status +run_boundary() { # [cd-dir] [fd-limit] -> exit status + local project=$1 target=$2 cd_dir=${3:-$1} fd_limit=${4:-} fakebin status fakebin=$(fm_fakebin "$(dirname "$project")/fake") fm_fake_exit0 "$fakebin" treehouse ( + if [ -n "$fd_limit" ]; then + ulimit -n "$fd_limit" || exit 71 + fi cd "$cd_dir" || exit 70 PATH="$fakebin:$PATH" python3 "$BOUNDARY_PY" "$target" "$project" ) >/dev/null 2>&1 @@ -158,9 +164,57 @@ EOF pass "a project/cwd mismatch is refused" } +test_realistically_large_and_deep_tree_respects_fd_limit() { + local rec case_dir project worktree fakebin status + rec=$(make_case realistically-large-tree) + read -r case_dir project worktree < "$fakebin/treehouse" <<'SH' +#!/bin/sh +root_descriptor=${FM_TREEHOUSE_RETURN_ROOT_FD:-} +[ -n "$root_descriptor" ] || exit 91 +[ "$FM_TREEHOUSE_RETURN_BOUNDARY_FDS" = "$root_descriptor" ] || exit 92 +[ -d "/dev/fd/$root_descriptor" ] || exit 93 +for candidate in /dev/fd/*; do + [ -d "$candidate" ] || continue + [ "${candidate##*/}" = "$root_descriptor" ] || exit 94 +done +exit 0 +SH + chmod +x "$fakebin/treehouse" + ( + ulimit -n 128 || exit 71 + cd "$project" || exit 70 + PATH="$fakebin:$PATH" python3 "$BOUNDARY_PY" "$worktree" "$project" + ) >/dev/null 2>&1 + status=$? + expect_code 0 "$status" "a worktree with more than 42,000 directories and depth beyond the descriptor limit must return" + pass "a real greater-than-42,000-directory fixture keeps only its root descriptor across exec" +} + extract_boundary_program test_in_tree_symlinks_are_accepted test_symlink_escaping_the_tree_is_still_not_followed test_symlinked_ancestor_is_refused test_non_directory_target_is_refused test_project_cwd_mismatch_is_refused +test_realistically_large_and_deep_tree_respects_fd_limit diff --git a/tests/fm-teardown.test.sh b/tests/fm-teardown.test.sh index 4a6fdb86a2..9be30fe8ae 100755 --- a/tests/fm-teardown.test.sh +++ b/tests/fm-teardown.test.sh @@ -3798,6 +3798,9 @@ test_teardown_removal_roots_fail_closed() { pass "missing and redirected removal roots never select another directory" } +# Only the root descriptor is inherited by Treehouse. That descriptor keeps +# relative traversal bound across replacement of the root pathname. Descendant +# validation remains point-in-time and this test does not claim otherwise. test_treehouse_return_stays_bound_to_validated_root() { local case_dir moved redirect marker rc case_dir=$(make_case treehouse-return-root-swap) @@ -3805,10 +3808,10 @@ test_treehouse_return_stays_bound_to_validated_root() { moved="$case_dir/moved-worktree" redirect="$case_dir/redirect-target" marker="$case_dir/bound-root-observed" - mkdir -p "$case_dir/wt/retained-descendant" + mkdir -p "$case_dir/wt/validated-descendant" printf 'validated worktree\n' > "$case_dir/wt/.bound-root-identity" - printf 'retained descendant\n' > "$case_dir/wt/retained-descendant/identity" - git -C "$case_dir/wt" add .bound-root-identity retained-descendant/identity + printf 'validated descendant\n' > "$case_dir/wt/validated-descendant/identity" + git -C "$case_dir/wt" add .bound-root-identity validated-descendant/identity git -C "$case_dir/wt" -c user.email=t@t -c user.name=t \ commit -qm "record bound worktree identity" add_fork_with_pushed_branch "$case_dir" @@ -3819,20 +3822,16 @@ test_treehouse_return_stays_bound_to_validated_root() { [ "$3" = "." ] || exit 92 [ "$FM_TREEHOUSE_RETURN_PROJECT" = "$FM_TREEHOUSE_EXPECT_PROJECT" ] || exit 93 bound_target=$3 -old_ifs=$IFS -IFS=, -set -- $FM_TREEHOUSE_RETURN_BOUNDARY_FDS -IFS=$old_ifs -[ "$#" -ge 2 ] || exit 95 -for descriptor in "$@"; do - [ -d "/dev/fd/$descriptor" ] || exit 96 -done +root_descriptor=${FM_TREEHOUSE_RETURN_ROOT_FD:-} +[ -n "$root_descriptor" ] || exit 95 +[ "$FM_TREEHOUSE_RETURN_BOUNDARY_FDS" = "$root_descriptor" ] || exit 96 +[ -d "/dev/fd/$root_descriptor" ] || exit 99 mv "$FM_TREEHOUSE_SWAP_TARGET" "$FM_TREEHOUSE_MOVED_TARGET" || exit 92 mkdir -p "$FM_TREEHOUSE_REDIRECT_TARGET" || exit 93 printf 'redirect target\n' > "$FM_TREEHOUSE_REDIRECT_TARGET/.bound-root-identity" ln -s "$FM_TREEHOUSE_REDIRECT_TARGET" "$FM_TREEHOUSE_SWAP_TARGET" || exit 94 [ "$(cat "$bound_target/.bound-root-identity")" = "validated worktree" ] || exit 97 -[ "$(cat "$bound_target/retained-descendant/identity")" = "retained descendant" ] || exit 98 +[ "$(cat "$bound_target/validated-descendant/identity")" = "validated descendant" ] || exit 98 : > "$FM_TREEHOUSE_BOUND_MARKER" exit 0 SH @@ -3851,7 +3850,44 @@ SH "Treehouse return did not execute from the validated worktree descriptor: $(cat "$case_dir/stderr")" assert_grep 'redirect target' "$redirect/.bound-root-identity" \ "Treehouse return traversed the replacement symlink target" - pass "Treehouse return remains bound across pathname replacement" + pass "the retained root descriptor binds Treehouse across pathname replacement" +} + +test_treehouse_return_validates_more_than_42000_directories() { + local case_dir marker rc + case_dir=$(make_case treehouse-return-directory-capacity) + write_meta "$case_dir" local-only ship + marker="$case_dir/treehouse-return-reached" + python3 - "$case_dir/wt" <<'PY' +import os +import sys + +root = sys.argv[1] +for index in range(42001): + os.mkdir(os.path.join(root, f"capacity-{index:05d}")) +PY + add_fork_with_pushed_branch "$case_dir" + rm -f "$case_dir/fakebin/.tmux-live" + cat > "$case_dir/fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +[ "$1" = return ] && [ "$2" = --force ] && [ "$3" = "." ] || exit 91 +root_descriptor=${FM_TREEHOUSE_RETURN_ROOT_FD:-} +[ -n "$root_descriptor" ] || exit 92 +[ "$FM_TREEHOUSE_RETURN_BOUNDARY_FDS" = "$root_descriptor" ] || exit 93 +[ -d "/dev/fd/$root_descriptor" ] || exit 94 +: > "$FM_TREEHOUSE_CAPACITY_MARKER" +exit 17 +SH + chmod +x "$case_dir/fakebin/treehouse" + set +e + FM_TREEHOUSE_CAPACITY_MARKER="$marker" \ + run_teardown "$case_dir" --force > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "failed large Treehouse return should retain the worktree" + assert_present "$marker" \ + "Treehouse validation exhausted descriptors before traversing 42,001 directories" + pass "Treehouse return validates more than 42,000 directories without retaining descendant descriptors" } test_teardown_distinguishes_dead_and_live_harness_processes() { @@ -4444,6 +4480,16 @@ if [ "${FM_TEST_FOCUSED:-}" = review-round-13-network ]; then exit 0 fi +if [ "${FM_TEST_FOCUSED:-}" = treehouse-return-root-swap ]; then + test_treehouse_return_stays_bound_to_validated_root + exit 0 +fi + +if [ "${FM_TEST_FOCUSED:-}" = treehouse-return-directory-capacity ]; then + test_treehouse_return_validates_more_than_42000_directories + exit 0 +fi + test_local_only_fork_remote_allows test_teardown_prompts_tasks_axi_done_when_compatible test_teardown_manual_backend_prompts_hand_edit_even_when_tasks_axi_present @@ -4496,6 +4542,7 @@ test_secondmate_retirement_rejects_in_home_remote_object_storage test_secondmate_retirement_rejects_source_common_dir_in_home test_teardown_removal_roots_fail_closed test_treehouse_return_stays_bound_to_validated_root +test_treehouse_return_validates_more_than_42000_directories test_teardown_distinguishes_dead_and_live_harness_processes test_secondmate_retirement_retains_reflog_and_rewritten_history test_secondmate_retirement_rejects_http_proxy_and_object_redirects