Repository navigation
node:http2: don't run an earlier write's callback mid-payload - #46823
dylan-conway wants to merge 2 commits into
Conversation
…laced it is queued A queued DATA frame holds one write callback. When a later write tops up that frame, the earlier callback is called at once. That happened in the middle of the walk over the later write's payload, which on a native socket is borrowed from the caller's ArrayBuffer. - A callback that called transfer() or resize() on that buffer made the remaining slices be copied from freed memory. - A callback that wrote to the same stream put its bytes in the middle of the payload. queue_frame() now returns the displaced callback. writeStream() and noTrailers() call it once the whole payload is queued and the stream borrow has ended. It is still called before they return.
|
Updated 6:55 PM PT - Oct 24th, 2026
❌ @claude, your commit 1324cde has 3 failures in
🧪 To try this PR locally: bunx bun-pr 46823That installs a local version of the PR into your bun-46823 --bun |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Queue displaced callbacks src/runtime/api/bun/h2_frame_parser.rs |
Stream::queue_frame returns displaced callbacks and preserves a queued frame’s END_STREAM flag when merging writes. send_data returns displaced callbacks with the settled stream state and deferred-callback flag. |
Dispatch callbacks and verify writes src/runtime/api/bun/h2_frame_parser.rs, test/js/node/http2/node-http2.test.js |
no_trailers and write_stream dispatch displaced callbacks after send processing. write_stream drops its payload, buffer, and guarded stream before dispatch. Tests check payload bytes, callback order, and END_STREAM placement. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
1324c
-
No actionable issue remains in the reviewed HTTP/2 callback and payload changes. The PR is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names the main change: a displaced write callback no longer runs in the middle of a payload in node:http2. It is short, specific, and matches the objectives and the file summaries, so it fit… Description check The description follows the template. 'What does this PR do?' explains the use-after-free, the fix in queue_frame, writeStream() and noTrailers(), and the END_STREAM placement change. 'How did you ver… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: a displaced write callback no longer runs in the middle of a payload in node:http2. It is short, specific, and matches the objectives and the file summaries, so it fits the changeset.
Full details: Description check
-
Explanation
The description follows the template. 'What does this PR do?' explains the use-after-free, the fix in queue_frame, writeStream() and noTrailers(), and the END_STREAM placement change. 'How did you verify your code works?' lists the new test, the Malloc=1 debug run, the existing http2 test directory run, and the 256 KB TCP comparison with Node. Both required sections are filled in with specific detail.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
This is a slow download on that agent, not something in this diff, so there is nothing to fix here. The other darwin aarch64 shard got its binary and is running. I can't retry BuildKite jobs from here. Could someone with access retry that one job? Generated by Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two things in the new displaced-callback plumbing: no_trailers dispatching while its &mut Stream binding is still in scope is not an aliasing problem in practice, since nothing touches that binding after send_data returns and the dispatch goes through enter_dispatch; and the chunked path keeping only the first slice's displaced value is sound on a native socket, because no JS runs between slices so later slices can only land on frames this write queued with an UNDEFINED callback.
Extended reasoning...
The change moves the displaced write-callback dispatch out of Stream::queue_frame into the two send_data callers in src/runtime/api/bun/h2_frame_parser.rs, deletes the black_box/raw-pointer scaffolding, and adds a native-parser test in test/js/node/http2/node-http2.test.js. It touches unsafe lifetime handling around a borrowed JS ArrayBuffer in the HTTP/2 writer, which is memory-safety-sensitive native code. One confirmed finding on END_STREAM being cleared by a re-entrant write from the displaced callback is posted inline, so a human look is already signalled; this note only records what else was examined.
A non-closing write that landed behind a queued closing write cleared the queued END_STREAM, or was queued after the frame that carried it. The peer then never saw the stream end, or got DATA after END_STREAM. Calling the displaced callback after the payload is queued made a write from that callback one such write. queue_frame() now moves the flag to whichever frame ends up last. Also drop the payload borrow before the displaced callback runs and mark SentData as must_use.
There was a problem hiding this comment.
I re-reviewed after 1324cde and didn't find any bugs; since this rewrites the re-entrancy handling in the native http2 frame parser and picks a semantics for writes that arrive behind a queued closing frame, a human look would still be worthwhile.
What was reviewed:
queue_framereturning the displaced callback: traced the cork/partial-cork/recursion paths — the old callback stays on its own frame when the new write spills into a new frame, and only the first queued slice insend_datacan displace a pre-existing callback, so laterdisplacedvalues are from this write's own frames (empty/undefined, andcall_write_callbackno-ops on non-callables).- Callers:
write_streamdropspayload/buffer/streambefore dispatching, andno_trailersdispatches aftersend_data; both are the only call sites ofqueue_frame/send_data. Callback order (displaced before the new write's callback) matches the old inline dispatch. - 1324cde (
end_stream |= take(last_frame.end_stream)): END_STREAM now migrates onto whatever frame ends up last, so a non-closing write made behind a queued closing write is sent as part of the stream rather than leaving it unended or being rejected — a design choice worth a maintainer's confirmation. The PR description's "Not changed: ... clears the queued END_STREAM" paragraph now describes the pre-1324cde9 behavior. - Test: six modes over a real TCP socket, awaits
dataSeen, asserts exact callback order, byte diff and END_STREAM-on-last-frame; covers both the UAF modes and the END_STREAM change.
Extended reasoning...
The change touches src/runtime/api/bun/h2_frame_parser.rs (Stream::queue_frame, H2FrameParser::send_data, write_stream, no_trailers) and adds one test in test/js/node/http2/node-http2.test.js. It removes the black_box/raw-pointer dispatch of a displaced write callback from inside queue_frame and defers that dispatch to the host-fn callers after all borrows end, fixing a use-after-free reachable by direct users of the native parser; the second commit moves a queued END_STREAM flag to the last queued frame, which addresses the inline finding from the prior review. No security-sensitive surface (auth, crypto, injection) is involved. The bug hunt ran dry and no CODEOWNER covers these paths, but the code is unsafe-adjacent native protocol code and the END_STREAM migration is a user-visible semantics choice, so a human confirmation is preferable to an automated approve.
|
Job: What failed on it, all 4 attempts each:
Why this is the agent and not this diff:
There is nothing to fix in this PR, and I can't retry BuildKite jobs from here. Could someone with access retry that job, ideally on another agent? The agent itself may need a look. The two Generated by Claude Code |
What does this PR do?
Fixes a use-after-free in the native
node:http2frame parser:writeStream()could run an earlier write's callback while it was still reading the caller'sArrayBuffer.A queued DATA frame holds one write callback. When a later write tops up that frame, the earlier callback is called at once. That happened inside
Stream::queue_frame, in the middle ofsend_data's walk over the payload, which on a native socket is borrowed rather than copied.transfer(0)orresize(0)on the buffer made the remaining slices be copied from freed memory.resize(0)segfaults on release 1.4.4.queue_framenow returns the displaced callback.writeStream()andnoTrailers()call it once the whole payload is queued and the&mut Streamborrow has ended, still before they return. No JS runs insidequeue_frameany more, so itsblack_boxand raw-pointer code is gone.node:http2keeps one write in flight per stream, so only direct use of the native parser reaches this.A queued END_STREAM now also stays on the last queued frame. On main, a non-closing write behind a queued closing write cleared the flag, or was sent as DATA after it. With the callback running later, a write made from it became such a write, so both are fixed here.
How did you verify your code works?
test/js/node/http2/node-http2.test.js(transfer, resize, overwrite, re-entrant write, write behind a closing write). It checks the bytes and END_STREAM flags the peer receives, and the callback order. It passes withbun bd testand fails on an unfixed debug build and withUSE_SYSTEM_BUN=1.Malloc=1, an unfixed debug build reportsheap-use-after-freeinVec::extend_from_sliceunderqueue_frame. The fixed build ends cleanly.test/js/node/http2/: no new failures against an unfixed build.Generated by Claude Code