Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
40 changes: 31 additions & 9 deletions src/jsc/CppTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ unsafe extern "C" {
safe fn Bun__EventLoopTaskNoContext__createdInBunVm(
task: &EventLoopTaskNoContext,
) -> *mut VirtualMachine;
safe fn Bun__EventLoopTaskNoContext__contextIdentifier(task: &EventLoopTaskNoContext) -> u32;
// safe: by-value `u32` in; resolves the context under the C++ contexts-map
// lock (same fence as `postTaskTo`/`markTerminating`) and no-ops if the
// context is gone or terminating.
Comment thread
robobun marked this conversation as resolved.
Outdated
safe fn ScriptExecutionContext__unrefEventLoopConcurrently(id: u32);
}

bun_opaque::opaque_ffi! {
Expand Down Expand Up @@ -47,14 +52,22 @@ impl EventLoopTaskNoContext {
unsafe { Bun__EventLoopTaskNoContext__performTask(this) }
}

/// Get the VM that created this task. `VirtualMachine` is process-lifetime
/// (PORTING.md §Global mutable state), so a [`BackRef`] is the right
/// non-owning handle: callers project `&VirtualMachine` via `Deref` and
/// route mutation through the VM's safe interior accessors (e.g.
/// `event_loop_shared()`).
/// The VM that created this task. Only safe to dereference on that VM's JS
/// thread (worker VMs are freed by `terminate()`); the pool-thread
/// completion uses [`Self::context_identifier`] instead.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn get_vm(&self) -> Option<bun_ptr::BackRef<VirtualMachine>> {
NonNull::new(Bun__EventLoopTaskNoContext__createdInBunVm(self)).map(bun_ptr::BackRef::from)
}

/// The creating VM's [`ScriptExecutionContextIdentifier`], captured at
/// construction so the pool-thread completion can unref the event loop
/// under the contexts-map lock instead of dereferencing the (possibly
/// freed) VM pointer.
///
/// [`ScriptExecutionContextIdentifier`]: crate::JSGlobalObject::ScriptExecutionContextIdentifier
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn context_identifier(&self) -> u32 {
Bun__EventLoopTaskNoContext__contextIdentifier(self)
}
}

/// A task created from C++ code that runs inside the workpool, usually via ScriptExecutionContext.
Expand All @@ -73,14 +86,17 @@ impl ConcurrentCppTask {
let cpp_task = self.cpp_task;
// `EventLoopTaskNoContext` is an `opaque_ffi!` ZST handle; `opaque_ref`
// is the centralised non-null deref proof. Valid until `run` consumes it.
let maybe_vm = EventLoopTaskNoContext::opaque_ref(cpp_task).get_vm();
let context_id = EventLoopTaskNoContext::opaque_ref(cpp_task).context_identifier();
drop(self);
// SAFETY: `cpp_task` is the valid C++ handle stored by `ConcurrentCppTask__createAndRun`;
// `opaque_ref` above proved it non-null and it has not yet been freed — `run` consumes it here.
unsafe { EventLoopTaskNoContext::run(cpp_task) };
if let Some(vm) = maybe_vm {
vm.event_loop_shared().unref_concurrently();
}
// Runs on a work-pool thread: the creating VM may be a worker freed by
// terminate() while `run` (the crypto op) ran. The identifier-keyed
// unref takes the contexts-map lock (serializing with the shutdown
// path's `markTerminating()`, which is called before the VM box is
// freed) and no-ops if the context is gone or terminating.
Comment thread
robobun marked this conversation as resolved.
Outdated
ScriptExecutionContext__unrefEventLoopConcurrently(context_id);
}
}

Expand All @@ -89,6 +105,12 @@ pub(crate) extern "C" fn ConcurrentCppTask__createAndRun(cpp_task: *mut EventLoo
crate::mark_binding!();
// `EventLoopTaskNoContext` is an `opaque_ffi!` ZST handle; `opaque_ref` is
// the centralised non-null deref proof. C++ just handed it over.
//
// Called on the creating VM's JS thread (only caller is
// `PhonyWorkQueue::dispatch` from the SubtleCrypto IDL entry points), so
// dereferencing the captured VM here is safe. The matching unref happens
// on a work-pool thread in `run_owned` and goes through the context
// identifier instead.
Comment thread
robobun marked this conversation as resolved.
Outdated
if let Some(vm) = EventLoopTaskNoContext::opaque_ref(cpp_task).get_vm() {
vm.event_loop_shared().ref_concurrently();
}
Expand Down
5 changes: 5 additions & 0 deletions src/jsc/bindings/EventLoopTaskNoContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,9 @@ extern "C" void* Bun__EventLoopTaskNoContext__createdInBunVm(const EventLoopTask
return task->createdInBunVm();
}

extern "C" WebCore::ScriptExecutionContextIdentifier Bun__EventLoopTaskNoContext__contextIdentifier(const EventLoopTaskNoContext* task)
{
return task->contextIdentifier();
}

} // namespace Bun
8 changes: 8 additions & 0 deletions src/jsc/bindings/EventLoopTaskNoContext.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "ZigGlobalObject.h"
#include "ScriptExecutionContext.h"
#include "root.h"

namespace Bun {
Expand All @@ -12,6 +13,7 @@ class EventLoopTaskNoContext {
public:
EventLoopTaskNoContext(JSC::JSGlobalObject* globalObject, Function<void()>&& task)
: m_createdInBunVm(defaultGlobalObject(globalObject)->bunVM())
, m_contextIdentifier(defaultGlobalObject(globalObject)->scriptExecutionContext()->identifier())
, m_task(WTF::move(task))
{
}
Expand All @@ -23,13 +25,19 @@ class EventLoopTaskNoContext {
}

void* createdInBunVm() const { return m_createdInBunVm; }
WebCore::ScriptExecutionContextIdentifier contextIdentifier() const { return m_contextIdentifier; }

private:
void* m_createdInBunVm;
// Captured for the pool-thread completion: the creating VM may be a worker
// freed by terminate() while the task ran, so the unref goes through the
// contexts-map lock instead of dereferencing m_createdInBunVm.
Comment thread
robobun marked this conversation as resolved.
Outdated
WebCore::ScriptExecutionContextIdentifier m_contextIdentifier;
Function<void()> m_task;
};

extern "C" void Bun__EventLoopTaskNoContext__performTask(EventLoopTaskNoContext* task);
extern "C" void* Bun__EventLoopTaskNoContext__createdInBunVm(const EventLoopTaskNoContext* task);
extern "C" WebCore::ScriptExecutionContextIdentifier Bun__EventLoopTaskNoContext__contextIdentifier(const EventLoopTaskNoContext* task);

} // namespace Bun
17 changes: 17 additions & 0 deletions src/jsc/bindings/ScriptExecutionContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -308,4 +308,21 @@ extern "C" void ScriptExecutionContext__markTerminating(JSC::JSGlobalObject* glo
context->markTerminating();
}

// Release one concurrent event-loop ref on the context's VM, if that context
// is still live and not terminating. Called from a work-pool thread after a
// ConcurrentCppTask's body has run; the creating VM may be a worker that
// worker.terminate() freed while the task was running. Taking the map lock
// serializes with markTerminating() (called from WebWorker::shutdown before
// the VM box is freed), so either we observe the live context and unref under
// the lock, or we observe it gone/terminating and drop the unref (the counter
// of a freed event loop needs no balancing). Same fence as postTaskTo().
Comment thread
robobun marked this conversation as resolved.
Outdated
extern "C" void ScriptExecutionContext__unrefEventLoopConcurrently(ScriptExecutionContextIdentifier id)
{
Locker locker { allScriptExecutionContextsMapLock };
auto* context = allScriptExecutionContextsMap().get(id);
if (!context || context->isTerminating())
Comment thread
robobun marked this conversation as resolved.
return;
Bun__eventLoop__incrementRefConcurrently(WebCore::clientData(context->vm())->bunVM, -1);
}

} // namespace WebCore
49 changes: 49 additions & 0 deletions test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,55 @@ test(
timeout,
);

// Regression: ConcurrentCppTask::run_owned (the work-pool wrapper for async
// WebCrypto ops) dereferenced the raw bunVM pointer captured by the C++
// EventLoopTaskNoContext to call unref_concurrently() after the crypto body
// ran. When the creating VM was a worker freed by terminate() while the
// crypto op was still running on the pool, that read the freed VM
// allocation. The body itself already posts back via postTaskTo(contextId)
// and so was safe; only the trailing unref was unfenced.
test.skipIf(!isASAN)(
"terminate() while crypto.subtle async ops are in flight does not UAF in ConcurrentCppTask",
async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const { Worker } = require("node:worker_threads");
const src = \`
const { parentPort } = require("node:worker_threads");
const s = crypto.subtle;
// Four lanes of PBKDF2 deriveBits on the work pool. The 200k-iteration
// SHA-512 body is long enough that terminate() reliably lands while
// at least one ConcurrentCppTask is still in run_owned.
for (let i = 0; i < 4; i++) (async () => {
const k = await s.importKey("raw", new TextEncoder().encode("pw-material-key"), "PBKDF2", false, ["deriveBits"]);
for (;;) try { await s.deriveBits({ name: "PBKDF2", salt: new Uint8Array(16), iterations: 200000, hash: "SHA-512" }, k, 512); } catch {}
})();
parentPort.postMessage("up");
\`;
for (let r = 0; r < ${rounds}; r++) {
const w = new Worker(src, { eval: true });
await new Promise(res => w.once("message", res));
// Let the pool lanes fill before terminating.
await Bun.sleep(60 + ((r * 41) % 220));
await w.terminate();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
console.log("ok");
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 });
},
timeout,
);

// Regression: the per-VM c-ares channel was destroyed in deinit_runtime_state
// (RuntimeState drop) AFTER JSC teardown and RareData.file_polls drop.
// ares_destroy() synchronously fires EDESTRUCTION query callbacks and socket-
Expand Down
Loading