node:http2: don't hold a thread-local borrow across the padded DATA write - #36917
Conversation
…rite send_data() and Stream::flush_queue() assembled a PADDED DATA payload inside SHARED_REQUEST_BUFFER.with_borrow_mut() and called write_all() from inside the closure. write() can flush the cork, and over a JS-backed transport that re-enters JS (onWrite -> Duplex _write). A transport that writes to another padded stream from its _write reached send_data() again with the RefCell still mutably borrowed and the process aborted with 'panic: RefCell already borrowed'. Move the scratch out of its thread-local slot for the duration of the write (the same take/put-back idiom uncork()/flush_batch_buffer() use), so a nested padded write gets its own buffer and can neither trip the borrow nor overwrite bytes the outer write() is still copying into the cork.
|
Status: reproduced on canary b66764f and current main (exit 134, |
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughHTTP/2 padded DATA serialization now builds an owned payload for each write. Partial, full, and single-frame paths use this logic. A subprocess test covers nested writes through a JavaScript Duplex transport. ChangesHTTP/2 padded DATA writes
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: 2
🤖 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`:
- Line 2024: Update the HTTP/2 fixture’s session and stream error handlers near
session.on("error") and the corresponding stream handler to route errors into
the completion promise’s rejection path instead of discarding them. Ensure the
success path around the final completion reporting only runs when no error has
occurred, so any error before completion produces a failing test result.
- Around line 2026-2051: The HTTP/2 test must validate actual outbound frame
ordering and structure rather than relying on scheduler turns and aggregate byte
counts. Update the fixture around the frame setup, `armed`, and `outer.write` to
buffer transport output and await an explicit headers-drained condition before
arming; then decode the buffered DATA frames and assert each frame’s stream ID,
PADDED flag, declared length, pad length, zero-filled padding, and expected
A/B/C payload bytes while preserving the `reentered` assertion.
🪄 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: 09156521-4598-438a-8fa1-831eacc618b4
📒 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}
There was a problem hiding this comment.
LGTM — clean fix for the re-entrant padded DATA panic, following the same take/put-back pattern the rest of the write path already uses.
What was reviewed:
- Traced
payload_sizebounds at all three call sites:get_paddingclamps tomax_len ≤ MAX_PAYLOAD_SIZE_WITHOUT_FRAME - 1, so1 + data.len() + padding ≤ 16374 < H2_CORK_BUFFER_SIZE— the safecopy_from_slicecannot panic. - Verified the
Cell<Option<Box<[u8]>>>::take()re-entrancy handling: outer call owns its buffer acrosswrite_all, nested call allocates fresh; the final.set()after both return drops one and retains one, no leak. - Confirmed
SHARED_REQUEST_BUFFERhad no other users; the oldptr::copy"may overlap" comment was spurious (sources never aliased the thread-local), so switching to safecopy_from_sliceis a strict improvement. - Test is event-driven (awaits byte-count completion, no sleeps), drains pipes concurrently, and asserts byte-exact payload counts matching Node — covers both the abort and the buffer-clobber hazard.
Extended reasoning...
Overview
This PR fixes a process abort (panic: RefCell already borrowed) in node:http2 when a padded DATA write over a JS-backed Duplex transport crosses the 16 KiB cork boundary and the transport's _write re-enters send_data. Three copy-pasted padded-write blocks in h2_frame_parser.rs (two in Stream::flush_queue, one in H2FrameParser::send_data) are replaced with a single DirectWriterStruct::write_padded helper. The thread-local scratch is changed from RefCell<Box<[u8; 16384]>> (borrowed across the write) to Cell<Option<Box<[u8]>>> (moved out for the duration), so a nested padded write allocates its own buffer instead of tripping a held borrow or overwriting the outer frame's bytes mid-copy. A subprocess test in node-http2.test.js reproduces the re-entrant scenario and asserts byte-exact payload counts matching Node.
Security risks
None. This is internal buffer management for an outbound HTTP/2 frame path; no untrusted input parsing is touched. The change removes three unsafe { ptr::copy } blocks in favor of safe slice ops. Buffer indexing is bounded: get_padding(size, max_size - 1) guarantees padding ≤ max_size - 1 - size, so payload_size = 1 + size + padding ≤ max_size ≤ MAX_PAYLOAD_SIZE_WITHOUT_FRAME = 16374 < 16384 at every call site, and the buffer is 16384 bytes (same as before).
Level of scrutiny
Medium. h2_frame_parser.rs is core protocol code, but this change is narrowly scoped to one bug class already being addressed in this file (#36905 merged, #36910 open — same re-entrant JS-Duplex write path). The fix follows the exact pattern the PR body notes is already established for uncork/flush_batch_buffer/flush_cork_buffer ("move the Vec out first"), and the helper lives on DirectWriterStruct next to write_all, which is the right layer. The take/put-back idiom is standard Rust for this re-entrancy shape; on nested return the inner buffer is dropped and the outer is retained — no leak, no double-free.
Other factors
The test is well-constructed per the repo's review rules: spawned subprocess with bunEnv, pipes drained via Promise.all, stderr asserted before exitCode, awaits the observable condition (total >= EXPECTED_WIRE) rather than sleeping, and asserts the strongest invariant — exact per-byte-value counts ({A: 12000, B: 3000, C: 13000}) that would detect the outer buffer being clobbered by the nested write, not just the absence of a crash. The PR body confirms the test fails on the unfixed build. The change also deduplicates ~36 lines into one helper and deletes three unsafe blocks, which is the shape the review guide asks for ("the second time a multi-line block appears in your diff, extract a named helper").
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
We shouldn't use static for any of these, there can be multiple threads due to worker_threads.
…all buffer Also make the test fixture exit non-zero on any session/stream 'error' and only issue the nested write from the chunk that carries the outer frame's payload (the mid-frame cork flush).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The three padded-DATA sites now emit the same bytes as before (payload_size == 1 + data.len() + padding at each caller matches what write_padded produces), SHARED_REQUEST_BUFFER has no remaining references, and three unsafe blocks are gone. Deferring to a human because the second commit swaps a reused thread-local scratch for a per-frame Vec allocation on the padded path — probably fine given padding is opt-in and flush_queue already .to_vec()s nearby, but worth a maintainer nod. Note the PR body still describes the abandoned Cell<Option<Box<[u8]>>> take/put-back approach from the first commit; the two comment-cop notes fired ~18s after 5fc4e38 landed, so they're on the current 3-line invariant comment — reads like a false positive to me, but flagging it since it's technically unresolved.
Extended reasoning...
Overview
Fixes a RefCell already borrowed panic in node:http2 when a padded DATA write over a JS-backed Duplex transport re-enters send_data from inside the transport's _write. The three call sites in Stream::flush_queue (×2) and H2FrameParser::send_data that previously assembled the padded payload inside SHARED_REQUEST_BUFFER.with_borrow_mut(|b| { ...; writer.write_all(...) }) now call a new DirectWriterStruct::write_padded(data, padding) helper that builds the payload in a per-call Vec and holds no borrow across write_all. The thread-local is removed entirely. A subprocess test in node-http2.test.js reproduces the re-entrancy and asserts the exact wire byte total and per-stream payload counts that Node produces.
Correctness verified
At all three call sites payload_size = data.len() + padding as usize + 1 when padding != 0 (h2_frame_parser.rs:1708, 1757, 7250), and the frame header's length is set to that value. write_padded emits [padding, ...data, ...zeros(padding)], i.e. exactly 1 + data.len() + padding bytes — byte-identical to the old buffer[0..payload_size] write, so the wire format is unchanged. Grep confirms no remaining references to SHARED_REQUEST_BUFFER. The old ptr::copy overlap concern is moot since the destination is a fresh Vec.
Security risks
None identified. The change removes unsafe code and a shared-mutable-state hazard; padding bytes are still zero-filled; frame lengths are unchanged.
Level of scrutiny
Medium-high. HTTP/2 framing is protocol-critical, but the diff is a mechanical consolidation of three identical blocks into one helper with equivalent output. The interesting decision is the second commit (5fc4e38): it drops buffer reuse in favor of a per-frame allocation. That's on an opt-in path (paddingStrategy) and adjacent code already allocates (able_to_send = ....to_vec() at :1700), so it's likely acceptable — but it's a perf/simplicity tradeoff a maintainer should acknowledge rather than a bot.
Other factors
- PR description is stale: it describes a
Cell<Option<Box<[u8]>>>take/put-back scheme, but the shipped code (after 5fc4e38) uses a plain per-callVec. - comment-cop fired at 22:00:10Z on lines 6145/6149; commit 5fc4e38 landed at 21:59:52Z, so those flags are on the current code. The flagged comment is a 3-line note explaining why no shared scratch may be held across
write()— that's a load-bearing invariant, not a workaround justification, so I read it as a heuristic false positive. - CodeRabbit's "fail on error events" note appears already addressed in the current diff (
fail(...)→process.exit(1)); its "validate frame structure" suggestion would strengthen the test but the existing exact-byte-count assertion (matching Node) is a reasonable invariant for what this PR fixes. - robobun confirms fails-before/passes-after on debug+ASAN with the full suite green.
|
Dropped it. For what it's worth, the slot I had added in the first commit lived inside the existing |
Per review: a shared reusable buffer is right, but it belongs to the VM (worker_threads means multiple JS threads), not a static. write_padded takes the buffer out of rare_data by value for the duration of the write, so the re-entrant case allocates its own instead of aliasing the outer one, and the slot keeps one buffer for reuse.
|
Moved the scratch into the VM's |
There was a problem hiding this comment.
No bugs found; all earlier nits (stale TLS comment, PR-description drift, doc-comment consolidation) are addressed. Deferring rather than approving because the fix places new per-VM state in RareData and relies on take-by-value semantics across a JS re-entry point in the h2 write path — sound as far as I can tell, but the design iterated three times during review and a maintainer should sign off on where the scratch lives.
What was reviewed:
write_paddedholds no borrow acrosswrite_all; a re-entrant call finds the slot empty andboxed_zeroeds its own;put_backviaget_or_insertcorrectly drops the second-returned buffer.- Payload bound: all three call sites cap
data.len()andpaddingviaMAX_PAYLOAD_SIZE_WITHOUT_FRAME, so1 + data.len() + padding ≤ 16374 < 16384— the fixed buffer cannot overflow. - The three removed
unsafe { ptr::copy }blocks are behavior-equivalent tocopy_from_slice+fill(0)for these bounds; the new path is strictly safer.
Extended reasoning...
Overview
Fixes a RefCell already borrowed panic in node:http2 when a JS-backed Duplex transport (via createConnection) re-enters the padded DATA send path from inside its own _write. Three sites in h2_frame_parser.rs (Stream::flush_queue × 2, H2FrameParser::send_data) previously assembled the padded payload inside SHARED_REQUEST_BUFFER.with_borrow_mut(|b| { …; writer.write_all(&b[..]) }), so the thread-local RefCell was still mutably borrowed when write_all → cork flush → onWrite ran user JS that called back into send_data.
The fix removes SHARED_REQUEST_BUFFER, adds an Option<Box<[u8; 16384]>> slot to RareData with take_/put_back_ accessors, and adds DirectWriterStruct::write_padded that takes the buffer by value, writes, then puts it back — so nothing is borrowed across the write and a nested caller allocates its own. Also updates the stale "32 KB combined" TLS comment and adds a subprocess regression test that fails-before / passes-after (verified in the PR evidence block).
Security risks
None identified. This is internal buffer management on the outbound path; no untrusted input parsing changes. The removed unsafe { ptr::copy } blocks are replaced with checked copy_from_slice, so bounds handling is stricter than before.
Level of scrutiny
Medium-high. The h2 outbound write path is production-critical for node:http2 compat, and the change threads through a JS re-entry point (write_all can synchronously run user JS over a Duplex transport). I verified: (1) the take-by-value pattern means no &mut or RefCell borrow spans the re-entry; (2) bun_vm().as_mut().rare_data() is called once before and once after write_all, never held across it, matching the established single-JS-thread escape-hatch pattern in this file and rare_data.rs; (3) payload_size is bounded by MAX_PAYLOAD_SIZE_WITHOUT_FRAME = 16374 at every call site (get_padding returns min(255, max_len - frame_len)), so the 16 KB fixed buffer cannot be overrun; (4) put_back uses get_or_insert, so when the outer call returns after a nested one already put its buffer back, the outer's buffer drops harmlessly.
Other factors
The design iterated three times during review (thread-local Cell<Option<Box>> → per-call Vec → RareData take/put). The final placement follows the existing temp_pipe_read_buffer pattern in RareData and is well-scoped, but adding per-VM state and choosing shared-scratch-vs-per-call-alloc is the kind of tradeoff a maintainer typically confirms. All prior review threads (CodeRabbit, comment-cop, my earlier nits) are resolved. The regression test is subprocess-based, drains stdout/stderr/exited concurrently, uses bunEnv, and asserts an exact node-matched {reentered, total, A, B, C} object rather than a loose match; frame-ordering under re-entrancy is explicitly out of scope (#36918).
There was a problem hiding this comment.
LGTM — the take-by-value RareData scratch cleanly removes the held RefCell borrow across the re-entrant write, and my earlier notes (stale PT_TLS comment, description sync, comment consolidation) are all addressed.
What was reviewed
write_padded: buffer is moved out of the slot beforewrite_all, so the re-entrant path findsNoneand allocates its own; no borrow held across the JS-reachable write.- Buffer sizing:
H2PaddedFrameBuffer = [u8; 16384]matches the removedSHARED_REQUEST_BUFFER;payload_sizebounds at all three call sites are unchanged (capped byMAX_PAYLOAD_SIZE_WITHOUT_FRAME/get_padding), so no new panic surface. RareDataaddition mirrors the existingtemp_pipe_read_bufferpattern (Option<Box<[u8; N]>>,boxed_zeroed, auto-dropped);put_backkeeping the first-returned buffer is fine — the extra one just drops.- Three
unsafe { ptr::copy }blocks replaced with safecopy_from_slice;SHARED_REQUEST_BUFFERfully removed (no remaining references).
Extended reasoning...
Overview
The PR fixes a reproducible process abort (panic: RefCell already borrowed, exit 134) in node:http2 when a client uses paddingStrategy over a user-supplied JS Duplex transport whose _write re-enters the session by writing to another stream. Three padded-DATA write sites in h2_frame_parser.rs previously called writer.write_all(...) from inside SHARED_REQUEST_BUFFER.with_borrow_mut(...); when that write crossed the cork boundary and flushed into the JS transport, the nested send_data tried to borrow the same thread-local RefCell again and panicked.
The fix adds DirectWriterStruct::write_padded, which takes a 16 KiB scratch buffer by value from a new per-VM RareData::h2_padded_frame_buffer slot, assembles [pad-length | data | zero padding], calls write_all, then hands the buffer back. A re-entrant call finds the slot empty and allocates its own via boxed_zeroed. SHARED_REQUEST_BUFFER is removed along with three unsafe ptr::copy blocks. The stale "32 KB combined" comment on the thread_local! block is corrected. A subprocess regression test in node-http2.test.js reproduces the re-entry over a fake Duplex, asserts the exact byte totals node produces, and is gate-verified to fail before / pass after.
Security risks
None. This is internal buffer plumbing on an outbound write path; no untrusted-input parsing, auth, or crypto is touched. The buffer size and indexing bounds are identical to the code being replaced.
Level of scrutiny
Moderate. The change touches a native re-entrancy path where JS can run mid-write, which is exactly the class REVIEW.md warns about ("anything that can run user JS can synchronously free your state"). The fix applies the correct pattern for that class: move the shared state out by value before the call that can re-enter, so nothing is borrowed or aliased across it. The RareData placement follows the repo rule ("Per-VM state goes on VirtualMachine/RareData, never process globals or thread-locals") and mirrors the neighboring temp_pipe_read_buffer slot exactly — same Option<Box<[u8; N]>> shape, same boxed_zeroed lazy init, same automatic Drop.
I checked that the &mut RareData obtained via bun_vm().as_mut().rare_data() is a temporary that drops after take_h2_padded_frame_buffer() returns and before write_all runs, so the re-entrant call's own &mut RareData doesn't stack-borrows-alias. The put_back semantics (get_or_insert, drop the second buffer) are correct for the nested-return case and documented.
Other factors
- The gate output confirms fail-before (both release and debug+ASAN abort) and pass-after (311 pass / 0 fail on the full
node-http2.test.js). - All prior review threads are resolved: coderabbit's concern about tying
reenteredto the outer flush was addressed by thechunk.includes(0x41)guard; my three earlier notes (stale PT_TLS comment, PR-description sync, cross-site comment repetition) were fixed in 18b1b6a / be7f3b4; the comment-cop flags were either trimmed or correctly identified as collateral (pre-existing/shortened comments). - The
payload_sizecomputed inwrite_padded(1 + data.len() + padding) matches the caller-sidepayload_sizewritten into the frame header, and the 16384-byte buffer bound is the same as the removedSHARED_REQUEST_BUFFER— no behavior change on the wire beyond removing the abort. - No CODEOWNERS cover these paths; no human reviewer has requested changes.
2 participants
What
node:http2over a user-supplied Duplex transport (createConnection) withpaddingStrategyenabled: writing to a second stream from inside the transport's_writeaborts the process withpanic: RefCell already borrowed.Reproduction
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 printsno crash.Cause
send_data's padded single-frame branch (and the two padded branches inStream::flush_queue) built the payload insideSHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| { ...; writer.write_all(&buffer[..payload_size]) }), so thewrite_allran inside the borrow. When the frame crosses the cork boundary,write()->flush_cork_buffer()->_write()->onWriteruns the Duplex_write(user JS) with the thread-local still mutably borrowed; the nestedreq2.write()->send_databorrows 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_buffermove 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'sRareData(per review: a thread-local static is wrong with worker_threads; per-VM is the right scope).write_paddedtakes the buffer out of itsrare_dataslot by value for the duration of the write, so nothing is borrowed acrosswrite(): 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_BUFFERis removed, along with the threeunsafe { 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_writeduring 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.panic: RefCell already borrowed, test failsnode-http2.test.js: 311 pass, 6 skip, 0 failRelated: #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).
[review] gate passed · iteration 0 · 3 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