Fix use-after-free in JSReadable*Controller end()/close() when onClose re-enters the event loop - #32597
Conversation
…ler.end/close
The generated JSReadable*Controller end() and close() host functions stashed
m_sinkPtr in a local, then called controller->detach() (which synchronously
invokes the JS onClose callback), and only afterward dereferenced the stashed
pointer via endWithSink()/close(). If the stream's pull() promise had already
settled, the onClose callback can re-enter the event loop and run the queued
on_resolve_stream reaction, which calls RequestContext::destroy_sink and
frees the HTTPServerWritable before endWithSink() reads from it.
Crash signature:
Segmentation fault at address 0xFFFFFFFFFFFFFFFF
endWithSink -> endFromJS -> finalize -> clearAndFree -> memset
Fix: clear the native pointer and run endWithSink()/close() first, then let
detach() fire onClose once we no longer hold a reference into the sink. The
signal.ptr is cleared via controllerDetached before endWithSink so
end_from_js's own signal.close() stays a no-op, matching the previous
behaviour.
Adds an ASAN-gated regression test that reproduces the heap-use-after-free
deterministically by draining microtasks from the stream's cancel() callback.
|
Updated 12:00 PM PT - Jun 22nd, 2026
❌ @robobun, your commit a1c3aec has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32597That installs a local version of the PR into your bun-32597 --bun |
|
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 (1)
WalkthroughThe codegen template for JSSink controllers ( ChangesSink controller shutdown re-entrancy fix
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
FileSink::end_from_js returns Err when flush() fails (e.g. ENOSPC), which js_end_with_sink converts to a thrown JS exception. The previous revision returned early on that exception and never called controller->detach(), so the stored onClose callback (which transitions the direct ReadableStream to closed/errored and calls underlyingSource.cancel()) never fired. Before the reorder onClose always ran regardless of whether endWithSink threw. Stash the sink's exception, run detach() (skipping only on a termination exception), discard any secondary onClose error, and rethrow the original.
There was a problem hiding this comment.
Thanks — the stash-and-rethrow in 2c0cd3d resolves my earlier concern about detach() being skipped on a throwing endWithSink. Not auto-approving since this reorders the native/JS teardown sequence across all six sink types and adds exception-handling at the boundary; worth a human pass on the lifecycle semantics.
Extended reasoning...
Overview
This PR fixes a use-after-free in the generated JSReadable*Controller end()/close() host functions (src/codegen/generate-jssink.ts). Previously, controller->detach() ran first (firing the onClose JS callback, which can re-enter the event loop and free the native sink), and only afterward was the stashed m_sinkPtr dereferenced via endWithSink() / close(). The fix reorders this: it calls __controllerDetached and nulls m_sinkPtr up front, runs the native end/close, and only then calls detach() to fire onClose. A follow-up commit (2c0cd3d) added stash-and-rethrow handling so detach() still runs if the native end/close throws a non-termination exception, addressing my earlier inline comment. An ASAN-gated regression test reproduces the original UAF deterministically.
Security risks
The bug being fixed is itself a memory-safety issue (heap UAF triggerable from user JS via cancel() re-entering the event loop). The fix reduces attack surface. I don't see new security risks introduced — the reordering is defensive and the exception-handling path correctly bails on termination exceptions without running more JS.
Level of scrutiny
High. This is generated C++ that runs for every direct ReadableStream controller across six sink types (ArrayBufferSink, FileSink, HTTP/HTTPS/H3 response sinks, NetworkSink), on a hot teardown path in Bun.serve. The change is small in line count but semantically subtle: it moves __controllerDetached (which clears the native-side signal) to before endWithSink/close, changes when m_sinkPtr is nulled relative to the native call, and introduces a stash/clear/rethrow exception dance that silently discards secondary errors from onClose. Each of these is reasonable in isolation and well-commented, but the interaction with each sink type's end_from_js / signal.close() behavior deserves a human reviewer who knows this lifecycle.
Other factors
- My prior 🔴 inline comment (skipped
detach()on throw) was addressed in 2c0cd3d and the thread is resolved; the 🟡 timeout nit was reasonably declined. - The bug-hunting system found no issues on the latest revision.
- CI shows musl build failures on an earlier commit (31f471a); status on the latest commit is not yet reflected.
- The PR description and inline comments are thorough, and the ASAN test gives good regression coverage for the specific crash, but it doesn't exercise the new exception-path (
endWithSinkthrowing) or the non-HTTP sink types.
There was a problem hiding this comment.
Thanks for the stash-and-rethrow fix in 2c0cd3d — that addresses my earlier concern. I'm not auto-approving since this reorders native sink teardown vs. JS re-entry across all six sink types and adds non-trivial JSC exception juggling; worth a human pass on the lifecycle.
Extended reasoning...
Overview
This PR changes the generated C++ for JSReadable*Controller::end() / close() (via src/codegen/generate-jssink.ts) so that the native endWithSink / close call runs before controller->detach() fires the JS onClose callback, fixing a Sentry-reported heap-use-after-free where onClose re-entered the event loop and freed the sink while a stale ptr was still on the C++ stack. A follow-up commit (2c0cd3d) added stash-and-rethrow around detach() so it still fires when the native end/close throws, addressing the regression I flagged in my first review. An ASAN-gated regression test reproduces the UAF deterministically.
Security risks
No new attack surface — this is a memory-safety hardening fix in an existing code path. The pre-fix behaviour was the dangerous one (UAF reachable from user cancel() callbacks).
Level of scrutiny
High. The change is small in line count but sits at the intersection of:
- Native pointer lifetime across a JS re-entry boundary, applied to 6 sink types (HTTP/HTTPS/H3 response, FileSink, NetworkSink, ArrayBufferSink) on the HTTP-serve hot path.
- JSC exception-scope semantics: the new stash-and-rethrow uses
tryClearException()(twice), distinguishes termination exceptions, deliberately swallows a secondaryonCloseerror in favour of the original sink error, and re-throws into the same scope. These are easy to get subtly wrong. - A behavioural ordering change:
controllerDetachednow runs beforeendWithSink, andonClosefires after the native end instead of before. The PR description argues this preserves the previoussignal.close()no-op behaviour, but verifying that across all sink implementations needs domain knowledge.
Other factors
- My earlier 🔴 finding (early-return skipping
detach()on throw) was fixed; the 🟡 timeout nit was reasonably justified and resolved. - The bug-hunting system found nothing on the latest revision.
- The fix is well-motivated (290 Sentry events) and comes with a deterministic ASAN repro plus claims of clean runs on the broader stream/server suites.
- coderabbit suggests cirospaciari, who owns much of this sink/RequestContext code — that's the right reviewer for the lifecycle invariants here.
Given the subtlety of the teardown ordering and the new exception-handling block, I'm deferring rather than auto-approving.
|
The diff itself is green locally and on every lane that touches it. Both CI failures are single unrelated tests on the
Neither touches stream controllers, JSSink, or the HTTP server. The new test ( |
2 participants
Sentry BUN-2WJA / BUN-2WKB (~290 events combined, Windows x86_64,
http_server=True, bun 1.2.23 through 1.3.14):Cause
The generated
JSReadable*Controllerend()andclose()host functions (src/codegen/generate-jssink.ts) stashm_sinkPtrin a local, callcontroller->detach(), and only afterward dereference the stashed pointer viaendWithSink()/${name}__close():detach()invokes the storedonClosecallback. For atype: "direct"stream this isreadDirectStream'sclose(stream, reason), which callsunderlyingSource.cancel(). That is arbitrary user code running whileptris still live on the C++ stack.If the stream's
pull()promise has already settled,RequestContext::on_resolve_streamis sitting in the microtask queue. Any path fromcancel()that drains microtasks (e.g. the server-side drain points inon_response/do_render_with_body, or an explicitdrainMicrotasks()) runshandle_resolve_stream, which callsdestroy_sinkand frees theHTTPServerWritable.endWithSink(ptr)then entersend_from_json the freed allocation;finalize()reads garbage forpooled_buffer/buffer.cap/buffer.ptrand faults in thememsetthe allocator's free-scrub path performs.The same ordering appears in the Rust port (
streams.rs/Sink.rs) unchanged.Fix
In
${controller}__endand${controller}__close, finish the native sink operation before any JS runs:${name}__controllerDetached(ptr, controller)and nullm_sinkPtrup front (soend_from_js's ownsignal.close()stays a no-op, matching the previous behaviour, and so the laterdetach()won't touch the native side again).endWithSink(ptr)/close(ptr).controller->detach()last. Withm_sinkPtralready null it only clearsm_onPulland firesonClose; by now we hold no reference into the sink, so re-entrant teardown is safe.Verification
New ASAN-gated test in
test/js/bun/http/serve-direct-readable-stream.test.tsreproduces the exact UAF deterministically by draining microtasks from the stream'scancel()callback (the test usesrequire("bun:jsc").drainMicrotasks()to force the drain that the production crash hits via the server's own drain points).ASAN output on the unfixed build
With the fix the fixture completes normally. Existing suites (
serve.test.ts,bun-server.test.ts,direct-readable-stream.test.tsx,streams.test.js, the sink leak tests) show no new failures against the unfixed build.