Repository navigation
node:http2: copy goaway opaqueData before writing over JS-backed sockets - #36905
Merged
Merged
Conversation
H2FrameParser.goaway borrowed the opaqueData ArrayBuffer and handed a slice into send_go_away, whose write() cork loop flushes through _write. On a JS-backed socket (BunSocket::None) _write calls onWrite, which runs the user Duplex's write() synchronously. That JS can detach or transfer the ArrayBuffer; the cork loop then resumes 'bytes = &bytes[avail..]' over the freed backing store, and send_go_away's subsequent binary_type.to_js(debug_data) copies it again. The GOAWAY debug-data payload that reaches the transport is whatever later occupies that heap block. Copy the bytes into an owned Vec at the JS boundary, matching read()'s existing defensive copy. goaway() runs once per session lifetime so the copy is not on a hot path.
Contributor
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 1960-1971: Update the GOAWAY handling around session.goaway and
the setImmediate callback to await a completion promise resolved by the frame
parser once a complete GOAWAY frame is present in received. Perform the payload
assertions only after that promise resolves, using bounded polling if necessary,
and remove reliance on a single arbitrary event-loop defer.
🪄 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: 22530043-0e91-4007-8835-7b5c2beefb44
📒 Files selected for processing (2)
src/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2.test.js
…mediate Resolve a promise once the Duplex has received the full 9+8+N byte frame rather than relying on one event-loop defer. Also shrink the payload to 64 KiB and the respray to 16 buffers so the test fits the default timeout under debug+ASan while still reproducing the stale-read deterministically.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
Author
|
CI status: the new
Diff is ready for review. |
This was referenced Aug 4, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 5, 2026
…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 -->
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 5, 2026
…uns mid-send (#36910) ## What Over a transport that runs user JS on write (a `createConnection` Duplex, or a `TLSSocket` upgraded from a JS Duplex via `tls.connect({ socket })`), `Http2Stream.write`/`end` can send **freed/recycled heap** as the DATA payload, and `ArrayBuffer.prototype.resize(0)` from inside the transport's `_write` **SEGVs** the process. ## Reproduction ```js import http2 from "node:http2"; import { Duplex } from "node:stream"; const MODE = process.argv[2] || "transfer"; // transfer | resize0 const SZ = 16374; let src = MODE === "resize0" ? new Uint8Array(new ArrayBuffer(SZ, { maxByteLength: SZ })) : new Uint8Array(SZ); for (let i = 0; i < SZ; i++) src[i] = 0x41 + (i % 23); const snap = Buffer.from(src), spray = [], wire = []; let armed = false, fired = 0; const duplex = new Duplex({ read() {}, write(chunk, enc, cb) { wire.push(Buffer.from(chunk)); if (armed && !fired++) { if (MODE === "resize0") src.buffer.resize(0); else { src.buffer.transfer(0); src = null; Bun.gc(true); for (let i = 0; i < 64; i++) spray.push(new Uint8Array(SZ).fill(0x5a)); } } cb(); }, }); const session = http2.connect("http://localhost:1", { createConnection: () => duplex }); session.on("error", () => {}); await new Promise(r => session.once("connect", r)); await new Promise(r => setTimeout(r, 20)); wire.length = 0; const req = session.request({ ":method": "POST", ":path": "/", "x-pad": "p".repeat(15000) }, { endStream: false }); req.on("error", () => {}); armed = true; req.write(src); // HEADERS still corked -> DATA straddles the 16 KiB cork await new Promise(r => setTimeout(r, 100)); const all = Buffer.concat(wire), parts = []; for (let i = 0; i + 9 <= all.length; ) { const len = all.readUIntBE(i, 3); if (all[i + 3] === 0) parts.push(all.subarray(i + 9, i + 9 + len)); i += 9 + len; } const got = Buffer.concat(parts); let diff = 0; for (let k = 0; k < got.length; k++) if (got[k] !== snap[k]) diff++; console.log({ MODE, dataBytes: got.length, foreignBytes: diff }); process.exit(0); ``` - `transfer` → `{ dataBytes: 16374, foreignBytes: 11280 }` (every foreign byte is the resprayed `0x5a`) - `resize0` → `panic(main thread): Segmentation fault` / ASan `SEGV` in `memcpy` ← `copy_from_slice` in `H2FrameParser::write`'s cork loop ← `write_all` ← `send_data` ← `write_stream` Node v26.3.0 is clean on both. ## Cause `writeStream` hands `send_data` a 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 can `transfer()` or `resize(0)` the buffer underneath it: | where JS runs mid-send | what goes stale | measured on main | |---|---|---| | the cork flush when a single DATA frame (≤16374 B) straddles the 16 KiB cork behind corked HEADERS | `bytes[avail..]` in `write()`'s loop | 11280/16374 foreign (the repro above); `resize(0)` SEGV | | the flush of the 9-byte DATA frame *header* when the cork is within 8 bytes of full | the whole payload, read by the next `write()` | 8000/8000 foreign | | `flush_batch_buffer()` right before the flow-control-limited tail is queued | the slice `queue_frame` copies | 32768/98303 foreign | | `cork()`: taking the cork slot flushes *another* session's corked bytes through that session's transport | the whole payload | 8000/8000 foreign (two Duplex sessions) | | any of the above with a `TLSSocket` over a JS Duplex (every TLS record is written through the Duplex) | same | 11283/16374 and 32768/98303 foreign, received by a real `createSecureServer` | The 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 `writeStream` boundary, mirroring the defensive copy the inbound `read()` path already makes: `stable_payload()` copies the payload into an owned buffer for the duration of `send_data` when this session's transport write runs JS (`BunSocket::None`, or a socket whose `InternalSocket` is `UpgradedDuplex`), 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* two `write()` calls of one send, and before `queue_frame`. `goaway`'s `opaqueData` was already given an owned copy at its boundary in #36905. ## Verification `test/js/node/http2/node-http2.test.js` gains a `describe.concurrent` block 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, `resize0` by crashing) and pass with this change; the rest of the file (317 tests) and the neighbouring http2 test files pass. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 7 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 (dcdca43) test/js/node/http2/node-http2.test.js: (pass) node none > Client Basics > should be able to send a GET request [768.83ms] (pass) node none > Client Basics > should be able to send a POST request [530.08ms] (pass) node none > Client Basics > constants [19.53ms] (pass) node none > Client Basics > getDefaultSettings [7.28ms] (pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [22.45ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [4.84ms] (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 [4.99ms] (pass) node none > Client Basics > should be able to send data using end [561.76ms] (pass) node none > Client Basics > should be able to mutiplex GET requests [553.74ms] (pass) node none > Client Basics > http2 should receive remoteSettings when receiving ... (truncated) release without fix: 101 failed, 6 skipped bun test v1.4.0-canary.1 (1498d7b) test/js/node/http2/node-http2.test.js: (pass) node none > Client Basics > constants [1.16ms] (pass) node none > Client Basics > getDefaultSettings [0.26ms] (pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.72ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.16ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.13ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.15ms] (pass) node none > Client Basics > headers cannot be bigger than 65536 bytes [2.78ms] (pass) node none > Client Basics > is possible to abort request [1.48ms] (pass) node none > Client Basics > aborted event should work with abortController [1.05ms] (pass) node none > Client Basics > aborted event should work with aborted signal [0.92ms] (pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [1.48ms] 783 | client.on("error", reject); 784 | const req = client.request({ ":path": "/", "test-hea ... (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 (dcdca43) test/js/node/http2/node-http2.test.js: (pass) node none > Client Basics > should be able to send a GET request [826.64ms] (pass) node none > Client Basics > should be able to send a POST request [551.36ms] (pass) node none > Client Basics > constants [17.78ms] (pass) node none > Client Basics > getDefaultSettings [6.75ms] (pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [21.72ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [4.79ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.38ms] (pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [4.98ms] (pass) node none > Client Basics > should be able to send data using end [579.83ms] (pass) node none > Client Basics > should be able to mutiplex GET requests [570.62ms] (pass) node none > Client Basics > http2 should receive remoteSettings when receiving ... (truncated) release with fix: 6 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision dcdca43 features baseline 22 deps, 108 codegen, 1175 objects in 1120ms ninja: Entering directory `/workspace/bun/build/release' [1/1238] install /workspace/bun bun install v1.4.0-canary.1 (1498d7b) Checked 124 installs across 170 packages (no changes) [46.00ms] [2/1238] install /workspace/bun/packages/bun-error bun install v1.4.0-canary.1 (1498d7b) Checked 1 install across 2 packages (no changes) [5.00ms] [3/1238] gen bindgenv2 [4/1238] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (1498d7b) Checked 129 installs across 147 packages (no changes) [16.00ms] [5/1238] gen ErrorCode+*.h [6/1238] fetch picohttpparser [picohttpparser] up to date [7/1238] fetch tinycc [tinycc] up to date [8/1238] gen .bind.ts → GeneratedBindings.cpp [9/1238] fetch libjpeg-turbo [libjpeg-turbo] up to date [10/1238] fetch zlib [zlib] up to date [11/1238] fetch nodejs (prebuilt) [nodejs] up to date [12/1238] host-cc deps/tinycc/codegen-tool [13/1238] subst deps/zlib/zli ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/api/bun/h2_frame_parser.rs | 41 ++++- test/js/node/http2/node-http2.test.js | 275 +++++++++++++++++++++++++++++++++ 2 files changed, 315 insertions(+), 1 deletion(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/api/bun/h2_frame_parser.rs 16 7 0 test/js/node/http2/node-http2.test.js 4 5 0 ``` </details> <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
session.goaway(code, lastStreamID, opaqueData)over a JS (non-native) transport could send freed/recycled heap bytes to the peer as GOAWAY debug data whenopaqueDataexceeds the 16 KiB cork buffer.Reproduction
245,777 of the 262,144 declared payload bytes come from the resprayed block, not the original
opaqueData. No ASan report (ArrayBuffer backing store is bmalloc/Gigacage); the wire-content oracle is the proof.Cause
H2FrameParser.goawayborrows theopaqueDataArrayBuffer (as_array_buffer+byte_slice()) and passes the slice intosend_go_away, which callsself.write(debug_data).write()'s cork loop fills the 16 KiB cork, thenflush_cork_buffer()→_write()→BunSocket::Nonearm callsonWrite, which runshttp2.ts'ssocket.write(buffer)→ the user Duplex'swrite()synchronously. That JS can detach/transfer the original buffer; the loop then resumesbytes = &bytes[avail..]over the freed backing store, andsend_go_awayalso copies the whole stale slice again viabinary_type.to_js(debug_data).Native TCP/TLS sessions (
generic_write) do not re-enter JS mid-write and are unaffected.Fix
Copy
opaqueDatainto an ownedVec<u8>at the JS boundary before any_writecan run, matching the existing defensive copy inread()ath2_frame_parser.rs:9321.goaway()runs at most once per session lifetime so the copy is not on a hot path.Scope
The
write_stream/send_datasingle-frame no-padding branch (h2_frame_parser.rs:7311) passes the same kind of borrowed slice throughwrite()and is intentionally left out of this PR. The ledger's control probe ofHttp2Stream.writeover the same Duplex measured it SAFE (61,440/61,440 DATA bytes original); that probe used a multi-frame payload which copies into the owned batch buffer. The single-frame window is narrow (payload 16,335-16,374 bytes with cork pre-filled), the trigger requires the user to detach the buffer they are themselves writing from inside their own transport, and DATA is the hot path. ABunSocket::None-gated copy there can be a follow-up if wanted.Verification
Before:
{ declared: 262144, orig_0x41: 16367, recycled_0x5a: 245777 }(3/3 runs)After:
{ declared: 262144, orig_0x41: 262144, recycled_0x5a: 0 }(3/3 runs)All 16 existing goaway tests in
test/js/node/http2/node-http2.test.jspass.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file