node:http2: keep a stream.write() payload stable while transport JS runs mid-send - #36910
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
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)
WalkthroughHTTP/2 DATA writes now copy borrowed payloads when synchronous JavaScript transport re-entry can mutate or detach the source buffer. Regression tests cover cork boundaries, frame-header boundaries, flow-control queues, cross-session cork handoff, and TLS over JavaScript ChangesHTTP/2 write safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 3437-3451: In the send path containing self.cork() and the bytes
slice, copy borrowed payload data before calling self.cork(), because uncorking
a foreign CORKED_H2 may execute JavaScript and invalidate the backing
ArrayBuffer. Ensure all later copy_from_slice(bytes) and _write(bytes)
operations use the stable owned data, regardless of native_socket state or
post-cork CORK_OFFSET values, and add a two-session regression covering the
invalidation scenario.
- Around line 3432-3437: Update write to acquire and retain self.keepalive()
before calling self.cork() when ENABLE_AUTO_CORK is enabled, keeping the guard
alive through the subsequent auto-cork and write flow so re-entry cannot release
this parser prematurely.
In `@test/js/node/http2/node-http2.test.js`:
- Around line 3024-3037: Update the test around the session.request flow to
replace no-op session and request error handlers and fixed setImmediate waits
with an observable completion promise tied to the armed Duplex receiving the
expected DATA output. Resolve it when the expected wire output is observed,
reject it from both transport error events, await it before clearing or parsing
wire, and use bounded polling for any remaining deferred-flush detection.
- Line 3030: Update the POST request fixture header in the surrounding HTTP/2
test to create the 15,000-character padding with Buffer.alloc(15000,
"p").toString() instead of "p".repeat(15000), preserving the existing header
value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6245972c-d112-421c-85ec-2c2eef3f92a9
📒 Files selected for processing (2)
src/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…t JS can run mid-send
Http2Stream.write hands send_data a slice borrowed from the caller's
ArrayBuffer. When the session's transport is user JS (a createConnection
Duplex, or a TLSSocket upgraded from a JS Duplex via tls.connect({ socket })),
that JS runs synchronously at several points before the send has consumed the
slice, and can transfer() or resize(0) the buffer underneath it:
- the mid-write cork flush when a single DATA frame straddles the 16 KiB cork
- the flush of the 9-byte DATA frame header itself when the cork is within 8
bytes of full (the payload is then read only after JS ran)
- flush_batch_buffer() before the flow-control-limited tail is queued
- cork(): taking the cork slot first flushes another session's corked bytes
through that session's transport, so a second session's JS runs too
Each of these put freed/recycled heap on the wire as DATA payload (and
resize(0) SEGVs in the cork copy). A copy inside write() cannot cover them
because the slice goes stale between two write() calls of one send.
Decide once at the writeStream boundary instead, mirroring the inbound copy
read() already makes: if this session's transport write runs JS, or the cork
slot is held with pending bytes by a session whose transport does, copy the
payload into an owned buffer for the duration of send_data. Native TCP/TLS
sockets over real connections never run JS from a write and stay zero-copy.
e19ac88 to
1ba41f8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/http2/node-http2.test.js`:
- Around line 3105-3108: Update the preface validation in the pending-data
handling block to throw immediately when pending.subarray(0, PREFACE.length)
does not equal PREFACE, before setting prefaceSeen or entering frame processing.
Include an error message identifying the HTTP/2 preface, the rejected bytes or
mismatch, the cause, and the concrete remedy; preserve the existing successful
path that removes the matched preface.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcd3d423-7deb-4bb3-b135-98756644a31a
📒 Files selected for processing (2)
src/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2.test.js
robobun
changed the title
|
Reworked in 1ba41f8 after the review round; summary for whoever picks this up: The first revision copied inside So the copy now happens once at the |
…ign-cork clause is load-bearing
There was a problem hiding this comment.
ff073fe addresses my last note — the new "native writer" cork-handover case connects session B over plain TCP to a local h2c server and asserts native: true / firedDuringWrite: true, so foreign_cork_runs_js() is now the deciding term there. No further findings from this pass.
Deferring to a human for the final sign-off: this is a memory-safety fix in the HTTP/2 send path that adds an unsafe raw-pointer read of the foreign corked parser and a new predicate reaching into bun_uws::InternalSocket.
What was reviewed: the stable_payload copy sits before every JS re-entry point send_data can reach (own-transport flush, header-write straddle, flush_batch_buffer before queue_frame, and cork()'s foreign uncork); the new unsafe deref reuses the exact SAFETY invariant already stated at cork():2826; native TCP/TLS keeps the borrowed path (Cow::Borrowed); the 9 subprocess cases each assert fired/firedDuringWrite so a re-entry that stops happening fails the test rather than passing vacuously.
Extended reasoning...
Overview
The PR fixes a use-after-free / SEGV in node:http2 where Http2Stream.write()/end() hands send_data a slice borrowed from the caller's ArrayBuffer, and any transport whose write path runs user JS synchronously (a createConnection Duplex, or a TLSSocket upgraded from a JS Duplex) can transfer(0)/resize(0) that buffer mid-send. The fix is a single boundary-level decision in writeStream: stable_payload() returns Cow::Owned when this session's transport (or the foreign session currently holding the thread-local cork slot with pending bytes) can run JS on write, otherwise Cow::Borrowed. Two small helpers (transport_write_runs_js, stable_payload) and one call site in h2_frame_parser.rs; ~330 lines of subprocess tests in node-http2.test.js.
Security risks
The bug being fixed is itself the security concern — reading freed heap into a network frame, and a reachable SEGV. The fix does not widen any attack surface; the only new unsafe is a read-only deref of CORKED_H2's pointer under the same "ref()'d until uncork()" invariant already relied on by cork() at line 2826. transport_write_runs_js() is a pure classifier over an enum. No new user-controlled input is parsed.
Level of scrutiny
High. This is native memory-safety code in a hot production path (every HTTP/2 DATA write), it introduces an unsafe block, and the correctness of the guard depends on having enumerated every synchronous JS re-entry point reachable from send_data. The PR went through three review iterations here: the first revision copied inside write() and missed the between-calls and foreign-cork surfaces; the current revision moves the copy to the writeStream boundary before any of them. That evolution and the surface-by-surface byte-count evidence in the description give good confidence in the mechanism, but a maintainer should confirm the InternalSocket::UpgradedDuplex match is the right layer to key on and that no other send_data caller borrows a JS slice.
Other factors
All earlier review threads are resolved. My last remaining note (the foreign_cork_runs_js() clause not being load-bearing) is answered by ff073fe's native-writer handover test, which asserts native: true via !!sessionB.socket._handle and firedDuringWrite: true, so B's transport_write_runs_js() is false and only the foreign-cork clause protects it. Tests await observable conditions (dataSeen(n), server-received body), wire every error to a nonzero subprocess exit, and use Buffer.alloc(n, fill).toString() per harness convention. The change keeps native sockets zero-copy, so there is no performance regression on the common path. Given it is not a simple/mechanical change and sits squarely in REVIEW.md's most-blocked category, I am deferring rather than approving.
…rite (#36917) ## What `node:http2` over a user-supplied Duplex transport (`createConnection`) with `paddingStrategy` enabled: writing to a second stream from inside the transport's `_write` aborts the process with `panic: RefCell already borrowed`. ## Reproduction ```js import http2 from "node:http2"; import { Duplex } from "node:stream"; let req2, armed = false, fired = 0; const duplex = new Duplex({ read() {}, write(chunk, enc, cb) { if (armed && fired++ === 0) req2.write(new Uint8Array(3000).fill(0x42)); cb(); }, final(cb) { cb(); }, }); const session = http2.connect("http://localhost:1", { createConnection: () => duplex, paddingStrategy: http2.constants.PADDING_STRATEGY_MAX }); session.on("error", () => {}); await new Promise(r => session.once("connect", r)); const f = (t, fl, sid, p) => { const h = Buffer.alloc(9); h.writeUIntBE(p.length, 0, 3); h[3] = t; h[4] = fl; h.writeUInt32BE(sid, 5); return Buffer.concat([h, p]); }; const set = Buffer.alloc(6); set.writeUInt16BE(4, 0); set.writeUInt32BE(0x7fffffff, 2); const wu = Buffer.alloc(4); wu.writeUInt32BE(0x70000000, 0); duplex.push(Buffer.concat([f(4, 0, 0, set), f(8, 0, 0, wu), f(4, 1, 0, Buffer.alloc(0))])); // server preface by hand await new Promise(r => setTimeout(r, 50)); req2 = session.request({ ":method": "POST", ":path": "/side" }, { endStream: false }); req2.on("error", () => {}); const req = session.request({ ":method": "POST", ":path": "/" }, { endStream: false }); req.on("error", () => {}); await new Promise(r => setTimeout(r, 30)); const pad = session.request({ ":method": "POST", ":path": "/pad", "x-pad": "q".repeat(15000) }, { endStream: false }); pad.on("error", () => {}); // ~13 KB corked HEADERS armed = true; req.write(new Uint8Array(12000).fill(0x41)); // padded DATA (12256+9) crosses the 16 KiB cork -> flush -> duplex.write() -> req2.write() await new Promise(r => setTimeout(r, 100)); console.log("no crash"); ``` Before: `panic: RefCell already borrowed` / "oh no: Bun has crashed", exit 134, every run (`core::cell::panic_already_borrowed` <- `H2FrameParser::send_data` <- `write_stream`). Node v26.3.0 prints `no crash`. ## Cause `send_data`'s padded single-frame branch (and the two padded branches in `Stream::flush_queue`) built the payload inside `SHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| { ...; writer.write_all(&buffer[..payload_size]) })`, so the `write_all` ran inside the borrow. When the frame crosses the cork boundary, `write()` -> `flush_cork_buffer()` -> `_write()` -> `onWrite` runs the Duplex `_write` (user JS) with the thread-local still mutably borrowed; the nested `req2.write()` -> `send_data` borrows it again and panics. Native TCP/TLS transports never run JS from `_write`, so only JS-backed transports are affected. The rest of the write path already follows the rule that no thread-local borrow is held across `_write` (`uncork`, `flush_batch_buffer`, `flush_cork_buffer` move their Vec out first); these three sites predate it. ## Fix Add `DirectWriterStruct::write_padded(data, padding)` and use it at all three sites. The scratch stays shared and reusable, but it now lives in the VM's `RareData` (per review: a thread-local static is wrong with worker_threads; per-VM is the right scope). `write_padded` takes the buffer out of its `rare_data` slot by value for the duration of the write, so nothing is borrowed across `write()`: a re-entrant padded write finds the slot empty, allocates its own buffer, and the slot keeps one buffer for reuse when they return. `SHARED_REQUEST_BUFFER` is removed, along with the three `unsafe { ptr::copy }` blocks. Frame ordering on the wire when a JS transport re-enters the session mid-frame is unchanged by this PR (it behaves like the unpadded path does today, see #36918 for that); this only removes the abort and keeps each frame's own bytes intact. ## Verification New test in `test/js/node/http2/node-http2.test.js`: three padded DATA writes in one tick over a JS Duplex (a corked fill frame, an outer frame that crosses the cork boundary, and a side-stream write issued from inside the transport's `_write` during that flush), then counts the payload bytes that reach the transport. Expected `{ reentered: true, total: 28795, A: 12000, B: 3000, C: 13000 }`, which is also what node v26.3.0 produces for the same script. - before (release and debug+ASAN): subprocess aborts with `panic: RefCell already borrowed`, test fails - after: passes; full `node-http2.test.js`: 311 pass, 6 skip, 0 fail Related: #36905 (merged) and #36910 cover the borrowed-payload side of the same re-entrant Duplex write path; this one is independent of both (the padded path already copied its payload, the problem was the held borrow). <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 failed, 6 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js" bun test v1.4.0 (110748b) test/js/node/http2/node-http2.test.js: (pass) node none > Client Basics > should be able to send a GET request [762.93ms] (pass) node none > Client Basics > should be able to send a POST request [526.52ms] (pass) node none > Client Basics > constants [19.71ms] (pass) node none > Client Basics > getDefaultSettings [7.26ms] (pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [22.03ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [4.89ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.46ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [4.85ms] (pass) node none > Client Basics > should be able to send data using end [558.09ms] (pass) node none > Client Basics > should be able to mutiplex GET requests [549.02ms] (pass) node none > Client Basics > http2 should receive remoteSettings when receiving ... (truncated) release without fix: 2 failed, 6 skipped bun test v1.4.0-canary.1 (b66764f) test/js/node/http2/node-http2.test.js: (pass) node none > Client Basics > constants [0.85ms] (pass) node none > Client Basics > getDefaultSettings [0.15ms] (pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.40ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.10ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.04ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.06ms] (pass) node none > Client Basics > is possible to abort request [3.70ms] (pass) node none > Client Basics > aborted event should work with abortController [0.83ms] (pass) node none > Client Basics > aborted event should work with aborted signal [0.75ms] (pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [1.18ms] (pass) node none > Client Basics > headers cannot be bigger than 65536 bytes [57.33ms] (skip) node none > Client Basics > should not leak memory (pass) node none > Client Basics > close callback [53 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 6 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js" bun test v1.4.0 (110748b) test/js/node/http2/node-http2.test.js: (pass) node none > Client Basics > should be able to send a GET request [1184.19ms] (pass) node none > Client Basics > should be able to send a POST request [774.97ms] (pass) node none > Client Basics > constants [31.96ms] (pass) node none > Client Basics > getDefaultSettings [11.95ms] (pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [33.02ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [4.86ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.52ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.46ms] (pass) node none > Client Basics > should be able to send data using end [827.20ms] (pass) node none > Client Basics > should be able to mutiplex GET requests [806.11ms] (pass) node none > Client Basics > http2 should receive remoteSettings when receivin ... (truncated) release with fix: 6 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 930ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/124] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited [2/124] gen cpp.rs (cppbind) [2/124] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_alloc v0.0.0 (/workspace/bun/src/bun_alloc) �[1m�[92m Compiling�[0m bun_libdeflate_sys v0.0.0 (/workspace/bun/src/libdeflate_sys) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/sr ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/rare_data.rs | 27 ++++++++++-- src/runtime/api/bun/h2_frame_parser.rs | 75 ++++++++++++++-------------------- test/js/node/http2/node-http2.test.js | 72 ++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 47 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/rare_data.rs 4 8 0 src/runtime/api/bun/h2_frame_parser.rs 11 13 0 test/js/node/http2/node-http2.test.js 3 5 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
What
Over a transport that runs user JS on write (a
createConnectionDuplex, or aTLSSocketupgraded from a JS Duplex viatls.connect({ socket })),Http2Stream.write/endcan send freed/recycled heap as the DATA payload, andArrayBuffer.prototype.resize(0)from inside the transport's_writeSEGVs the process.Reproduction
transfer→{ dataBytes: 16374, foreignBytes: 11280 }(every foreign byte is the resprayed0x5a)resize0→panic(main thread): Segmentation fault/ ASanSEGVinmemcpy←copy_from_sliceinH2FrameParser::write's cork loop ←write_all←send_data←write_streamNode v26.3.0 is clean on both.
Cause
writeStreamhandssend_dataa slice borrowed from the caller's ArrayBuffer (StringOrBuffer::from_js_with_encoding(..).slice()). When the session's transport is user JS, that JS runs synchronously at several points before the send has consumed the slice, and cantransfer()orresize(0)the buffer underneath it:bytes[avail..]inwrite()'s loopresize(0)SEGVwrite()flush_batch_buffer()right before the flow-control-limited tail is queuedqueue_framecopiescork(): taking the cork slot flushes another session's corked bytes through that session's transportTLSSocketover a JS Duplex (every TLS record is written through the Duplex)createSecureServerThe multi-frame path already copies into the owned batch buffer for non-TCP sockets, which is why larger single writes previously measured safe. Native TCP/TLS sockets over real connections never run JS from a write and are unaffected.
Fix
Decide once at the
writeStreamboundary, mirroring the defensive copy the inboundread()path already makes:stable_payload()copies the payload into an owned buffer for the duration ofsend_datawhen this session's transport write runs JS (BunSocket::None, or a socket whoseInternalSocketisUpgradedDuplex), or when the cork slot is currently held, with pending bytes, by a session whose transport does. Otherwise it stays borrowed, so native sockets keep the zero-copy path.A copy inside
write()(the first revision of this PR) cannot cover this: the slice goes stale between twowrite()calls of one send, and beforequeue_frame.goaway'sopaqueDatawas already given an owned copy at its boundary in #36905.Verification
test/js/node/http2/node-http2.test.jsgains adescribe.concurrentblock with one subprocess case per row above (wire-content oracle; for the TLS cases the oracle is the body the server receives). All 7 fail on main (5 with foreign bytes on the wire,resize0by crashing) and pass with this change; the rest of the file (317 tests) and the neighbouring http2 test files pass.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file