Skip to content

fix(shell): prevent double-free during GC finalization#26626

Merged
Jarred-Sumner merged 2 commits intomainfrom
claude/fix-shell-gc-double-free-26625
Feb 1, 2026
Merged

fix(shell): prevent double-free during GC finalization#26626
Jarred-Sumner merged 2 commits intomainfrom
claude/fix-shell-gc-double-free-26625

Conversation

@robobun
Copy link
Collaborator

@robobun robobun commented Jan 31, 2026

Summary

Fixes #26625

This fixes a segmentation fault that occurred on Windows x64 when the GC finalizer tried to free shell interpreter resources that were already partially freed during normal shell completion.

  • Added explicit cleanup_state enum to track resource ownership state
  • needs_full_cleanup: Nothing cleaned up yet, finalizer must clean everything
  • runtime_cleaned: finish() already cleaned IO/shell, finalizer skips those
  • Early return in #derefRootShellAndIOIfNeeded() when already cleaned
  • Explicit state-based cleanup in deinitFromFinalizer()

The vulnerability existed on all platforms but was most reliably triggered on Windows with high GC pressure (many concurrent shell commands).

Test plan

  • Build passes (bun bd)
  • New regression test added (test/regression/issue/26625.test.ts)
  • Existing shell tests pass (same 4 pre-existing failures, no new failures)

🤖 Generated with Claude Code

Fixes #26625

This fixes a segmentation fault that occurred when the GC finalizer tried to
free shell interpreter resources that were already partially freed during
normal shell completion.

The issue happened in this sequence:
1. Shell script completes, `finish()` calls `#derefRootShellAndIOIfNeeded()`
   which frees root_io and root_shell, then sets `this_jsvalue = .zero`
2. GC runs later and calls `deinitFromFinalizer()`, which called
   `#derefRootShellAndIOIfNeeded()` again
3. The guard `if (this_jsvalue != .zero)` was unreliable for preventing
   double-free in all scenarios

The fix adds an explicit `cleanup_state` enum to track resource ownership:
- `needs_full_cleanup`: Nothing cleaned up yet, finalizer must clean everything
- `runtime_cleaned`: finish() already cleaned IO/shell, finalizer skips those

This was reported on Windows x64 with high GC pressure (many concurrent shell
commands), but the vulnerability existed on all platforms.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@robobun
Copy link
Collaborator Author

robobun commented Jan 31, 2026

Updated 6:21 PM PT - Jan 31st, 2026

@Jarred-Sumner, your commit 19b5a3f has 1 failures in Build #36321 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 26626

That installs a local version of the PR into your bun-26626 executable, so you can run:

bun-26626 --bun

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 31, 2026

Walkthrough

The changes add a cleanup state tracking mechanism to the shell interpreter to prevent double-free crashes during garbage collection finalization. A new cleanup_state enum field tracks whether resources need full cleanup or have already been cleaned, allowing the finalizer to skip redundant cleanup steps. Regression tests are added to verify the fix under high GC pressure.

Changes

Cohort / File(s) Summary
Shell Interpreter Finalization
src/shell/interpreter.zig
Introduces cleanup_state enum field with two states (needs_full_cleanup, runtime_cleaned) to track resource cleanup progress. Modifies derefRootShellAndIOIfNeeded to early-out if cleanup already occurred and deinitFromFinalizer to conditionally perform cleanup based on state, preventing double-free during GC finalization.
Regression Tests
test/regression/issue/26625.test.ts
Adds two regression tests for issue 26625 that spawn concurrent shell commands, stress GC with multiple Bun.gc(true) invocations, and verify no crashes occur during finalization under high GC pressure.

Possibly related PRs

  • #26277: Modifies shell interpreter finalization to prevent double-free by removing redundant resource cleanup operations, complementing the state-based tracking approach in this PR.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: preventing double-free during GC finalization in the shell module.
Description check ✅ Passed The PR description covers all required template sections with clear explanations of what changed, how it was verified (build, new regression test, existing tests), and is well-documented.
Linked Issues check ✅ Passed The changes address issue #26625 by adding cleanup_state tracking to prevent double-free during GC finalization, implementing the required fix for the segmentation fault on Windows x64 under high GC pressure.
Out of Scope Changes check ✅ Passed All changes directly address the double-free vulnerability: cleanup_state enum addition, early-return logic, and state-based cleanup in deinitFromFinalizer. The regression test validates the fix without introducing unrelated changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@narukeu
Copy link

narukeu commented Jan 31, 2026

I've tried running the build from this PR on Windows three times: it crashed the first time, worked normally the second time, and crashed again the third time.

@Jarred-Sumner Jarred-Sumner merged commit 56b5be4 into main Feb 1, 2026
3 of 5 checks passed
@Jarred-Sumner Jarred-Sumner deleted the claude/fix-shell-gc-double-free-26625 branch February 1, 2026 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue with Bun's GC Mechanism: The Bun bundled with the Windows version of OpenCode crashes during LLM code analysis

3 participants