node:zlib: block worker shutdown on in-flight async compression (UAF) - #35155
node:zlib: block worker shutdown on in-flight async compression (UAF)#35155robobun wants to merge 8 commits into
Conversation
worker.terminate() while an async zlib/brotli/zstd write is running on the thread pool crashed with a cross-thread heap-use-after-free: the pool-thread completion (CompressionStream::async_job_run) dereferenced global_this -> bun_vm_concurrently() -> event_loop() after WebWorker::shutdown had already dealloc'd the VirtualMachine box that the EventLoop is a field of. do_work() itself was also reading/writing JSC-heap-backed input/output buffers that teardownJSCVM freed. Fix: add an EventLoop::work_pool_pending counter. zlib's write() brackets the WorkPool hop with work_pool_task_ref()/unref() (unref is the pool thread's last VM access, Release-ordered). WebWorker::shutdown spins on the counter reaching zero (Acquire) after stopping cross-thread CppTask posters and before release_queued_tasks_for_shutdown / teardownJSCVM / VM dealloc, so the JSC heap and VM box are live for the pool thread's whole callback. This is a targeted fix for the zlib site; #34154 is the general ShutdownGate that covers every WorkPool/HTTP producer.
|
Updated 1:03 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 12f4d39 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35155That installs a local version of the PR into your bun-35155 --bun |
WalkthroughChangesWorkPool shutdown synchronization
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/node/zlib/zlib-worker-terminate.test.ts`:
- Around line 4-16: Remove the entire regression narrative comment above the
test, including the heap-use-after-free explanation, stack traces, and
implementation-specific details. Leave the test code unchanged and do not add a
replacement comment unless a confirmed issue URL is available.
- Around line 47-49: Update the worker lifecycle in the test around the message
wait and w.terminate() so termination happens immediately when the "up" message
arrives, removing the randomized sleep. Make the readiness promise reject on
worker error or an exit occurring before "up", while preserving successful
resolution once "up" is received.
🪄 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: 6118a14a-0f6d-4470-aaef-84dd41b98edd
📒 Files selected for processing (4)
src/jsc/event_loop.rssrc/jsc/web_worker.rssrc/runtime/node/node_zlib_binding.rstest/js/node/zlib/zlib-worker-terminate.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/zlib/zlib-worker-terminate.test.ts (1)
33-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not swallow all compression failures.
Line 35 retries forever after every failure, while Line 40 reports
"up"unconditionally. A broken or unsupported zlib path can therefore still producestdout: "ok"without exercising successful asynchronous compression. Propagate the first error or make readiness depend on a successful initial operation.As per coding guidelines: tests must prove they fail for the intended reason, and failures must not be swallowed.
Suggested failure propagation
- (async () => { for (;;) { try { await f(); } catch {} } })(); + (async () => { + for (;;) await f(); + })().catch(error => { + console.error(error); + process.exit(1); + });🤖 Prompt for 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. In `@test/js/node/zlib/zlib-worker-terminate.test.ts` around lines 33 - 40, Update the lanes helper and readiness signaling around gz, br, and df so compression errors are not silently swallowed: propagate the first failure or require each lane to complete an initial successful compression before posting "up". Ensure parentPort.postMessage("up") only occurs after asynchronous compression succeeds, while preserving the ongoing retry behavior only for subsequent operations if needed.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/node/zlib/zlib-worker-terminate.test.ts`:
- Around line 33-40: Update the lanes helper and readiness signaling around gz,
br, and df so compression errors are not silently swallowed: propagate the first
failure or require each lane to complete an initial successful compression
before posting "up". Ensure parentPort.postMessage("up") only occurs after
asynchronous compression succeeds, while preserving the ongoing retry behavior
only for subsequent operations if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc7f47fa-91b7-4a9c-ad63-4634d98f710d
📒 Files selected for processing (1)
test/js/node/zlib/zlib-worker-terminate.test.ts
The sleep was not load-bearing: 5 in-flight compressions are already on the pool by the time 'up' arrives (verified 3/3 UAF on unfixed ASAN build with no sleep).
|
Addressed in bb6a570:
On the duplicate-PR bot: #34154 is the general |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The barrier guarantees every in-flight async write posts its completion before release_queued_tasks_for_shutdown runs, but __bun_release_task_at_shutdown had no NativeZlib/Brotli/Zstd arm, so the payload was re-queued and the CompressionStream box (with its Strong this_value, ref'd poll_ref, pinned buffers, and write()'s +1) leaked when the worker VM box was raw-dealloc'd. Add CompressionStream::release_unrun (the resource-release subset of run_from_js_thread, no JS callbacks) and wire it into the shutdown drain for all three tags. LSan with detect_leaks=1 now shows no CompressionStream/Native* allocations in the repro. Also tighten the test's sanitizer check to 'ERROR: AddressSanitizer' so unrelated pre-existing LSan summaries (WebWorker box, fs Binding) can't trip it.
|
CI status:
Diff is ready for review. |
|
Verified this barrier also closes the sibling face where Repro: a worker that The existing test's |
|
Superseded by #36983, which generalizes this shutdown fence to every off-thread job family (the counter lives on EventLoop as outstanding_offthread) and carries this PR's zlib bracketing, release_unrun arms, and test verbatim. Closing in favor of that PR. |
2 participants
Problem
worker.terminate()while an asyncnode:zliboperation (gzip/brotliCompress/deflate/zstd) is running on the thread pool crashes with a cross-thread heap-use-after-free:The pool-thread completion callback walks
global_this -> bun_vm_concurrently() -> event_loop()to post its result afterWebWorker::shutdownhas already raw-dealloc'd theVirtualMachinebox (theEventLoopis a value field of it).do_work()itself also reads/writes the pinned JSC-heap input/output buffers thatteardownJSCVMfrees. Under a release build this is a plain SIGSEGV.Worker
process.exit()and an uncaught throw take the same shutdown path and hit the same race.Fix
Add a small shutdown barrier on
EventLoop:work_pool_pending: AtomicU32counts WorkPool jobs scheduled from this VM's JS thread whose pool-thread callback has not yet made its lastEventLoop/VM access.CompressionStream::write()callswork_pool_task_ref()immediately beforeWorkPool::schedule;async_job_runcallswork_pool_task_unref()(Release) as its final VM access, afterenqueue_task_concurrent.WebWorker::shutdownspins onwork_pool_pending == 0(Acquire) after stopping the cross-thread CppTask posters and beforerelease_queued_tasks_for_shutdown/teardownJSCVM/ the VMdealloc. The Release/Acquire pair guarantees the VM box and JSC heap are live for the pool thread's whole callback, and the completion each job posts is then reclaimed by the existing drain.Each pending job is one bounded compression step, so
terminate()latency is bounded by the slowest in-flight chunk (the same model Node'suv_rundrain gives).Scope
This is a targeted fix for the
node:zlibsite only. #34154 is the generalShutdownGatethat covers every WorkPool/HTTP-thread producer (fetch, S3,node:fsasync, dns, napi, crypto, bundler, ...); the counter here is the minimal subset of that design and will be subsumed when it lands. #32073 tracks the generation-token follow-up.Test
test/js/node/zlib/zlib-worker-terminate.test.tsspawns a worker that keeps 5 lanes of gzip/brotli/deflate in flight and terminates it mid-compression, 4 rounds under ASAN / 10 otherwise.Fail-before / pass-after
[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file