Bun.plugin.clearAll(): clear namespaces and groups together - #32261
Conversation
|
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 (3)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughAdds centralized plugin registry clearing. ChangesPlugin clear state reset and validation
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 — minimal, obviously-correct fix for the parallel-vector desync in clearAll(), with regression tests for both failure modes.
Extended reasoning...
Overview
This PR fixes Bun.plugin.clearAll(), which previously cleared onLoadPlugins.groups without onLoadPlugins.namespaces and onResolvePlugins.namespaces without onResolvePlugins.groups. Since Base::namespaces and Base::groups are parallel-indexed (used together in Base::group() / Base::append()), this desync caused a WTF::Vector OOB assert (SIGABRT) on onLoad re-registration and stale-callback execution on onResolve re-registration. The fix adds a Base::clear() that resets fileNamespace, namespaces, and groups atomically, and calls it for both plugin sets. Net code change: +7 lines in the header, -4/+2 in the .cpp. Two subprocess regression tests cover both paths.
Security risks
None. This is internal bookkeeping for the runtime plugin registry — clearing three Vectors instead of two. No parsing, no auth, no untrusted input handling, no new surface area.
Level of scrutiny
Low. The root-cause analysis in the PR description matches the code exactly: Base::group() indexes groups[i] by the position found in namespaces, so clearing one without the other is plainly wrong. The fix is the obvious one-liner refactor (clear all three together) and is strictly more correct than what it replaces. Tests are well-isolated in subprocesses to avoid disturbing the rest of the file's plugin state.
Other factors
The one inline finding is an explicitly pre-existing, perf-only nit (mustDoExpensiveRelativeLookup not reset) that this PR neither introduces nor worsens — fine to address separately. No CODEOWNERS coverage for these paths, no outstanding reviewer comments, and the change is self-contained with no callers of the new Base::clear() other than jsFunctionBunPluginClear.
There was a problem hiding this comment.
LGTM — straightforward fix keeping the parallel namespaces/groups vectors in sync on clearAll(), and the follow-up reset of mustDoExpensiveRelativeLookup looks good.
Extended reasoning...
Overview
Adds BunPlugin::Base::clear() (clears fileNamespace, namespaces, groups together) and rewrites jsFunctionBunPluginClear to call it for both onLoadPlugins and onResolvePlugins, plus resets mustDoExpensiveRelativeLookup. Two subprocess regression tests added to plugins.test.ts.
Security risks
None. This is internal plugin-registry bookkeeping; no auth, crypto, parsing of untrusted input, or permission surface touched.
Level of scrutiny
Low. The bug and fix are mechanical: namespaces and groups are parallel-indexed (see Base::group() / Base::append()), and the previous clearAll() cleared them asymmetrically, causing an OOB index on re-register (onLoad) or stale callbacks (onResolve). The new code strictly clears a superset of what the old code cleared, so it cannot regress existing behavior — it only restores the invariant that the two vectors stay the same size.
Other factors
- The follow-up commit addressed my earlier nit (resetting
mustDoExpensiveRelativeLookup), and that thread is resolved. - Tests are run in subprocesses so they don't wipe the file-level plugins the rest of the suite depends on; the assertions check exit code 0 and exact stdout, which would catch both the SIGABRT and the stale-callback failure modes described in the PR.
- No CODEOWNERS entry covers these paths.
- Net C++ change is ~10 lines with no new control flow or allocation patterns.
There was a problem hiding this comment.
LGTM — the parallel-array desync fix is correct and the nit was addressed; CI failures are unrelated infra (musl LTO / asan flakes).
Extended reasoning...
Overview
This PR fixes Bun.plugin.clearAll() so that the parallel-indexed namespaces and groups vectors on BunPlugin::Base are cleared together. The old code cleared onLoadPlugins.groups without onLoadPlugins.namespaces (→ OOB index / SIGABRT on re-register) and onResolvePlugins.namespaces without onResolvePlugins.groups (→ stale callbacks). The fix adds a small Base::clear() helper and calls it for both plugin maps, plus resets mustDoExpensiveRelativeLookup (my prior nit, now addressed in dc82119). Two subprocess regression tests cover both failure modes.
Security risks
None. This is internal plugin-registry bookkeeping — clearing WTF::Vectors of Strong<> handles. No new inputs are parsed, no auth/crypto/permissions touched, and the change strictly makes clearAll() more thorough than before.
Level of scrutiny
Low-to-moderate. The C++ change is ~10 lines and mechanically obvious once you see that Base::group() indexes groups[i] by the position found in namespaces — clearing them together is the only correct option. clear() is non-virtual, but call sites use the concrete OnLoad/OnResolve members directly so no dispatch surprise. The test additions follow the existing bunExe()/bunEnv subprocess pattern in the same file and are appropriately isolated (so clearAll() doesn't nuke the file's other plugin fixtures).
Other factors
- My prior inline nit (reset
mustDoExpensiveRelativeLookup) was applied and the thread is resolved. - Bug hunter found no issues.
- The Buildkite failures reported by robobun are unrelated to this diff: three are musl LTO linker errors ("Linking two modules of different data layouts") and the rest are pre-existing asan flakes in
test/cli/install/GHSA-pfwx-36v6-832x.test.ts/package.json— none touch the plugin code or test file changed here.
a597b87 to
b7332d1
Compare
There was a problem hiding this comment.
LGTM — small, well-tested fix for the parallel-array desync; the inline UAF note is a contrived self-inflicted edge case and non-blocking.
Extended reasoning...
Overview
This PR fixes Bun.plugin.clearAll(), which previously cleared onLoadPlugins.groups but not .namespaces, and onResolvePlugins.namespaces but not .groups. Since namespaces and groups are parallel-indexed in BunPlugin::Base, this desync caused a SIGABRT (out-of-bounds WTF::Vector access) on re-registration of an onLoad namespace, and stale-callback execution for onResolve. The fix adds a 6-line Base::clear() that resets all three fields together and calls it for both plugin maps. A follow-up commit (dc82119) addressed my earlier nit by also resetting mustDoExpensiveRelativeLookup. Two new subprocess tests in plugins.test.ts exercise both failure paths.
Security risks
None. This is plugin-registry bookkeeping inside a single process; no auth/crypto/permissions/external-input parsing is touched. The UAF noted inline requires the user's own plugin code to call clearAll() from inside its own namespaced onResolve callback — self-inflicted, same-process, and a variant of a pre-existing groups.append() realloc hazard rather than a new attack surface.
Level of scrutiny
Low-to-moderate. The C++ change is ~10 net lines and the correctness argument is trivial: two parallel Vectors must be cleared together, and the new Base::clear() does exactly that plus the existing fileNamespace.clear(). The previous code was plainly wrong (asymmetric field clearing); the new code is the obvious fix. None of the touched files fall under CODEOWNERS.
Other factors
- My prior review nit (
mustDoExpensiveRelativeLookup) was addressed and resolved. - Tests are subprocess-isolated so they don't disturb the rest of
plugins.test.ts, and the PR description reports the full suite still passes (33 pass / 1 pre-existing todo). - The one new finding from this run is explicitly a non-blocking nit covering a contrived reentrancy case; it doesn't affect the normal register → clearAll → re-register flow this PR targets.
Given the small scope, clear root cause, targeted tests, and resolved prior feedback, this is safe to approve without further human review.
|
(superseded — see the verification comment below for current state on |
b7332d1 to
cf2499f
Compare
|
Rebased onto An independent fuzz report hit the same root cause from a different angle, and it's worth recording because the symptom is silent rather than a crash. The existing tests re-register the same namespace after Bun.plugin({ name: "old", setup: b => b.onResolve({ filter: /.*/, namespace: "aa" }, oldResolve) });
Bun.plugin.clearAll();
Bun.plugin({ name: "new", setup: b => b.onResolve({ filter: /.*/, namespace: "bb" }, newResolve) });
await import("bb:hello");
// oldResolve runs for the "bb" namespace; newResolve never runs.
Added:
All four The previous build's only non-flaky failure was |
There was a problem hiding this comment.
LGTM — both earlier review threads addressed; the parallel-vector invariant fix is minimal and well-tested.
Extended reasoning...
Overview
The PR fixes Bun.plugin.clearAll() (jsFunctionBunPluginClear), which previously cleared mismatched halves of the parallel-indexed namespaces/groups vectors on onLoadPlugins and onResolvePlugins, leaving them out of sync and causing a SIGABRT (WTF::Vector bounds assert) or stale-callback execution on re-registration. The fix adds BunPlugin::Base::clear() that resets fileNamespace, namespaces, and groups together, calls it for both plugin maps, and additionally resets mustDoExpensiveRelativeLookup alongside the virtualModules delete. Four subprocess tests are added to test/js/bun/plugin/plugins.test.ts.
Security risks
None. This is internal bookkeeping for runtime plugin registration; no untrusted input parsing, no auth/crypto/permissions surface. The change strictly restores an invariant (namespaces.size() == groups.size()) that the surrounding code already assumes.
Level of scrutiny
Low-to-medium. It touches C++ JSC bindings, but the actual change is a 7-line helper plus a 5-line call-site rewrite that is trivially verifiable against the data structure's own group()/append() methods — those index groups[i] by the position of namespaces[i], so clearing both together is the only correct behavior. No allocation, exception-scope, or GC-rooting changes.
Other factors
Both of my prior inline threads are resolved: the mustDoExpensiveRelativeLookup reset was applied in dc5bef6, and the reentrancy concern I raised about OnResolve::run() is moot on current main — that function already snapshots matched callbacks into a local MarkedArgumentBuffer before invoking any user JS, so groups.clear() during a callback cannot dangle the iteration. The new tests follow harness conventions (bunExe/bunEnv, tempDir, concurrent subprocess spawn with piped stdout/stderr/exit asserted together) and directly exercise both failure modes described in the PR body plus the cross-namespace mis-routing and virtual-module lookup-mode reset. No open reviewer comments remain and the bug-hunting pass found nothing.
Status: rebased onto latest main, ready to mergeNow at VerificationDebug + ASAN,
Re-confirmed against the new base that both preconditions still hold: the assertion is present, and CIPrevious runs went red only on infrastructure, never on this diff:
|
cf2499f to
319877f
Compare
|
Rebased onto The collision was confined to Re-verified on the rebased tree: the four |
There was a problem hiding this comment.
LGTM — both prior nits are addressed and the added tests cover the cross-namespace and lookup-mode-reset cases.
Extended reasoning...
Overview
Fixes Bun.plugin.clearAll() so the parallel-indexed namespaces and groups vectors on BunPlugin::Base are cleared together via a new Base::clear(), and resets OnLoad::mustDoExpensiveRelativeLookup alongside the virtualModules delete. Native change is ~10 lines across BunPlugin.{h,cpp}; the rest is four subprocess tests in test/js/bun/plugin/plugins.test.ts.
Security risks
None. Bun.plugin is user-controlled runtime configuration, not a trust boundary; the change only affects how registered callbacks are torn down. No parsing of untrusted input, no auth/crypto/permissions.
Level of scrutiny
Low-to-medium. The bug and fix are mechanical: the old code cleared two fields from each of onLoadPlugins/onResolvePlugins asymmetrically, leaving namespaces and groups desynchronized (SIGABRT on one side, stale callbacks on the other). The fix clears all three fields on both sides through a single helper, which is the obviously-correct shape. No CODEOWNERS on this path.
Other factors
I reviewed this twice previously. The mustDoExpensiveRelativeLookup nit was addressed in dc82119, and the reentrant-clearAll() iterator-invalidation concern I flagged is moot after the rebase — OnResolve::run() on current main (via #33072) already snapshots matching callbacks into a MarkedArgumentBuffer before invoking user JS, so groups.clear() mid-callback no longer dangles. Since then the only additions are two more tests (cross-namespace mis-routing and the debug-assertion for the stale lookup flag), both verified to fail on the PR base and pass with the fix. CI failures on the last build were an artifact-download timeout and known Windows flakes, none touching the plugin registry.
Unblocked: rebased onto a green baseThe earlier CI red here was a This PR on its own merits
Zero unresolved review threads. If the new build shows red, the one lane to double-check first is |
|
Verified this change also fixes a crash reproducible on current main and on 1.4.0: Bun.plugin({ name: "before", setup(b) {
b.onLoad({ filter: /.*/, namespace: "cr" }, () => ({ contents: "export default 'before'", loader: "js" }));
}});
Bun.plugin.clearAll();
Bun.plugin({ name: "after", setup(b) {
b.onResolve({ filter: /.*/, namespace: "cr" }, (a) => ({ path: a.path, namespace: "cr" }));
}});
await import("cr:x"); // exit 134 without this fix, clean rejection with itOn this branch the repro exits 0 with a clean rejection, and the four One more data point for merging: the cookie-map failures that made build 68774 red were fixed on main by #33425 (48ff9eb), which landed a few hours after this branch's last push. A re-run against current main should come back green. |
jsFunctionBunPluginClear cleared onLoadPlugins.groups but not onLoadPlugins.namespaces, and onResolvePlugins.namespaces but not onResolvePlugins.groups. These vectors are parallel-indexed, so after clearAll() re-registering the same namespace would return &groups[i] past the end of an empty vector (onLoad, SIGABRT on the WTF::Vector bounds assert) or hit a stale group entry holding the old callback (onResolve). Add Base::clear() that resets fileNamespace, namespaces, and groups together, and call it for both onLoad and onResolve.
The flag is only ever written true by mock.module() for relative specifiers and gates an extra path-join in resolveVirtualModule(). After clearAll() deletes virtualModules it should go back to false so later non-relative virtual modules skip the unnecessary branch.
… mode reset The existing tests re-register the same namespace after clearAll(). Add the case where a different namespace is registered, which takes the cleared namespace's index and so inherits its callbacks, and a test for the mustDoExpensiveRelativeLookup reset.
319877f to
7816f86
Compare
There was a problem hiding this comment.
LGTM — the inline nit on the self-referential stderr field is worth tightening but non-blocking; the load-bearing stdout/exitCode assertions catch both the SIGABRT and stale-callback regressions.
Extended reasoning...
Overview
Adds Base::clear() in BunPlugin.h that resets fileNamespace, namespaces, and groups together, and rewires jsFunctionBunPluginClear to call it for both onLoadPlugins and onResolvePlugins instead of clearing a mismatched subset of each. Also resets mustDoExpensiveRelativeLookup alongside the virtualModules delete (my earlier nit, now addressed). Four subprocess tests in plugins.test.ts cover the onLoad OOB abort, the onResolve stale-callback path, cross-namespace index reuse, and the virtual-module lookup-mode assertion.
Security risks
None. clearAll() is a same-process teardown of user-registered plugin callbacks; no untrusted-input parsing, no auth/crypto, no new external surface.
Level of scrutiny
Medium — native C++ in JSC bindings, but the change is a 7-line inline method plus a 5-line call-site rewrite whose correctness follows directly from Base::group() indexing groups[i] by namespaces[i]. The parallel-vector invariant is visible ~15 lines above the new clear(). No allocation, no exception paths, no GC interaction.
Other factors
- Both of my prior inline comments are resolved: the
mustDoExpensiveRelativeLookupreset landed in this diff, and theOnResolve::runreentrancy hazard was deferred to a follow-up (and the snapshot-before-iterate fix has since landed on main, sogroups.clear()during a callback no longer dangles the iteration). - Tests were verified to fail against merge-base
src/(exit 134 / "stale onResolve callback ran" /ASSERTION FAILED: !mustDoExpensiveRelativeLookup) and pass with the fix, per the PR thread. - The one open nit (self-referential
stderrin the expected object) weakens only the stderr check;stdoutis asserted exactly andexitCode: 0catches the abort, so the tests still fail for the right reason without the fix. - CI red on the last build was an unrelated
cookie-map.test.tsbreakage on the base commit, since fixed on main.
The expected objects passed the captured stderr back as its own expected value, so that field could never fail. bunEnv silences debug logging and all four subprocesses are clean, so assert the empty string.
There was a problem hiding this comment.
LGTM — the stderr assertion nit from the last pass is addressed in ce16ca6, and all prior feedback is resolved.
Extended reasoning...
Overview
This PR fixes a parallel-array desync bug in Bun.plugin.clearAll(). The old jsFunctionBunPluginClear cleared onLoadPlugins.groups but not .namespaces, and onResolvePlugins.namespaces but not .groups, breaking the invariant that Base::group() relies on (namespaces[i] ↔ groups[i]). The fix adds Base::clear() that resets all three fields together, calls it for both plugin maps, and resets mustDoExpensiveRelativeLookup alongside the virtualModules delete. Net native change is ~7 lines; the rest is four subprocess tests.
Security risks
None. This is a state-reset fix in the runtime plugin registry; no untrusted input parsing, auth, or crypto is touched.
Level of scrutiny
Medium — native C++ in the JSC bindings, but the change is mechanical (clear three parallel vectors together instead of two of four). The root cause is well-explained and the fix is the obvious one. I raised three concerns across earlier passes and all were addressed: the mustDoExpensiveRelativeLookup reset (37d7090), the reentrant clearAll()-from-callback UAF (moot now that OnResolve::run() snapshots callbacks into a MarkedArgumentBuffer before invoking user JS), and the self-referential stderr in the test assertions (ce16ca6).
Other factors
The four new tests follow harness conventions (subprocess isolation via bunEnv/bunExe, concurrent pipe draining, combined-object assertions with stderr: ""), and the author has repeatedly verified fail-before/pass-after against merge-base src/. All review threads are resolved and no bugs were found this run.
2 participants
Repro
Cause
jsFunctionBunPluginClearclearedonLoadPlugins.groupsbut notonLoadPlugins.namespaces, andonResolvePlugins.namespacesbut notonResolvePlugins.groups.Base::namespacesandBase::groupsare parallel-indexed:Base::group()looks upnamespaces[i]and returns&groups[i].After
clearAll():namespaces.size() == N,groups.size() == 0. Re-registering the same namespace hits thegroup()lookup branch inBase::append, which returns&groups[i]past the end of an empty vector and trips theWTF::Vectorbounds assert (SIGABRT, exit 134).namespaces.size() == 0,groups.size() == N. Re-registering appends a fresh entry to both, butgroups[0]is still the stale pre-clearAll()group, so the next resolve for that namespace runs the old callback.Fix
Add
Base::clear()that resetsfileNamespace,namespaces, andgroupstogether, and call it for bothonLoadPluginsandonResolvePlugins.Verification
Two new subprocess tests in
test/js/bun/plugin/plugins.test.tscover both paths. Without the fix the onLoad test aborts with exit 134 and the onResolve test fails withstale onResolve callback ran; with the fix both pass. Fullplugins.test.tssuite: 33 pass, 1 pre-existing todo.[review] gate passed · iteration 10 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 10
evidence per changed file
root cause · written by the author bot
Bun.plugin.clearAll()cleared only one of the two parallel-indexed vectors in each plugin registry, emptyingonLoadPlugins.groupswhile leavingnamespacespopulated and doing the reverse foronResolvePlugins, so a later registration in a previously used namespace madeBase::group()index past the end ofgroupsand appendStronghandles into unowned memory. The fix adds a singleBunPlugin::Base::clear()that resets namespaces, groups, and the remaining per-registry state together and calls it for both the onLoad and onResolve registries, with the host function also removing v…