server.publish()/ws.publish(): convert topic and message before reading server state

[Jarred-Sumner]

What

server.publish(topic, data) borrowed the topic as a ZigString view into a JSString and then converted data with to_js_string(), which can run user JS and GC. When the topic came from toString()/toPrimitive the backing JSString was otherwise unreferenced and got collected, so uWS TopicTree::lookupTopic read a freed buffer (ASan heap-use-after-free READ of size 8000 with Malloc=1; silently mis-delivered otherwise). ServerWebSocket.publish/publishText/publishBinary had the same shape via the publish context fetched before the conversions.

Both paths now convert the topic and the message first — to_js_string() once each, holding the JSString*s (ensure_still_alive after the uWS call) and borrowing their views — and read self.app / the publish context exactly once, after that.

Repro (before)

const TOPIC = "t_".repeat(4000);
const srv = Bun.serve({ port: 0, fetch: (req, s) => s.upgrade(req) ? undefined : new Response("x"),
  websocket: { open(ws) { ws.subscribe(TOPIC); }, message() {} } });
const c = new WebSocket(`ws://127.0.0.1:${srv.port}/`); let got = null; c.onmessage = e => { got = e.data; };
await new Promise(r => { c.onopen = r; }); await Bun.sleep(50);
const topic = Object.assign(new String("z"), { [Symbol.toPrimitive]: () => "t_".repeat(4000).slice(0) + "" });
const data = Object.assign(new String("z"), { [Symbol.toPrimitive]() { Bun.gc(true); globalThis.k = Array.from({length: 300}, (_, i) => "Q".repeat(40000 + i)); Bun.gc(true); return "payload"; } });
srv.publish(topic, data);   // Malloc=1 ASan: heap-use-after-free READ of size 8000

Tests

test/js/bun/websocket/websocket-server.test.ts — "server.publish() keeps the topic alive while converting the message". Fails on the ASan canary and debug main; passes here.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 42 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 72 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: de63815c-a81d-4c3f-bf17-069d94f5067a

📥 Commits

Reviewing files that changed from the base of the PR and between fea1829 and aadbbb2.

📒 Files selected for processing (3)

• src/runtime/server/ServerWebSocket.rs
• src/runtime/server/server_body.rs
• test/js/bun/websocket/websocket-server.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Beyond the inline finding, I checked the refactored server_body.rs::publish() payload-resolution block: js_str.view(global).to_slice() dropping the temporary ZigString is fine (it's a Copy ptr+len; bytes live in the JSString kept alive via ensure_still_alive), JSValue::to_slice on the topic returns a ref-holding/owned ZigStringSlice that survives GC, and the removed second empty-topic check was unreachable.

Extended reasoning...

Checked the ~60-line refactor of server_body.rs::publish() for lifetime hazards introduced by collapsing three branches into one: the deferred-init let string_slice; outlives buffer, the array_buffer binding is hoisted so buffer.slice() stays valid, and to_boolean() on compress doesn't run user JS. The topic now goes through JSValue::to_slice (owning/ref-holding) instead of get_zig_string (borrowed view), which is the actual UAF fix. The removed "publish requires a non-empty topic" branch was dead — ZigString::to_slice() on a non-empty ZigString never yields an empty slice.

[claude]

🔴 send() has the same "to_js_string can run user JS" shape this PR guards in publish()/publishText(), but is left unguarded: an object message's [Symbol.toPrimitive] can call ws.terminate(), which synchronously runs webSocketData->~WebSocketData(), and the following self.websocket().send(...) then reads/appends to the destructed ext (getBufferedAmount(), buffer.append). close() in this file already re-checks is_closed() after coercion for exactly this reason — add if self.is_closed() { return Ok(JSValue::js_number(0.0)); } right after to_js_string here to complete the class.

Extended reasoning...

What the bug is

ServerWebSocket::send() calls message_value.to_js_string(global_this)? on any non-buffer, non-Blob argument without an is_string() gate (unlike sendText()), so an object argument's [Symbol.toPrimitive]/toString runs. That user JS can call ws.terminate() (or server.stop(true)), which — before send() proceeds — synchronously destructs the uWS per-socket ext data. The very next line, self.websocket().send(buffer, Opcode::Text, compress, true), then operates on a destructed C++ object.

Code path

1. send() reaches the string branch and calls to_js_string(global_this) on an object arg.
2. User [Symbol.toPrimitive] calls ws.terminate() → self.websocket().close() → uws_ws_close → us_socket_close().
3. us_socket_close() dispatches the close callback synchronously; WebSocketContext's close handler runs webSocketData->~WebSocketData(); (WebSocketContext.h:307), destructing the in-placed WebSocketData (whose base AsyncSocketData owns a std::string buffer).
4. Control returns to send(). self.flags still holds the same packed_websocket_ptr (nothing clears it), so self.websocket() returns the same pointer.
5. WebSocket::send() (bun-uws WebSocket.h:129+) reads getBufferedAmount() — the destructed std::string's .length() — and webSocketData->subscriber, and on the write2 short-write path calls webSocketData->buffer.append(...) on the destructed string. All of this is C++ UB; the append in particular is heap corruption if the string had freed its backing storage.

Why existing code doesn't prevent it

send() checks self.is_closed() before the coercion, not after. Nothing after to_js_string re-validates liveness. In contrast, close() in this same file already carries the guard — see the comment "to_slice_or_null can run user toString(), which may re-entrantly ws.close()… re-check the guard" followed by if self.is_closed() { return Ok(JSValue::UNDEFINED); }. And this PR adds the analogous re-check to publish() / publishText() after to_js_string. send() is the remaining sibling.

Addressing the refutation

One reviewer objected that (a) us_socket_close() does not free the socket allocation synchronously (it links to closed_head for end-of-tick free), so the pointer is not "dangling", and (b) this is not the same class as the PR's fix because send() reads neither the topic bytes nor handler.app.

Point (a) is correct about the allocation but does not make the code safe. The socket struct itself survives the tick, but its ext — the in-placed WebSocketData — is destructed synchronously at WebSocketContext.h:307 by the close handler that us_socket_close dispatches before returning. WebSocket::send() immediately reads from and (on the short-write branch) writes into that destructed object. Using a destructed non-trivial C++ object is undefined behavior regardless of whether its storage has been deallocated; if buffer had a heap allocation, ~basic_string() freed it and the subsequent .length()/.append() is a heap use-after-free of the string's backing. us_socket_write2 checking is_closed and returning 0 does not help — that 0 is what routes execution into the backpressure branch that touches buffer.

Point (b) reads the class too narrowly. REVIEW.md's rule is about the pattern — "Anything that can run user JS can synchronously free your state… re-validate liveness guards after every callback" — and this PR is applying exactly that pattern to to_js_string sites in this file. The PR's own added comment reads "to_js_string can run user JS that stops the server"; server.stop(true) force-closes this socket too, landing in the same ~WebSocketData() path. And close() already establishes is_closed()-after-coercion as the local idiom for this pattern. Per REVIEW.md — "Fix the whole class in the same PR… same-class sites are ONE concern, not scope creep. Grep for every sibling site sharing the pattern" — send() is a sibling site.

Step-by-step proof

// inside a websocket message handler:
ws.send(Object.assign(new String('z'), {
  [Symbol.toPrimitive]() { ws.terminate(); Bun.gc(true); return 'x'; }
}));

• send(): is_closed() → false; falls through buffer/Blob checks; calls to_js_string.
• toPrimitive runs → terminate() sets flags.closed = true, calls self.websocket().close() → us_socket_close → close handler runs webSocketData->~WebSocketData() and Rust on_close (which sees was_closed = true and skips accounting).
• to_js_string returns "x".
• self.websocket().send("x", Text, …) → WebSocket.h:133 reads getBufferedAmount() on destructed AsyncSocketData::buffer; line 147 reads destructed webSocketData->subscriber; if the ≥16 KB fast path is taken, us_socket_write2 returns 0 (closed) and the else-branch appends header+payload to the destructed buffer. UB / heap corruption.

Fix

One line, mirroring close() and this PR's publish() change:

let js_string = message_value.to_js_string(global_this)?;
if self.is_closed() {
    return Ok(JSValue::js_number(0.0));
}
let view = js_string.view(global_this);

[robobun]

^{Updated 9:10 PM PT - Aug 16th, 2026}

❌ @Jarred-Sumner, your commit aadbbb2 has 1 failures in Build #99820 (All Failures):

• test/cli/install/bun-install.test.ts - code 1 on 🐧 3.23 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 39389

That installs a local version of the PR into your bun-39389 executable, so you can run:

bun-39389 --bun

[claude]

🟡 Use Buffer.alloc(n, fill).toString() instead of "x".repeat(n) in the new test's subprocess script — "t_".repeat(4000) (twice) and "Q".repeat(40000 + i) in the 300-iteration loop. .repeat() is very slow in debug JSC builds (REVIEW.md harness convention), and the neighboring Sec-WebSocket-Protocol test in this file already follows the pattern with Buffer.alloc(128, ...).toString().

Extended reasoning...

What the issue is

The new "server.publish() keeps the topic alive while converting the message" test spawns a subprocess whose script builds large repetitive strings with String.prototype.repeat:

• const TOPIC = "t_".repeat(4000); — an 8,000-char topic
• [Symbol.toPrimitive]() { return "t_".repeat(4000).slice(0) + ""; } — the same 8,000 chars again per publish
• for (let i = 0; i < 300; i++) k.push("Q".repeat(40000 + i)); — ~12M chars of .repeat() inside the heap-spray loop

REVIEW.md's "Copy harness conventions exactly" section explicitly lists this: "Buffer.alloc(n, fill).toString() instead of \"x\".repeat(n) (slow in debug JSC)." test/CLAUDE.md carries the same rule. The subprocess runs the debug-built binary under Malloc=1, so debug-JSC .repeat() cost applies directly.

Why the convention matters here

The 300 × ~40k .repeat() loop is exactly the shape the rule targets — millions of characters through a debug-JSC intrinsic that is not JIT-optimized in debug builds. Under debug+ASAN this can add measurable wall-clock to a file that already runs a lot of subprocess tests, and REVIEW.md notes "a correct but slow test still gets changes-requested."

Why the substitution is safe for the repro

The test's GC-UAF repro needs (a) a fresh, otherwise-unreferenced JSString for the topic so Bun.gc(true) inside the message's toPrimitive can collect it, and (b) heap spray to reuse the freed block. Buffer.alloc(n, fill).toString() satisfies both:

• Buffer.alloc(8000, "t_").toString() allocates a fresh JSString each call from the native UTF-8 → JS path; nothing else references it, so it is just as collectible as the current "t_".repeat(4000).slice(0) + "" result. The .slice(0) + "" de-rope dance becomes unnecessary.
• Buffer.alloc(40000 + i, "Q").toString() produces the same-length, same-content spray strings — identical heap pressure.

The neighboring "Sec-WebSocket-Protocol … does not use-after-free" test in this same file (also a Malloc=1 ASAN repro of a freed StringImpl) already follows the convention: const part = Buffer.alloc(128, "abcdefghijklmnopqrstuvwxyz0123456789").toString();.

Step-by-step

1. Subprocess starts under the debug binary with Malloc=1.
2. "t_".repeat(4000) runs once for TOPIC — 8,000 chars via debug .repeat().
3. server.publish(topic, data) triggers topic[Symbol.toPrimitive] → another "t_".repeat(4000) + .slice(0) + concat.
4. Then data[Symbol.toPrimitive] runs the 300-iteration loop; each iteration calls "Q".repeat(40000 + i) — cumulatively ~12M chars through debug .repeat().
5. None of this .repeat() work is load-bearing for the assertion — the test only checks { rc: 7, result: "payload" } — so the equivalent Buffer.alloc(...).toString() output is byte-identical while running through Bun's native fast path.

Fix

const TOPIC = Buffer.alloc(8000, "t_").toString();
// …
const topic = Object.assign(new String("z"), { [Symbol.toPrimitive]() { return Buffer.alloc(8000, "t_").toString(); } });
// …
for (let i = 0; i < 300; i++) k.push(Buffer.alloc(40000 + i, "Q").toString());

Severity

Nit — a documented harness/style convention affecting debug-lane test speed, not correctness.

[Jarred-Sumner]

Re the send() note: the closed check after string conversion for send/sendText/subscribe/etc. is in #39385 (each method now converts its argument once, then does a single closed check right before the uWS call), so it isn't duplicated here.