mirror of
https://github.com/oven-sh/bun
synced 2026-02-02 15:08:46 +00:00
fix(shell): prevent double-free during GC finalization (#26626)
## 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 - [x] Build passes (`bun bd`) - [x] New regression test added (`test/regression/issue/26625.test.ts`) - [x] Existing shell tests pass (same 4 pre-existing failures, no new failures) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Bot <claude-bot@bun.sh> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
This commit is contained in:
@@ -280,6 +280,17 @@ pub const Interpreter = struct {
|
|||||||
exit_code: ?ExitCode = 0,
|
exit_code: ?ExitCode = 0,
|
||||||
this_jsvalue: JSValue = .zero,
|
this_jsvalue: JSValue = .zero,
|
||||||
|
|
||||||
|
/// Tracks which resources have been cleaned up to avoid double-free.
|
||||||
|
/// When the interpreter finishes normally via finish(), it cleans up
|
||||||
|
/// the runtime resources (IO, shell env) and sets this to .runtime_cleaned.
|
||||||
|
/// The GC finalizer then only cleans up what remains (args, interpreter itself).
|
||||||
|
cleanup_state: enum(u8) {
|
||||||
|
/// Nothing has been cleaned up yet - need full cleanup
|
||||||
|
needs_full_cleanup,
|
||||||
|
/// Runtime resources (IO, shell env) have been cleaned up via finish()
|
||||||
|
runtime_cleaned,
|
||||||
|
} = .needs_full_cleanup,
|
||||||
|
|
||||||
__alloc_scope: if (bun.Environment.enableAllocScopes) bun.AllocationScope else void,
|
__alloc_scope: if (bun.Environment.enableAllocScopes) bun.AllocationScope else void,
|
||||||
estimated_size_for_gc: usize = 0,
|
estimated_size_for_gc: usize = 0,
|
||||||
|
|
||||||
@@ -1222,6 +1233,11 @@ pub const Interpreter = struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn #derefRootShellAndIOIfNeeded(this: *ThisInterpreter, free_buffered_io: bool) void {
|
fn #derefRootShellAndIOIfNeeded(this: *ThisInterpreter, free_buffered_io: bool) void {
|
||||||
|
// Check if already cleaned up to prevent double-free
|
||||||
|
if (this.cleanup_state == .runtime_cleaned) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
if (free_buffered_io) {
|
if (free_buffered_io) {
|
||||||
// Can safely be called multiple times.
|
// Can safely be called multiple times.
|
||||||
if (this.root_shell._buffered_stderr == .owned) {
|
if (this.root_shell._buffered_stderr == .owned) {
|
||||||
@@ -1240,10 +1256,26 @@ pub const Interpreter = struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
this.this_jsvalue = .zero;
|
this.this_jsvalue = .zero;
|
||||||
|
// Mark that runtime resources have been cleaned up
|
||||||
|
this.cleanup_state = .runtime_cleaned;
|
||||||
}
|
}
|
||||||
|
|
||||||
fn deinitFromFinalizer(this: *ThisInterpreter) void {
|
fn deinitFromFinalizer(this: *ThisInterpreter) void {
|
||||||
this.#derefRootShellAndIOIfNeeded(true);
|
log("Interpreter(0x{x}) deinitFromFinalizer (cleanup_state={s})", .{ @intFromPtr(this), @tagName(this.cleanup_state) });
|
||||||
|
|
||||||
|
switch (this.cleanup_state) {
|
||||||
|
.needs_full_cleanup => {
|
||||||
|
// The interpreter never finished normally (e.g., early error or never started),
|
||||||
|
// so we need to clean up IO and shell env here
|
||||||
|
this.root_io.deref();
|
||||||
|
this.root_shell.deinitImpl(false, true);
|
||||||
|
},
|
||||||
|
.runtime_cleaned => {
|
||||||
|
// finish() already cleaned up IO and shell env via #derefRootShellAndIOIfNeeded,
|
||||||
|
// nothing more to do for those resources
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
this.keep_alive.disable();
|
this.keep_alive.disable();
|
||||||
this.args.deinit();
|
this.args.deinit();
|
||||||
this.allocator.destroy(this);
|
this.allocator.destroy(this);
|
||||||
|
|||||||
Reference in New Issue
Block a user