Skip to content
Closed

ai slop #29363

Show file tree
Hide file tree
Changes from all 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
23 changes: 22 additions & 1 deletion src/bun.js/bindings/webcore/Worker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -130,18 +130,29 @@
extern "C" void WebWorker__setRef(
void* worker,
bool ref);

void Worker::setKeepAlive(bool keepAlive)
{
Locker locker { m_implLock };
if (!impl_)
return;
WebWorker__setRef(impl_, keepAlive);
}

void Worker::clearZigImpl()
{
Locker locker { m_implLock };
impl_ = nullptr;
}

bool Worker::updatePtr()
{
Locker locker { m_implLock };
if (!WebWorker__updatePtr(impl_, this)) {
impl_ = nullptr;
m_onlineClosingFlags = ClosingFlag;
m_terminationFlags.fetch_or(TerminatedFlag);
return false;

Check notice on line 155 in src/bun.js/bindings/webcore/Worker.cpp

View check run for this annotation

Claude / Claude Code Review

ref count leak when Worker thread spawn fails

Pre-existing ref count leak when Worker thread spawn fails. In Worker::create(), worker->ref() is called unconditionally before checking if impl is null; the matching worker->deref() only fires inside WebWorker__dispatchExit, which is never reached on the two early-failure paths (null impl from WebWorker__create, or thread spawn failure via WebWorker__updatePtr). The Worker object is permanently leaked in both cases. This PR correctly adds impl_=nullptr in the updatePtr() failure path (fixing th
Comment on lines 133 to 155

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing ref count leak when Worker thread spawn fails. In Worker::create(), worker->ref() is called unconditionally before checking if impl is null; the matching worker->deref() only fires inside WebWorker__dispatchExit, which is never reached on the two early-failure paths (null impl from WebWorker__create, or thread spawn failure via WebWorker__updatePtr). The Worker object is permanently leaked in both cases. This PR correctly adds impl_=nullptr in the updatePtr() failure path (fixing the use-after-free), but the companion ref count imbalance is left unaddressed.

Extended reasoning...

What the bug is and how it manifests

In Worker::create() (Worker.cpp), worker->ref() is called unconditionally at line 222 with the comment "now referenced by Zig". This extra reference is the Zig-side ownership token that is balanced only by worker->deref() inside WebWorker__dispatchExit (Worker.cpp:493). There are two code paths where WebWorker__dispatchExit is never called, leaving the refcount permanently inflated by 1 and the Worker object unreachable for destruction.

The specific code paths that trigger it

Path A — WebWorker__create returns null: worker->ref() is called unconditionally, raising refcount to 2 (adoptRef starts it at 1). If WebWorker__create() returns null (e.g., a preload module URL fails to resolve), the code executes return Exception { TypeError, ... }. The local Ref destructs, decrementing refcount to 1. No code ever calls deref() again — the Worker leaks.

Path B — WebWorker__updatePtr fails (thread spawn fails): In web_worker.zig, when std.Thread.spawn() fails, the Zig side calls worker.deinit() (line 76) and returns false. The deinit() function only frees Zig-side memory; it does NOT call WebWorker__dispatchExit. Without dispatchExit, worker->deref() is never called. The JS wrapper is eventually GC'd (one deref: refcount 2 to 1), but it never reaches 0.

Why existing code does not prevent it

The ref/deref pattern relies entirely on WebWorker__dispatchExit being the single point of cleanup. There is no guard or RAII wrapper around the worker->ref() call in create() that would ensure a matching deref() on early-exit paths. The two failure paths diverge before a Zig thread is ever started, so the normal exit/deinit lifecycle never runs.

Impact

Every new Worker() call that fails at thread-spawn time permanently leaks a Worker C++ object (and all memory it holds: options, URL strings, pending task queue, etc.). In environments where worker creation can transiently fail (resource limits, preload module errors), this accumulates unboundedly.

How to fix it

For Path A: call worker->deref() before returning the Exception, or move worker->ref() to after the null check so it is only called when a thread is actually starting. For Path B: add worker->deref() in the updatePtr() failure branch in Worker::create(), mirroring what WebWorker__dispatchExit would have done.

Step-by-step proof for Path A

  1. new Worker() is called with a URL whose preload module fails to resolve.
  2. adoptRef(*new Worker(...)) — refcount = 1.
  3. WebWorker__create(...) returns null.
  4. worker->ref() is called — refcount = 2.
  5. if (!impl) is true, return Exception { TypeError, ... }.
  6. The local Ref destructor runs — refcount = 1.
  7. No further owner; Worker::~Worker() is never called. Object is permanently leaked.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed this is a pre-existing leak — leaving it out of this PR to keep the diff focused on the use-after-free.

}

return true;
Expand Down Expand Up @@ -216,7 +227,10 @@
return Exception { TypeError, errorMessage.toWTFString(BunString::ZeroCopy) };
}

worker->impl_ = impl;
{
Locker locker { worker->m_implLock };
worker->impl_ = impl;
}
worker->m_workerCreationTime = MonotonicTime::now();

return worker;
Expand Down Expand Up @@ -263,6 +277,9 @@
{
// m_contextProxy.terminateWorkerGlobalScope();
m_terminationFlags.fetch_or(TerminateRequestedFlag);
Locker locker { m_implLock };
if (!impl_)
return;
WebWorker__notifyNeedTermination(impl_);
}

Expand Down Expand Up @@ -467,6 +484,10 @@

extern "C" void WebWorker__dispatchExit(Zig::GlobalObject* globalObject, Worker* worker, int32_t exitCode)
{
// The Zig WebWorker struct is about to be freed on this (worker) thread.
// Block until any in-flight ref()/unref()/terminate() from the parent
// thread is done, then clear the pointer so they become no-ops.
worker->clearZigImpl();
worker->dispatchExit(exitCode);
// no longer referenced by Zig
worker->deref();
Expand Down
4 changes: 3 additions & 1 deletion src/bun.js/bindings/webcore/Worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class Worker final : public ThreadSafeRefCounted<Worker>, public EventTargetWith
void dispatchEvent(Event&);
void dispatchCloseEvent(Event&);
void setKeepAlive(bool);
void clearZigImpl();

void postTaskToWorkerGlobalScope(Function<void(ScriptExecutionContext&)>&&);

Expand Down Expand Up @@ -119,7 +120,8 @@ class Worker final : public ThreadSafeRefCounted<Worker>, public EventTargetWith
// Tracks TerminateRequestedFlag and TerminatedFlag
std::atomic<uint8_t> m_terminationFlags { 0 };
const ScriptExecutionContextIdentifier m_clientIdentifier;
void* impl_ { nullptr };
Lock m_implLock;
void* impl_ WTF_GUARDED_BY_LOCK(m_implLock) { nullptr };
};

JSValue createNodeWorkerThreadsBinding(Zig::GlobalObject* globalObject);
Expand Down
58 changes: 58 additions & 0 deletions test/js/web/workers/worker-terminate-race.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

// The Zig WebWorker struct is freed on the worker thread once the worker
// exits. These tests hammer ref()/unref()/terminate() from the parent
// thread while the worker thread is tearing down, which used to read the
// freed struct (ASAN use-after-poison in WebWorker__setRef /
// WebWorker__notifyNeedTermination).

async function run(src: string) {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", src],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toBe("");
if (exitCode !== 0) {
expect(stderr).toBe("");
}
expect(exitCode).toBe(0);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The run() helper in worker-terminate-race.test.ts checks stderr with expect.any(String) instead of expect(stderr).toBe(""), violating the convention explicitly documented in test/CLAUDE.md. This silently masks non-fatal warnings or error output during teardown races that don't cause a non-zero exit code.

Extended reasoning...

Bug description: The run() helper at line 19 of worker-terminate-race.test.ts asserts stderr using stderr: expect.any(String), which accepts any string content—including non-empty error output. The documented convention in test/CLAUDE.md (lines 48 and 94) explicitly shows the pattern expect(stderr).toBe("") for subprocess tests.

Specific code path: The assertion object is { stdout: "", stderr: expect.any(String), exitCode: 0 }. The expect.any(String) matcher passes as long as stderr is a string, making it equivalent to asserting nothing about stderr content. All three test cases share this single run() helper, so all three inherit the permissive check.

Why existing code doesn't prevent it: Jest/Bun test's toEqual matcher treats expect.any(String) as a type-only check ("is this a string?"), not a value check. Since stderr is always a string from proc.stderr.text(), the assertion can never fail regardless of what the subprocess prints.

Impact: The tests exist specifically to exercise race conditions during worker thread teardown (ref/unref/terminate while the worker exits). If the fix is incomplete or regresses, the process might print non-fatal diagnostic messages, ASAN symbolization notes, Zig panic-style warnings, or other runtime output to stderr before exiting cleanly with code 0. These would be silently swallowed. ASAN crashes are still caught via exit code, but softer issues are not.

Step-by-step proof: Suppose a regression causes WebWorker__setRef to print "WARNING: potential race detected\n" to stderr before returning normally (exit 0). With expect.any(String), the test passes. With expect(stderr).toBe(""), the test fails and the regression is caught. The empty workers ("data:text/javascript,") produce no console output by design, so there is no valid reason to tolerate non-empty stderr.

Fix: Change line 19 from stderr: expect.any(String) to stderr: "" (or restructure the assertion to use expect(stderr).toBe("") separately), matching the pattern used in neighboring tests like message-port-closed-leak.test.ts:51.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attempted fix (commit 1b946a8) improves failure diagnostics by splitting the combined assertion, but still does not fully address the original concern. The current code only checks expect(stderr).toBe("") when exitCode \!== 0, which means stderr is still silently ignored on clean exits (exitCode 0).

The original comment's core concern was: if a regression causes a non-fatal warning to be printed to stderr but the process still exits with code 0, the test would still pass silently. That scenario is unchanged.

The fix should unconditionally assert expect(stderr).toBe("") regardless of exit code:

expect(stdout).toBe("");
expect(stderr).toBe("");
expect(exitCode).toBe(0);

The empty data:text/javascript, workers produce no legitimate stderr output, so there is no valid reason to conditionally skip the assertion based on exit code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional is intentional. Debug/ASAN builds print WARNING: ASAN interferes with JSC signal handlers; useWasmFastMemory and useWasmFaultSignalHandler will be disabled. to stderr on every invocation, so an unconditional expect(stderr).toBe("") would make the test fail on every debug CI run. The exit-code check catches the actual use-after-free (ASAN aborts with non-zero exit), and stderr is surfaced in the failure output when that happens.


test.concurrent("Worker: ref/unref after terminate does not use-after-free", async () => {
await run(`
const w = new Worker("data:text/javascript,", {});
w.terminate();
for (let i = 0; i < 100000; i++) {
w.unref();
w.ref();
}
w.terminate();
w.unref();
`);
});

test.concurrent("Worker: ref/unref racing natural exit does not use-after-free", async () => {
await run(`
const w = new Worker("data:text/javascript,", {});
const end = Date.now() + 2000;
while (Date.now() < end) {
w.unref();
w.ref();
}
w.unref();
`);
});

test.concurrent("Worker: terminate racing natural exit does not use-after-free", async () => {
await run(`
const w = new Worker("data:text/javascript,", {});
const end = Date.now() + 2000;
while (Date.now() < end) {
w.terminate();
}
`);
});
Loading