fix(shell): prevent double-free of ShellArgs in error path

[robobun]

Summary

• Fixes a double-free bug in the shell interpreter error handling path

What Changed

When interpreter.init() succeeds but globalThis.hasException() is true, the code was calling shargs.deinit() before interpreter.finalize(). However, interpreter.args points to shargs after init() succeeds, so calling interpreter.finalize() -> deinitFromFinalizer() -> this.args.deinit() would then try to deinit an already-freed ShellArgs, causing a double-free.

This is a related issue to #24368, which reported crashes during GC finalization of ShellInterpreter objects. While the main fix for #24368 was added in v1.3.6 (commit 367eeb3), this fixes an additional double-free bug in the error handling path.

Test plan

• Added regression tests in test/regression/issue/24368.test.ts
• Tests pass with bun bd test test/regression/issue/24368.test.ts
• Basic shell functionality verified

Fixes #24368

🤖 Generated with Claude Code

[robobun]

^{Updated 6:27 PM PT - Jan 19th, 2026}

❌ @Jarred-Sumner, your commit 46a3984 has 3 failures in Build #35348 (All Failures):

• test/integration/next-pages/test/dev-server.test.ts - 1 failing on 🍎 13 aarch64
• test/regression/issue/11297/11297.test.ts - 1 failing on 🪟 2019 x64-baseline
• cmake/targets/BuildBun.cmake - cmake error on 🐧 x64-musl-baseline - build-zig
• cmake/targets/BuildBun.cmake - cmake error on 🐧 x64-asan - build-cpp

---

🧪   To try this PR locally:

bunx bun-pr 26277

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

bun-26277 --bun

[coderabbitai]

Caution

Review failed

The pull request is closed.

Walkthrough

The change removes an explicit shargs.deinit() call from the error-handling path after interpreter initialization, delegating all resource cleanup to the interpreter's finalize() method to prevent double-deallocation of shell arguments.

Changes

Cohort / File(s)	Summary
Shell interpreter error-path cleanup src/shell/interpreter.zig	Removed redundant shargs.deinit() in error-handling path; interpreter.finalize() now handles deallocation of interpreter.args (which points to shargs). Eliminates double-free risk.

Possibly related PRs

• fix(shell): prevent double-free of ShellArgs in error path #26277: Makes identical code-level change in src/shell/interpreter.zig to remove shargs.deinit() call and rely on interpreter.finalize() for cleanup.
• Fixes #23177 #25936: Modifies src/shell/interpreter.zig deinitialization and finalization logic to adjust resource teardown responsibility between explicit cleanup and finalizer methods.
• Fix double-free in createArgv function #22978: Fixes similar redundant deallocation issue by removing duplicate free calls for argument buffers in related deallocation paths.

Suggested reviewers

• zackradisic
• nektro

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: preventing a double-free bug in the ShellArgs error handling path, which directly addresses the changes made in the PR.
Description check	✅ Passed	The PR description comprehensively covers the required sections: it explains what the PR does (fixes double-free bug), what changed (details the error path issue), and how it was verified (regression tests added and passing).
Linked Issues check	✅ Passed	The PR directly addresses issue #24368 by fixing a double-free bug in the shell interpreter error handling path and includes regression tests that verify the fix prevents crashes during GC finalization.
Out of Scope Changes check	✅ Passed	All changes are scoped to addressing issue #24368: the fix in interpreter.zig corrects the error path cleanup, and regression tests in 24368.test.ts validate the fix against GC stress scenarios.

_{✏️ 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.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/shell/interpreter.zig (1)

747-754: Finalize here skips root IO/root shell cleanup before destroying the interpreter.

At Line 747–754, interpreter.finalize() runs deinitFromFinalizer(), but this_jsvalue is still .zero, so #derefRootShellAndIOIfNeeded() won’t call root_io.deref() or root_shell.deinitImpl(). That leaks the dup’d stdin/cwd fd and env maps on this error path. Prefer a cleanup path that releases IO/root_shell plus args before destroying the interpreter.

💡 Proposed fix

-            // Note: Don't call shargs.deinit() here - interpreter.finalize() will do it
-            // since interpreter.args points to shargs after init() succeeds.
-            interpreter.finalize();
+            // Fully release args + IO/root_shell before destroying the interpreter.
+            interpreter.args.deinit();
+            interpreter.#deinitFromExec();

🤖 Fix all issues with AI agents

In `@test/regression/issue/24368.test.ts`:
- Around line 9-18: The test holds ShellPromise objects in the results array
when Bun.gc(true) is called, preventing finalizers from running reliably; after
awaiting Promise.all(results) clear references by emptying or nulling the
results array (e.g., set results.length = 0 or assign null) so
ShellPromise/ShellInterpreter objects can become unreachable before calling
Bun.gc(true) in the test "shell interpreter GC finalization does not crash".

[robobun]

Addressed the review feedback: Updated the test to clear the results array after awaiting Promise.all() so that ShellPromise/ShellInterpreter objects can become unreachable before calling Bun.gc(true).