FileReader: copy a chunk that does not fit the pull buffer instead of borrowing it past the read - #38969
FileReader: copy a chunk that does not fit the pull buffer instead of borrowing it past the read#38969robobun wants to merge 1 commit into
Conversation
… borrowing it past the read A read issued from on_pull can be nested inside another read loop's on_read_chunk dispatch; since the per-loop scratch buffer is only lent to the outermost loop, such a read collects its bytes in a heap buffer that PosixBufferedReader::read_with_fn frees right after delivering the final EOF chunk. FileReader kept a raw slice of that chunk (ReadDuringJSOnPullResult::Temporary) and copied it out only after the read returned, from freed memory. Chunks handed to on_read_chunk are only valid during the call, so the on_pull state machine now copies: a chunk that fits is still written straight into the pull buffer, anything else (and whatever was already written into the pull buffer) goes through FileReader::buffered. This also covers a second EOF chunk arriving in the same read, which used to be dropped or to discard the bytes already copied.
|
Warning Review limit reached
Next review available in: 4 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 (3)
Comment |
|
Status: fix is up for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the ReadDuringJSOnPullResult state machine in memory-safety-critical code (FileReader backs every piped stdio / process.stdin / Bun.file().stream() consumer), a maintainer look would still be worthwhile.
What was reviewed:
- Traced the
Js → UseBufferedtransition: bytes already copied into the pull buffer are prepended tobufferedbefore the chunk, so ordering is preserved and nothing already-delivered is dropped. - Checked that
bufcannot aliasself.bufferedon this path (on_pullassertsbufferedis empty before the read; chunks come from scratch orreader._buffer), so the removedis_slice_in_vec_capacityguard is not needed here. - Verified the dropped
retspecial-case forTemporaryonly affected the!has_more(EOF) branch, where the read loop is done regardless of the return value. - Test uses file-marker handshakes with deadlines instead of sleeps, drains all pipes concurrently, and asserts exact byte content — looks deterministic.
Extended reasoning...
Overview
The PR fixes a heap-use-after-free in FileReader::on_pull: when a nested read (triggered from inside a 'data' handler) delivered an EOF chunk larger than the pull buffer, the old code stored a raw slice of that chunk in ReadDuringJSOnPullResult::Temporary and copied it out after read() returned — but since #38656, nested reads use a heap Vec that is freed on the way out of read_with_fn, so the copy read freed memory. The fix removes the borrowed Temporary variant entirely: chunks that don't fit the pull buffer are copied into FileReader::buffered immediately (along with any bytes already in the pull buffer), and on_pull returns buffered as an owned result. The unused AmountRead variant and UseBuffered's size field are also removed. A doc comment on BufferedReaderParent::on_read_chunk now states the chunk-lifetime contract.
Security risks
None. This is an internal buffer-lifetime fix; no user-controlled input parsing, no auth/crypto/permissions.
Level of scrutiny
High. FileReader sits behind process.stdin, spawned stdio streams, and Bun.file().stream(). The change touches &'static mut lifetime-laundered buffers, nested JsCell::with_mut projections, and the highwater-mark backstop that governs whether the read loop continues. The reasoning in the PR description is thorough and the state machine is strictly simpler after the change (three variants, none borrowing), but the blast radius of a mistake here is every stream consumer.
Other factors
- The bug hunting system found no issues.
- I traced the aliasing question (whether
bufcan point intoself.bufferedon theread_inside_on_pullpath) and confirmed it cannot:on_pullassertsbufferedis empty before issuing the read, and chunks originate from the per-loop scratch orreader._buffer, neverFileReader::buffered. - The removed
retspecial-case forTemporaryonly mattered on the!has_morebranch, which is EOF — the read loop stops regardless of the return there. - The new test is well-constructed: file-marker handshakes with 15s deadlines instead of sleeps, exact byte-content assertions, concurrent pipe draining, and a documented ASAN repro on unfixed main. It's POSIX-only (
skipIf(!isPosix)), which matches the POSIX-specific synchronous-read path where the bug lives. - No prior human review comments to address.
Given the memory-safety stakes and hot-path surface area, deferring to a maintainer rather than auto-approving.
2 participants
Problem
test/js/node/test/parallel/test-http-chunk-problem.jsis red on main (build 97618, x64-asan):AddressSanitizer: heap-use-after-free ... READ of size 42880in the process that streamscat's stdout into an HTTP response. Same signature intest/regression/issue/09041.test.ts,spawn-stdin-readable-stream.test.ts("very large chunked data"),node-stream.test.js("process.stdin should pipe correctly"); on non-ASAN lanes it shows up as corrupt piped data (shell-pipe-read-fault.test.tsteedIsAllA: false, varying sha1 in the http test).StreamResult::to_js(src/runtime/webcore/streams.rs:785) copying the valueFileReader::on_pullreturned; the memory was freed byPosixBufferedReader::read_with_fn(src/io/PipeReader.rs:1336) on the way out of the read thaton_pullitself issued (src/runtime/webcore/FileReader.rs:1023). Full report under details.'data'handler, which runs inside the outer read loop'son_read_chunkdispatch. Since HTMLRewriter: don't read a streamed input ahead of its reader #38656 only the outermost loop may use the per-loop scratch buffer, so this nested read takesread_with_fn's_bufferpath: it collects the bytes in aVec, delivers them as the EOF chunk, and frees theVecbefore returning.FileReader::on_read_chunk, when an EOF chunk did not fit the pull buffer, stored a raw slice of the chunk (ReadDuringJSOnPullResult::Temporary) andon_pullcopied it out after the read returned. That was only ever safe because chunks used to come from the scratch buffer; PipeReader: don't re-deliver streamed bytes after a re-entrant read #38726 fixed the double delivery that the same re-routing caused inread_blocking_pipebut did not cover this.Fix
FileReader: theon_pullread state no longer borrows a chunk. A chunk that fits is still copied straight into the pull buffer (Js { buffer, filled }); anything else is copied intoFileReader::buffered, together with whatever had already been copied into the pull buffer, andon_pullhands outbuffered(UseBuffered).Temporaryand the never-producedAmountReadvariant are gone, and so is theretspecial case forTemporary.on_read_chunkreceives memory owned by the reader that is refilled (scratch) or freed/cleared (_bufferpaths, both POSIX loops and the Windows reader) as soon as the call returns, so the only thing a consumer may do with a chunk it needs later is copy it. Every otherBufferedReaderParentalready does (shell, subprocess, filter_run, FileResponseStream via uWS, Terminal, install); FileReader was the one exception. The copy costs nothing extra in practice:Temporarywas turned into a freshUint8Arrayby one memcpy,bufferedis handed to JS as an owned buffer after one memcpy. The new shape also keeps a second EOF chunk delivered in the same read (HUP drain of a blocking pipe, socket with more than the cutoff queued) instead of dropping it or discarding the bytes already copied into the pull buffer, which the old arm did.BufferedReaderParent::on_read_chunk.test/js/node/child_process/child_process.test.ts("child.stdout pull nested in a 'data' event") reproduces the exact ASAN report on unfixed main (READ of size 98304, freed atread_with_fn, allocated atread_with_fn'sreserve, fromFileReader::on_pull) and passes with the fix; the rest of that file passes (the two tests that time out here also time out without this change, they just spawn many ASAN children; the default-shell test needs$SHELL).bun bd test/js/node/test/parallel/test-http-chunk-problem.js3/3 green;process-stdin.test.ts,node-stream.test.js,09041.test.ts(all five runs correct, 78s under ASAN in this container),spawn-stdin-readable-stream.test.ts,html-rewriter.test.js,streams.test.js,spawn.test.ts,spawn-streaming-stdout.test.ts: 628 pass.Background
PosixBufferedReader(src/io/PipeReader.rs) reads a pipe/socket and calls its parent'son_read_chunk(chunk)for each piece. Streaming parents implementBufferedReaderParent;FileReaderis the parent behindBun.file().stream(),process.stdin, and spawned stdio streams._bufferVec instead and delivers slices of that Vec.on_pull(buffer)is the JS side asking for data, passing the typed array to fill. On POSIXon_pullruns the reader synchronously andon_read_chunkreports intoread_inside_on_pull;on_pullturns that into the pull result.FileReader::bufferedis the reader's own Vec for bytes read while no pull could take them.'data'handler, then_read) run inside the outer loop'son_read_chunk, and their next pull reads synchronously from there.ASAN report from the new test on unfixed main