JSBuffer: allow TerminationException in bufferFromPointerAndLengthAndDeinit's no-exception assert - #36819
JSBuffer: allow TerminationException in bufferFromPointerAndLengthAndDeinit's no-exception assert#36819robobun wants to merge 5 commits into
Conversation
…Deinit's no-exception assert JSUint8Array::create allocates, which can service VMTraps and observe the TerminationException a concurrent worker.terminate() installed. The assertNoException() that follows it SIGABRTs a debug build when an async crypto.pbkdf2() completion (Pbkdf2Ctx::then -> JSValue::create_buffer) runs on a worker that has just been terminated. Use assertNoExceptionExceptTermination() like the other allocation-adjacent call sites (ModuleLoader, ZigGlobalObject, JSDOMExceptionHandling, etc.).
WalkthroughChangesWorker termination buffer handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line relaxation of a debug-only assert to the termination-aware variant, with a regression test that mirrors the five sibling tests already in this file.
What was reviewed:
- Confirmed
assertNoExceptionExceptTermination()is the established pattern at allocation-adjacent assert sites across the bindings (ModuleLoader, ZigGlobalObject, JSCommonJSModule, ObjectBindings, headers-handwritten.h'sASSERT_NO_PENDING_EXCEPTION, etc.). - Checked the test is hermetic, drains pipes concurrently, wires
error/exitinto the readiness promise, and isskipIf(!isDebug)since release WebKit compiles the assert out. - Verified the
length == 0branch still callsbytesDeallocatorbefore the (unchanged) create call, so no ownership regression.
Extended reasoning...
Overview
The PR changes one line in src/jsc/bindings/JSBuffer.cpp: scope.assertNoException() → scope.assertNoExceptionExceptTermination() at the tail of JSBuffer__bufferFromPointerAndLengthAndDeinit. It adds a ~60-line debug-only regression test to test/js/web/workers/worker-terminate-lifetime.test.ts that terminates workers mid-crypto.pbkdf2() completion to exercise the VMTraps-during-allocation path.
Security risks
None. This only relaxes a debug-build assertion to tolerate the sticky TerminationException that JSC installs when a worker is terminated. Release builds compile the assert out entirely, so there is no runtime behavior change in shipped binaries. The test is hermetic (no network, no filesystem beyond in-memory worker eval).
Level of scrutiny
Low. The src change is a mechanical swap to an established idiom — grep shows assertNoExceptionExceptTermination() already used in 10+ bindings files for exactly this reason (allocation can service VMTraps). The comment above the assert is updated to name the new invariant. The length == 0 branch that frees ptr via bytesDeallocator is untouched.
Other factors
The test is a near-verbatim copy of the pattern used by the five other terminate()-vs-TerminationException regression tests already in the same file (same ready() helper wiring error/exit to reject, same round/worker fan-out, same await Promise.all([stdout, stderr, exited]), same skipIf(!isDebug) gating with a comment explaining why). The PR description includes fail-before / pass-after output. No CODEOWNERS entries cover these paths, and there are no outstanding reviewer comments.
ENABLE(EXCEPTION_SCOPE_VERIFICATION) = ASSERT_ENABLED || ASAN_ENABLED, and the release-asan build sets both, so the assert is compiled in there too. CI has no debug lane, so the previous skipIf(!isDebug) would have skipped the test everywhere. Also reword the comments: allocation is not a VMTraps checkpoint; the TerminationException is installed at an earlier bytecode trap check (in a prior lane's JS callback) and merely observed here.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 485-503: In the worker source used by the test, move
parentPort.postMessage("up") to after the loop that invokes lane() so all PBKDF2
jobs are queued before readiness is reported. Keep Bun.sleep(r % 7) solely as
stress variation and do not use it for synchronization; preserve the existing
ready and worker-termination flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90684f8d-e9df-4476-b8bb-08fa3b92b22b
📒 Files selected for processing (2)
src/jsc/bindings/JSBuffer.cpptest/js/web/workers/worker-terminate-lifetime.test.ts
There was a problem hiding this comment.
LGTM — one-token swap to assertNoExceptionExceptTermination(), matching the pattern already used across the bindings.
What was reviewed:
- Confirmed
assertNoExceptionExceptTermination()is the established idiom at 10+ sibling sites (ModuleLoader, ZigGlobalObject, JSCommonJSModule, ObjectBindings, headers-handwritten.h'sASSERT_NO_PENDING_EXCEPTION). - Test mirrors the existing worker-terminate-lifetime.test.ts shape exactly:
skipIf(!isDebug && !isASAN)gate,ready()wires error/exit to reject, pipes drained concurrently, stderr→stdout→exitCode order. - Both prior bot comments (comment-cop paragraph comment, coderabbit "up" ordering) are resolved in the current diff.
Extended reasoning...
Overview
One-line change in src/jsc/bindings/JSBuffer.cpp: scope.assertNoException() → scope.assertNoExceptionExceptTermination() in JSBuffer__bufferFromPointerAndLengthAndDeinit. Plus a new debug/ASAN-gated regression test in test/js/web/workers/worker-terminate-lifetime.test.ts that terminates workers while async crypto.pbkdf2() completions are allocating result Buffers.
Security risks
None. This relaxes a debug-only assertion to permit the sticky TerminationException (which is expected during worker.terminate()); it does not change release-build behavior, allocation, ownership, or any user-reachable path.
Level of scrutiny
Low. The native change is a single-token swap to a JSC helper that is already the repo convention for allocation-adjacent assert sites — I found it in 10 other bindings files (ModuleLoader.cpp, ZigGlobalObject.cpp, JSDOMExceptionHandling.cpp, JSCommonJSModule.cpp, bindings.cpp, ObjectBindings.cpp, BunPlugin.cpp, NodeVMScript.cpp, NodeVMSourceTextModule.cpp, and the ASSERT_NO_PENDING_EXCEPTION macro in headers-handwritten.h). The assert is compiled out in release builds, so the fix only affects debug/ASAN process-abort behavior.
Other factors
The test is a near-verbatim clone of the six sibling regression tests already in the same file: same skipIf gate rationale, same subprocess-spawn shape, same ready() helper that wires error/exit to reject, concurrent pipe drain via Promise.all, and the file's canonical stderr→stdout→exitCode assertion order. The PR description shows fail-before/pass-after on a debug build. All prior review threads on the PR (comment-cop on the C++ comment length, coderabbit on postMessage("up") ordering) are resolved in commits 92ae6e4 and 65fa330, and the current diff reflects those fixes. The ~31s debug+ASAN runtime with a 120s timeout is in line with the other tests in this file.
There was a problem hiding this comment.
LGTM — one-token swap to assertNoExceptionExceptTermination(), matching the established pattern across the bindings.
What was reviewed
- Confirmed
assertNoExceptionExceptTermination()is the existing idiom at 10+ sibling sites insrc/jsc/bindings/(ModuleLoader, ZigGlobalObject, JSCommonJSModule, headers-handwritten.h'sASSERT_NO_PENDING_EXCEPTION, etc.). - New test mirrors the sibling
skipIf(!isDebug)tests in the same file: subprocess, concurrent pipe drain, stderr→stdout→exitCode ordering, error/exit wired to reject the readiness promise. - Both bot review threads (comment-cop on the C++ comment, coderabbit on "up" ordering) are addressed and resolved in the current diff.
Extended reasoning...
Overview
One-line change in src/jsc/bindings/JSBuffer.cpp: scope.assertNoException() → scope.assertNoExceptionExceptTermination() in JSBuffer__bufferFromPointerAndLengthAndDeinit. Plus a new debug-only regression test appended to test/js/web/workers/worker-terminate-lifetime.test.ts that reproduces the SIGABRT via concurrent worker.terminate() racing async crypto.pbkdf2() completions.
Security risks
None. This weakens a debug/ASAN-only assertion to allow the sticky TerminationException (which is a legitimate cross-thread signal, not an error condition) while still asserting no ordinary exceptions. It does not change release-build behavior and does not touch any trust boundary.
Level of scrutiny
Low. The native change is a single well-known idiom substitution — grep shows the same carve-out already applied at 10+ allocation-adjacent assert sites in src/jsc/bindings/ and codified in ASSERT_NO_PENDING_EXCEPTION in headers-handwritten.h. The PR description traces the exact call chain (Pbkdf2Ctx::then → JSValue::create_buffer → this function) and explains why the original assert's premise ("only JSUint8Array::create can throw") holds for ordinary exceptions but not for a pre-installed termination trap. The test is a near-verbatim copy of the four sibling termination-race tests already in the file, gated skipIf(!isDebug) with the reason for not widening to release+ASAN documented (blocked on #36817).
Other factors
All prior review feedback is resolved: the comment-cop paragraph-comment complaint was reverted to the original one-liner in 92ae6e4, and the coderabbit "post 'up' after queuing lanes" suggestion was applied in 65fa330 — both visible in the current diff. The test follows house rules: await using, bunEnv/bunExe(), concurrent Promise.all pipe drain, stderr asserted first, readiness promise rejects on error/exit, Bun.sleep used only as jitter after an awaited condition. PR description shows fail-before/pass-after on a debug build.
|
Updated 4:06 AM PT - Aug 3rd, 2026
✅ @robobun, your commit 2ed6d1f1d25544e9568b49f12dc28e044c517282 passed in 🧪 To try this PR locally: bunx bun-pr 36819That installs a local version of the PR into your bun-36819 --bun |
1 participant
JSBuffer__bufferFromPointerAndLengthAndDeinitends withscope.assertNoException()on the grounds that the only thing inside that can throw isJSC::JSUint8Array::createand we control theArrayBufferit wraps. That holds for ordinary exceptions, but a concurrentworker.terminate()can install the stickyTerminationExceptionat a bytecode trap checkpoint on the worker thread before this function is entered, and nothing between the scope declaration and the assert clears it. The assert then SIGABRTs the whole process.Hit from async
crypto.pbkdf2()'s JS-thread completion with several lanes in flight: one lane's JS callback takes the termination trap, the next lane's completion entersJSBuffer__bufferFromPointerAndLengthAndDeinitwith the exception still pending.JSValue::create_buffer/create_buffer_from_box(the Rust wrappers of this C++ function) are also reached from the shell interpreter's stdout/stderr resolve,Bun.gunzipSync/zstdDecompress/inflateSync,TextEncoder, and FFItoBuffer, so any of those on a terminating worker trips it the same way.Use
assertNoExceptionExceptTermination(), matching the other allocation-adjacent assert sites in the bindings (ModuleLoader, ZigGlobalObject, JSDOMExceptionHandling, JSCommonJSModule, bindings.cpp, ObjectBindings) and theASSERT_NO_PENDING_EXCEPTIONhelper inheaders-handwritten.h.Verification
New
skipIf(!isDebug)test inworker-terminate-lifetime.test.ts: 4 workers each keep 8 lanes of 1-iterationpbkdf2()hot, parent terminates mid-stream, 15 rounds. On a debug build without the src/ change:With the change it passes (~31s under debug+ASAN, 3/3 clean).
The assert is also compiled in for release+ASAN (
ENABLE(EXCEPTION_SCOPE_VERIFICATION) = ASSERT_ENABLED || ASAN_ENABLED), but that lane hits the orthogonalAnyTaskJobpool-thread UAF (#36817) that this assert was masking. The gate stays debug-only, matching the sibling tests in this file, and can widen to!isDebug && !isASANonce #36817 lands.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts