Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 102 additions & 92 deletions src/jsc/bindings/JSBundlerPlugin.cpp

Large diffs are not rendered by default.

53 changes: 37 additions & 16 deletions src/jsc/bindings/JSBundlerPlugin.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <JavaScriptCore/RegularExpression.h>
#include "napi_external.h"
#include <JavaScriptCore/Yarr.h>
#include <wtf/ThreadSafeRefCounted.h>
#include "WriteBarrierList.h"

typedef void (*JSBundlerPluginAddErrorCallback)(void*, void*, JSC::EncodedJSValue, JSC::EncodedJSValue);
Expand All @@ -18,7 +19,13 @@ namespace Bun {

using namespace JSC;

class BundlerPlugin final {
class JSBundlerPlugin;

// Heap-allocated and ref-counted separately from the JSBundlerPlugin GC cell so the
// bundle thread can read filter lists after a Worker's VM (and its JSC heap) is torn
// down. The owning JSBundlerPlugin cell holds one ref; the Rust-side Plugin handle
// holds another.
Comment thread
robobun marked this conversation as resolved.
Outdated
class BundlerPlugin final : public ThreadSafeRefCounted<BundlerPlugin> {
public:
/// In native plugins, the regular expression could be called concurrently on multiple threads.
/// Therefore, we need a mutex to synchronize access.
Expand All @@ -41,7 +48,7 @@ class BundlerPlugin final {
{
}

bool match(JSC::VM& vm, const String& path);
bool match(const String& path);
};

class NamespaceList {
Expand Down Expand Up @@ -74,7 +81,9 @@ class BundlerPlugin final {

struct NativePluginCallback {
JSBundlerPluginNativeOnBeforeParseCallback callback;
Bun::NapiExternal* external;
// NapiExternal::value() captured at append time so the parse thread never
// reads the GC cell; onBeforeParseExternals keeps the cell alive for GC.
Comment thread
robobun marked this conversation as resolved.
void* externalValue;
Comment thread
robobun marked this conversation as resolved.
/// This refers to the string exported in the native plugin under
/// the symbol BUN_PLUGIN_NAME
///
Expand All @@ -95,8 +104,8 @@ class BundlerPlugin final {
PerNamespaceCallbackList fileCallbacks = {};
Vector<PerNamespaceCallbackList> namespaceCallbacks = {};

int call(JSC::VM& vm, BundlerPlugin* plugin, int* shouldContinue, void* bunContextPtr, const BunString* namespaceStr, const BunString* pathString, OnBeforeParseArguments* onBeforeParseArgs, OnBeforeParseResult* onBeforeParseResult);
void append(JSC::VM& vm, JSC::RegExp* filter, String& namespaceString, JSBundlerPluginNativeOnBeforeParseCallback callback, const char* name, NapiExternal* external);
int call(BundlerPlugin* plugin, int* shouldContinue, void* bunContextPtr, const BunString* namespaceStr, const BunString* pathString, OnBeforeParseArguments* onBeforeParseArgs, OnBeforeParseResult* onBeforeParseResult);
void append(JSC::VM& vm, JSC::RegExp* filter, String& namespaceString, JSBundlerPluginNativeOnBeforeParseCallback callback, const char* name, void* externalValue);

Vector<FilterRegExp>* group(const String& namespaceStr, unsigned& index)
{
Expand All @@ -118,33 +127,45 @@ class BundlerPlugin final {
};

public:
bool anyMatchesCrossThread(JSC::VM&, BunString* namespaceStr, BunString* path, bool isOnLoad);
void tombstone() { tombstoned = true; }

BundlerPlugin(void* config, BunPluginTarget target, JSBundlerPluginAddErrorCallback addError, JSBundlerPluginOnLoadAsyncCallback onLoadAsync, JSBundlerPluginOnResolveAsyncCallback onResolveAsync)
: addError(addError)
, onLoadAsync(onLoadAsync)
, onResolveAsync(onResolveAsync)
static Ref<BundlerPlugin> create(void* config, BunPluginTarget target, JSBundlerPluginAddErrorCallback addError, JSBundlerPluginOnLoadAsyncCallback onLoadAsync, JSBundlerPluginOnResolveAsyncCallback onResolveAsync)
{
this->target = target;
this->config = config;
return adoptRef(*new BundlerPlugin(config, target, addError, onLoadAsync, onResolveAsync));
}

bool anyMatchesCrossThread(BunString* namespaceStr, BunString* path, bool isOnLoad);
void tombstone() { tombstoned = true; }

JSBundlerPlugin* cell() const { return m_cell; }
void setCell(JSBundlerPlugin* cell) { m_cell = cell; }
Comment thread
coderabbitai[bot] marked this conversation as resolved.

NamespaceList onLoad = {};
NamespaceList onResolve = {};
NativePluginList onBeforeParse = {};
BunPluginTarget target { BunPluginTargetBrowser };

WriteBarrierList<JSC::JSPromise> deferredPromises = {};
// The raw `NapiExternal*` stored in `NativePluginCallback` is dereferenced
// off the JS thread; this list keeps those cells alive for GC.
// Keeps each registered NapiExternal alive for GC so its finalizer does not
// release the user context while the build is still using it.
Comment thread
robobun marked this conversation as resolved.
Outdated
WriteBarrierList<NapiExternal> onBeforeParseExternals = {};

JSBundlerPluginAddErrorCallback addError;
JSBundlerPluginOnLoadAsyncCallback onLoadAsync;
JSBundlerPluginOnResolveAsyncCallback onResolveAsync;
void* config { nullptr };
bool tombstoned { false };

private:
BundlerPlugin(void* config, BunPluginTarget target, JSBundlerPluginAddErrorCallback addError, JSBundlerPluginOnLoadAsyncCallback onLoadAsync, JSBundlerPluginOnResolveAsyncCallback onResolveAsync)
: addError(addError)
, onLoadAsync(onLoadAsync)
, onResolveAsync(onResolveAsync)
{
this->target = target;
this->config = config;
}

// Back-pointer into the owning VM's GC heap. Only dereferenced on the JS thread.
JSBundlerPlugin* m_cell { nullptr };
};

} // namespace Zig
9 changes: 3 additions & 6 deletions src/runtime/api/JSBundler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1635,7 +1635,7 @@ pub mod js_bundler {
global: &JSGlobalObject,
target: jsc::BunPluginTarget,
) -> *mut Plugin;
safe fn JSBundlerPlugin__tombstone(plugin: &Plugin);
safe fn JSBundlerPlugin__destroy(plugin: &Plugin);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
safe fn JSBundlerPlugin__runOnEndCallbacks(
plugin: &mut Plugin,
build_promise: JSValue,
Expand Down Expand Up @@ -1701,9 +1701,7 @@ pub mod js_bundler {
impl PluginJscExt for Plugin {
fn create(global: &JSGlobalObject, target: jsc::BunPluginTarget) -> *mut Plugin {
jsc::mark_binding();
let plugin = JSBundlerPlugin__create(global, target);
JSValue::from_cell(plugin).protect();
plugin
JSBundlerPlugin__create(global, target)
}
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

fn run_on_end_callbacks(
Expand Down Expand Up @@ -1739,8 +1737,7 @@ pub mod js_bundler {

fn destroy(this: *mut Plugin) {
jsc::mark_binding();
JSBundlerPlugin__tombstone(Plugin::opaque_ref(this));
JSValue::from_cell(this).unprotect();
JSBundlerPlugin__destroy(Plugin::opaque_ref(this));
}
Comment thread
robobun marked this conversation as resolved.

fn global_object(&self) -> &JSGlobalObject {
Expand Down
10 changes: 5 additions & 5 deletions src/runtime/api/js_bundle_completion_task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,11 @@ impl JSBundleCompletionTask {
///
/// Centralises the `Option<NonNull> → Option<&mut T>` deref so callers
/// (`to_js_error` / `on_complete_anytask`) stay safe. The plugin is a C++
/// `JSBundlerPlugin` opaque created by [`PluginJscExt::create`] and
/// `protect()`-ed for the task's lifetime; it is freed only via
/// `Plugin::destroy` in `deinit` *after* `take()` clears `self.plugins`.
/// While the field is `Some` the pointee is therefore live, pinned, and
/// disjoint from `*self` (separate C++-heap allocation).
/// `BundlerPlugin` heap allocation created by [`PluginJscExt::create`]
/// (which also `gcProtect`s the owning `JSBundlerPlugin` GC cell and takes
/// a +1 ref for this task); it is released only via `Plugin::destroy` in
/// `deinit` *after* `take()` clears `self.plugins`. While the field is
/// `Some` the pointee is therefore live, pinned, and disjoint from `*self`.
Comment thread
robobun marked this conversation as resolved.
#[inline]
fn plugins_mut(&mut self) -> Option<&mut Plugin> {
// SAFETY: see fn doc — C++-heap opaque, live while `self.plugins` is
Expand Down
8 changes: 3 additions & 5 deletions src/runtime/bake/bake_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,7 @@ impl Drop for UserOptions {
if let Some(p) = self.bundler_options.plugin {
// `p` is the FFI handle returned by `Plugin::create` in
// `parse_plugin_array`; `PluginJscExt::destroy` is its paired
// (safe) destructor — it null-checks via `opaque_ref` and
// unprotect()s the JSCell / tombstones the C++ object.
// destructor (tombstones + gcUnprotects the cell + drops the ref).
Plugin::destroy(p.as_ptr());
}
}
Expand Down Expand Up @@ -321,8 +320,7 @@ impl SplitBundlerOptions {
Some(p) => p,
None => {
let p = Plugin::create(global, bun_jsc::BunPluginTarget::Bun);
let p = NonNull::new(p)
.expect("JSBundlerPlugin__create returns a non-null protected JSCell");
let p = NonNull::new(p).expect("JSBundlerPlugin__create returns non-null");
self.plugin = Some(p);
p
}
Expand Down Expand Up @@ -360,7 +358,7 @@ impl SplitBundlerOptions {
};

// `Plugin` is an `opaque_ffi!` ZST — `opaque_mut` is the safe
// deref. Handle held live in `self.plugin` (protected JSCell).
// deref. Handle held live in `self.plugin` (ref-counted C++ heap).
let plugin_result = Plugin::opaque_mut(plugin.as_ptr()).add_plugin(
function,
empty_object,
Expand Down
91 changes: 91 additions & 0 deletions test/bundler/bun-build-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1525,3 +1525,94 @@ test("Bun.build can be called thousands of times in one process without crashing
expect(stdout.trim()).toBe("OK 400");
expect(exitCode).toBe(0);
}, 180_000);

// The bundle thread calls JSBundlerPlugin__anyMatches / hasOnBeforeParsePlugins against
// the plugin's filter vectors while a build is in flight. Those vectors used to live
// inline in the JSBundlerPlugin GC cell, so worker.terminate() (which tears down the
// worker's JSC heap) freed them out from under the bundle thread:
//
// JSBundlerPlugin.cpp:96: runtime error: reference binding to null pointer of type
// 'Bun::BundlerPlugin::FilterRegExp'
//
// The filter data is now a ThreadSafeRefCounted heap allocation that outlives the cell,
// so the cross-thread read stays valid after the worker's VM is gone.
//
// This test cannot assert "exit 0": the same terminate() race also trips the
// pre-existing, plugin-independent complete_on_bundle_thread use-after-free (posting the
// build result to the dead worker's event loop), which reproduces on current main with
// or without this change. So we run the race repeatedly and assert only that no attempt
// faults inside JSBundlerPlugin.cpp. Malloc=1 routes JSC allocations through system
// malloc so ASAN can see writes to the freed cell.
test.skipIf(!isASAN)(
"terminating a Worker mid-Bun.build() does not read freed plugin filter data on the bundle thread",
async () => {
const MODULES = 300;
const files: Record<string, string> = {};
let entry = "";
for (let i = 0; i < MODULES; i++) {
files[`m${i}.js`] = `export const v${i} = ${i};\n`;
entry += `import { v${i} } from "./m${i}.js";\n`;
}
entry += `export default 0;\n`;
files["entry.js"] = entry;
files["w.cjs"] = `
const { parentPort, workerData: d } = require("node:worker_threads");
async function lane(k) {
for (;;) {
await Bun.build({
entrypoints: [d.dir + "/entry.js"],
plugins: [{ name: "p" + k, setup(b) { b.onLoad({ filter: /never-matches-anything/ }, () => undefined); } }],
}).catch(() => {});
}
}
parentPort.postMessage("up");
for (let k = 0; k < 2; k++) lane(k);
`;
files["parent.mjs"] = `
import { Worker } from "node:worker_threads";
const dir = process.argv[2];
for (let r = 0; r < 40; r++) {
const ws = [];
for (let i = 0; i < 2; i++) {
const w = new Worker(dir + "/w.cjs", { workerData: { dir } });
w.on("error", () => {});
ws.push(w);
}
await Promise.all(ws.map(w => new Promise(res => { w.once("message", res); setTimeout(res, 3000); })));
await Bun.sleep(5 + (r * 7) % 30);
await Promise.all(ws.map(w => w.terminate()));
}
console.log("ok");
`;

using dir = tempDir("bun-build-worker-plugin-terminate", files);

// The subprocess is expected to crash (the unrelated complete_on_bundle_thread UAF
// still exists on main), and on a given crash it may land in either site. Run enough
// attempts that, without the fix, at least one lands in JSBundlerPlugin.cpp.
const ATTEMPTS = 8;
const frames: string[] = [];
for (let attempt = 0; attempt < ATTEMPTS; attempt++) {
await using proc = Bun.spawn({
cmd: [bunExe(), join(String(dir), "parent.mjs"), String(dir)],
env: { ...bunEnv, Malloc: "1" },
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text()]);
await proc.exited;

const pluginFrames = stderr.split("\n").filter(l => /JSBundlerPlugin\.cpp|BundlerPlugin::|FilterRegExp/.test(l));
if (pluginFrames.length > 0) {
frames.push(`attempt ${attempt}:\n${pluginFrames.join("\n")}`);
}
if (stdout.includes("ok") && stderr === "") {
// Reached the end with no sanitizer report at all; nothing left to probe.
break;
}
}

expect(frames).toEqual([]);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
120_000,
);
Loading