bundler: keep BundlerPlugin filter data alive across Worker.terminate() - #36807
bundler: keep BundlerPlugin filter data alive across Worker.terminate()#36807robobun wants to merge 10 commits into
Conversation
The bundle thread reads a plugin's onLoad/onResolve filter regex lists (via JSBundlerPlugin__anyMatches / hasOnBeforeParsePlugins / callOnBeforeParsePlugins) while a build is in flight. Those lists lived inline in the JSBundlerPlugin GC cell, so terminating a Worker that had kicked off a Bun.build() with a plugin freed them out from under the bundle thread: JSBundlerPlugin.cpp:96: runtime error: reference binding to null pointer of type 'Bun::BundlerPlugin::FilterRegExp' BundlerPlugin is now ThreadSafeRefCounted and heap-allocated separately from the GC cell. The cell holds one ref, and the Rust-side Plugin handle (what JSBundleCompletionTask / BundleV2 carry across threads) holds another, so the filter data outlives VM teardown. The Rust opaque now points at the BundlerPlugin heap allocation directly; every extern C entry point routes JS-thread work through plugin->cell() and the cross-thread ones no longer touch the cell at all. FilterRegExp::match dropped its VM& parameter: Yarr::RegularExpression uses the bytecode interpreter and the MatchingContextHolder it constructed was unused.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe plugin bridge now uses a reference-counted ChangesBundlerPlugin ownership migration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…mments; update stale bake prose
…ertion, fix stale Plugin-opaque comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/jsc/bindings/JSBundlerPlugin.h`:
- Around line 134-137: Invalidate the plugin back-pointer in
JSBundlerPlugin::tombstone() by clearing m_cell, and update cell() callers to
handle a null result. In src/jsc/bindings/JSBundlerPlugin.h lines 134-137, apply
the invalidation; in src/jsc/bindings/JSBundlerPlugin.cpp lines 611-616, capture
the cell pointer before tombstoning and skip gcUnprotect when that pointer is
null.
- Around line 80-84: Ensure NativePluginCallback::externalValue remains valid
until every detached parse callback has completed, even during Worker::shutdown.
Synchronize JSC VM teardown with outstanding parse work before releasing
onBeforeParseExternals, or transfer the payload and its finalizer ownership to a
VM-independent lifetime owner; update the shutdown/callback coordination rather
than relying only on JSBundlerPlugin rooting.
In `@src/runtime/api/JSBundler.rs`:
- Line 1638: Update the `JSBundlerPlugin__destroy` declaration and its
corresponding dispatch paths around the additional referenced lines to accept
and forward `*mut Plugin` rather than `&Plugin`; keep the value as a raw pointer
through the destruction call so no shared reference remains while `deref()` may
free the allocation.
In `@test/bundler/bun-build-api.test.ts`:
- Line 1611: Normalize stderr before the clean-exit check in the test’s
execution loop, removing benign ASAN/debug output while preserving meaningful
diagnostics. Use the normalized value in the condition that sets sawCleanExit,
so successful runs with only recognized benign stderr still reach the existing
meaningfulness assertion.
- Around line 1600-1624: Update the subprocess loop around Bun.spawn to record
each attempt’s proc.exitCode and proc.signalCode after awaiting proc.exited,
along with the relevant stdout/stderr outcome. Include this per-attempt
diagnostic data in the meaningfulness assertion for sawSanitizerReport ||
sawCleanExit, while preserving the required stdout/stderr assertions before any
exit-code assertion and the existing frames check.
🪄 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: 90b9d955-749f-482a-8454-665e2c8f239c
📒 Files selected for processing (8)
src/bundler/bundle_v2.rssrc/jsc/bindings/JSBundlerPlugin.cppsrc/jsc/bindings/JSBundlerPlugin.hsrc/runtime/api/JSBundler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/bake_body.rssrc/runtime/bake/mod.rstest/bundler/bun-build-api.test.ts
…Tail in test diagnostics
|
CI on 85687ea (build #87998): both failures are unrelated to this diff.
The new ASAN-gated test in |
…te stale trait doc
|
Updated 11:52 PM PT - Aug 2nd, 2026
✅ @robobun, your commit d53dca045b12e9e4f690629ed150fa1f473aaf56 passed in 🧪 To try this PR locally: bunx bun-pr 36807That installs a local version of the PR into your bun-36807 --bun |
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 `@src/runtime/server/server_body.rs`:
- Around line 1048-1052: The native JSBundler::Plugin handle must be explicitly
destroyed on every lifecycle exit. In src/runtime/server/server_body.rs lines
1048-1052, add a cleanup guard immediately after heap::take and disarm it only
after ownership is assigned to Pending; in src/runtime/server/server_body.rs
lines 1230-1235, bind the pending plugin and call JSBundler::Plugin::destroy
before the debug assertion.
In `@test/bundler/bun-build-api.test.ts`:
- Line 1617: Update the sawCleanExit assignment in the subprocess handling logic
to require proc.exitCode === 0 and proc.signalCode === null in addition to the
existing stdout and stderr checks, so it is only set after a clean successful
exit.
🪄 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: 6e9afdb0-8aac-4f2e-8781-72b5ace99c1a
📒 Files selected for processing (3)
src/runtime/api/JSBundler.rssrc/runtime/server/server_body.rstest/bundler/bun-build-api.test.ts
…deinit does not double-destroy the ServePlugins-owned handle
There was a problem hiding this comment.
No new issues found on d53dca0 — the earlier double-destroy on the cancelled HTML-bundle path is fixed by hoisting this.plugins = None above the cancelled early-return, and the ServePlugins create/destroy pairing now covers all arms. That said, this is a cross-thread memory-lifetime refactor of a JSC GC cell (inline → ThreadSafeRefCounted heap split, new m_cell back-pointer, changed FFI ownership contract on Plugin::create/destroy) with knock-on changes to three separate callers, so it warrants a human look.
What was reviewed:
BundlerPluginrefcount balance across__create/__destroyand cell finalization;m_cellonly dereferenced on the JS thread.- All three
Plugin::createcallers (JSBundleCompletionTask, bakeUserOptions,ServePlugins) now pair withPlugin::destroyon every terminal path including the scopeguard-covered error paths. - The direct
HTMLBundle::State::deinit→JSBundleCompletionTask::deinitpath for a still-borrowed plugin pointer — ruled out (Route self-ref keeps it alive untilon_complete_anytaskhas clearedplugins). NativePluginCallbacknow storesexternal->value()instead of theNapiExternal*cell so the parse thread never reads GC memory.
Extended reasoning...
Overview
This PR fixes a use-after-free where the bundle thread reads BundlerPlugin filter vectors after Worker.terminate() has torn down the owning VM's heap. The fix separates BundlerPlugin from the JSBundlerPlugin GC cell: it becomes a ThreadSafeRefCounted heap allocation, the cell holds one Ref<>, and the Rust-side opaque handle holds a second +1 taken in JSBundlerPlugin__create and released in the new JSBundlerPlugin__destroy. All extern-C entry points are re-typed from JSBundlerPlugin* (cell) to BundlerPlugin* (heap); cross-thread ones (anyMatches, hasOnBeforeParsePlugins, callOnBeforeParsePlugins) no longer touch the cell or VM&, and JS-thread ones go through a new m_cell back-pointer. NativePluginCallback stores the raw void* externalValue captured at append time instead of a NapiExternal* cell pointer. On the Rust side, Plugin::create/destroy forward directly to C++ (no more Rust-side protect()/unprotect()), and every caller of Plugin::create was audited: JSBundleCompletionTask::deinit, bake UserOptions::drop, and ServePlugins (Drop, handle_on_reject, plus a scopeguard around the fallible setup path).
The PR went through several review iterations that surfaced and fixed real issues: a missing destroy in the ServePlugins path (4f281e1), a double-destroy on the cancelled HTML-bundle path where JSBundleCompletionTask::deinit would destroy a borrowed handle that ServePlugins also destroys (d53dca0), stale trait docs, and test-assertion tightening. All threads are resolved.
Security risks
None user-facing beyond memory safety. The change is entirely about cross-thread lifetime of native filter data; no new attack surface, input parsing, or auth logic.
Level of scrutiny
High. This is squarely in REVIEW.md's most-blocked category — native memory safety across threads with a GC-cell back-pointer, a new refcounted allocation whose refs must balance on every terminal path (success, error, cancellation, VM teardown), and a changed FFI ownership contract that touched three independent callers. The fact that the review process itself found a double-destroy that was only reachable via Bun.serve bunfig plugins + HTML route + server.stop() mid-bundle demonstrates the interaction surface is non-trivial. The test is ASAN-gated and deliberately tolerates a different pre-existing UAF (complete_on_bundle_thread, tracked in #35158/#35767), asserting only that no crash frame lands in JSBundlerPlugin.cpp — reasonable given the scope, but it means the fix is verified indirectly.
Other factors
Yarr::MatchingContextHolderwas removed fromFilterRegExp::matchon the basis thatYarr::RegularExpression::matchruns the bytecode interpreter and never consults it — this looks correct but is a behaviour claim about JSC internals a human familiar with Yarr should confirm.WriteBarrierList<>members (deferredPromises,onBeforeParseExternals) now live on the heap-allocatedBundlerPluginrather than inline in the cell, but are still visited viaplugin->deferredPromises.visit(this, visitor)from the cell'svisitChildren— this is fine while the cell holds aRef<>but worth a second pair of eyes on the GC-visitor contract.- CI on the last pre-fix commit (85687ea) was green on the ASAN lane; d53dca0 build #88013 was still running per the last robobun update.
1 participant
Problem
Terminating a Worker while a
Bun.build()with anonLoad/onResolveplugin is running on the singleton bundle thread trips a null-pointer dereference reading the plugin's filter regex list:With
Malloc=1the same read is a heap-use-after-free inJSBundlerPlugin__anyMatches(JSBundlerPlugin.cpp:512,pluginObject->vm()).Cause
BundlerPlugin(the filter vectors plus callbacks) was an inline member of theJSBundlerPluginGC cell. The Rust-sidePluginopaque thatJSBundleCompletionTask/BundleV2carry across to the bundle thread was a raw pointer into that cell.gcProtectkeeps the cell alive through a normal GC, butWebWorker::shutdownruns~VM(), which tears down the JSC heap regardless. The bundle thread then reads freed cell bytes fromJSBundlerPlugin__anyMatches,JSBundlerPlugin__hasOnBeforeParsePluginsandJSBundlerPlugin__callOnBeforeParsePlugins.Fix
BundlerPluginis nowThreadSafeRefCountedand heap-allocated separately from the GC cell:JSBundlerPluginholds aRef<BundlerPlugin>.JSBundlerPlugin__creategcProtects the cell, takes a +1 on theBundlerPlugin, and returns theBundlerPlugin*(what the RustPluginopaque now points at).anyMatches,hasOnBeforeParsePlugins,callOnBeforeParsePlugins) operate on theBundlerPlugindirectly and never touch the cell.matchOnLoad,drainDeferred,runSetupFunction, ...) go throughplugin->cell()forvm()/globalObject()/LazyPropertyaccess; these only run while the JS thread (and hence the VM) is alive.JSBundlerPlugin__destroydoestombstone() + gcUnprotect(cell) + deref();PluginJscExt::create/destroyforward straight to the C++ side.FilterRegExp::matchandNativePluginList::callno longer takeVM&:Yarr::RegularExpression::matchruns the bytecode interpreter and theMatchingContextHolderthat was constructed around it was never consulted.Verification
New ASAN-gated test in
test/bundler/bun-build-api.test.tsspawns a subprocess that repeatedly starts Workers which loopBun.build(300-module graph, synchronoussetupregistering anonLoadfilter) and terminates them mid-build. The test runs up to eight subprocess attempts and asserts no sanitizer frame lands inJSBundlerPlugin.cpp.JSBundlerPlugin__anyMatchesin the stack on 2 of the first 4 attempts.JSBundlerPlugin.cpp.bun-build-api.test.ts(50 pass, 1 skip, 1 todo),bundler_plugin.test.ts,bundler_plugin_chain.test.ts,bundler_defer.test.tsandnative-plugin.test.tsall pass.Related
The same terminate() race also hits the pre-existing, plugin-independent
complete_on_bundle_threadUAF (posting the build result to the freed worker event loop). That reproduces on current main with or without this change and is covered by #35158 / #35767; this PR is scoped to the plugin-data read and the test is written accordingly.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts