PipeReader: keep the outer read loop's scratch claim when a nested read is refused - #38873
PipeReader: keep the outer read loop's scratch claim when a nested read is refused#38873robobun wants to merge 1 commit into
Conversation
…ad is refused ReadScratchClaim::try_claim built its result with bool::then_some(Self), whose argument is constructed before the condition is tested. On the refused path that value was dropped straight away, and its Drop cleared READ_SCRATCH_IN_USE, releasing the claim held by the outer read loop. So only the first nested read under an outer dispatch stayed out of the per-loop scratch buffer; every later one read into it, under the chunk the outer loop was still delivering out of it. Test the flag and set it explicitly instead. With every nested read now reading into its own buffer, size read_blocking_pipe's streamed reserve to a full default pipe buffer (64 KiB) so a consumer that re-pulls from inside its chunk handler keeps getting one chunk per pipe buffer instead of four.
|
Warning Review limit reached
Next review available in: 22 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)
Comment |
|
Status: superseded by #38886, which started from this branch (its two html-rewriter tests are carried over there), replaces the thread-local flag with a claim owned by the scratch buffer itself, and also covers |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the try_claim() fix is a provably-correct rewrite of the then_some(Self) footgun, and the streaming reserve bump is well-measured.
Checked: the refused path now constructs no ReadScratchClaim value, so Drop cannot clear READ_SCRATCH_IN_USE and release the outer loop's claim; read_with_fn's 16 KiB reserve is correctly left alone (it accumulates across reads, unlike read_blocking_pipe's per-read deliver-and-clear); the two new tests use distinguishable byte content ('a' vs 'b') so scratch aliasing shows up as wrong bytes in outer.text(), and cover both read loops (regular file → read_with_fn, stdin pipe → read_blocking_pipe).
Extended reasoning...
Overview
Two changes to src/io/PipeReader.rs plus two regression tests in test/js/workerd/html-rewriter.test.js:
ReadScratchClaim::try_claim()is rewritten from(!in_use.replace(true)).then_some(Self)to an explicit test-then-set. The original was a Rust footgun:then_someevaluates its argument eagerly, so aReadScratchClaim(a ZST with aDropthat clears the thread-local flag) was constructed unconditionally and dropped on the refused path — releasing the outer read loop's claim. The rewrite constructsSelfonly after the flag is confirmed unset, so no guard exists to drop on theNonepath.- In
read_blocking_pipe's no-scratch-claim branch, the streaming-mode reserve is bumped from 16 KiB to 64 KiB. Because streaming mode delivers-and-clears per read, the reserve is the effective chunk size; with the claim now correctly refused for every nested read, a re-pulling consumer (process.stdin.on('data')) would otherwise see 4× the chunk count. 64 KiB matches the default pipe buffer on Linux/macOS and restores the pre-#38656 chunking (256 × 64 KiB for a 16 MiB stream, per the PR's measurements). The non-streaming (accumulating) reserve is unchanged.
Security risks
None. This is single-threaded scratch-buffer bookkeeping — a thread-local Cell<bool> guarding which buffer a synchronous nested read loop reads into. No untrusted input parsing, no auth/crypto, no cross-thread state. The bug it fixes was a data-aliasing corruption (nested read overwriting the outer loop's chunk-in-progress), and the fix strictly narrows when the shared scratch is used.
Level of scrutiny
Medium-high — PipeReader.rs is a core POSIX I/O path (every pipe/file/stdin stream). But the change surface is tiny and mechanically verifiable: the try_claim() rewrite is an explicit expansion of what the one-liner was meant to do, and reading impl Drop for ReadScratchClaim (line 177-181) confirms exactly why constructing-then-dropping on the refused path was wrong. I checked read_with_fn (line 1311) still uses a 16 KiB reserve and confirmed the PR's rationale: that loop accumulates across reads until EAGAIN so the reserve is only the initial allocation, not the chunk size — no change needed there.
Other factors
- Tests are placed alongside the existing #38656 test in the same describe block, use a second file with 'b' bytes vs the outer's 'a' bytes so corruption is directly observable in the outer output, and cover both read loops. The stdin variant spawns a child, drains all three pipes concurrently, and asserts stderr/stdout/exitCode in the right order.
- The PR description includes a chunking measurement table proving the reserve bump exactly restores release-build behavior, and a list of the ~12 related test files run on the debug build.
MaxBuf::clamp_read_bufstill clamps the read buffer after the reserve, so the 64 KiB bump doesn't defeat maxbuf enforcement.- The bug-hunting pass found nothing.
3 participants
Problem
new HTMLRewriter().transform(new Response(Bun.file(other))).text()makes the outer.text()come back as<b x="1">bbbb...(the other file's content) instead of the outer document. Same result whether the outer document comes from a regular file or from a stdin pipe.ReadScratchClaim(src/io/PipeReader.rs:161) so that only the outermost read loop on the thread reads into the per-loop scratch buffer, and nested loops read into their own_buffer.try_claim()was written as(!in_use.replace(true)).then_some(Self).bool::then_sometakes its argument by value, so aReadScratchClaimis constructed before the condition is tested; on the refused paththen_somedrops it, andimpl Drop for ReadScratchClaimclearsREAD_SCRATCH_IN_USE. The first refused nested read therefore released the outer loop's claim, and every nested read after it claimed the scratch and refilled it under the outer loop's chunk.transform()+.text()inside a handler is exactly two nested reads (the initial readtransform()performs, then the drain.text()triggers), so the case HTMLRewriter: don't read a streamed input ahead of its reader #38656 was written for was still broken in its most natural form. The existing test only consumed a transform created outside the handler, which is a single nested read, so it kept passing.Fix
try_claim()tests the flag and sets it explicitly, so no claim value exists on the refused path and the outer loop's claim stays set until its own claim drops.on_read_chunkpoints into the scratch until that dispatch returns, and a nested loop of any depth or count during that window must stay out of it. Releasing it early is exactly the aliasing the claim exists to prevent._buffer,read_blocking_pipe's streamed branch reserves 64 KiB instead of 16 KiB. In streaming mode that branch delivers and clears per read, so the reserve is the chunk size, and a consumer that re-pulls from inside its chunk handler (process.stdin.on("data")on a piped stdin) takes every read after the first through it. 16 KiB would have turned one chunk per pipe buffer into four (measurements below); 64 KiB is the default pipe buffer on Linux and macOS and keeps the chunking identical to the current release. The non-streaming (accumulating) reserve is unchanged.read_with_fn) and from the child's stdin pipe (read_blocking_pipe), each with a handler that transforms and reads a second file whose bytes differ from the outer document's. Both fail on main with the outer output replaced by the other file's bytes, both pass with this change.process.stdoutalone takes 1.7 s in this debug build) and shell-blocking-pipe'sgenerateHeapSnapshottest (the snapshot alone exceeds the 5 s timeout here); both reproduce without this diff's code paths being involved.Background
PipeReadBuffer. The POSIX read loops in src/io/PipeReader.rs (read_with_fnfor files, sockets and non-blocking pipes;read_blocking_pipefor blocking pipes such as a piped stdin) read into it and pass the filled slice straight to the parent'son_read_chunk, so a streamed chunk lives in the scratch until that callback returns.on_read_chunkcan run user JS. FileReader's native-sink path hands the slice to HTMLRewriter, which runs content handlers while lol_html is still parsing out of it; a handler that starts or drains another streamed body runs another read loop synchronously, inside the outer one's dispatch. FileReader's JS-facing path copies the chunk before JS runs, which is whyprocess.stdinconsumers see no corruption, only the chunking difference above.ReadScratchClaim: a thread-local flag plus a Drop guard. A read loop that gets the claim uses the scratch; one that is refused reads into the reader's own_buffer(16 KiB reserve inread_with_fn, which grows as it drains to EAGAIN; per-read delivery inread_blocking_pipe, so the reserve there is the chunk size).Chunking measurements: 16 MiB piped into `process.stdin.on("data")`, 64 KiB pipes
try_claimfix aloneThe 9 x 16 KiB chunks on main are the one correctly refused nested read per outer dispatch; everything after it went back into the scratch.
read_with_fnconsumers (a child's stdout via child_processdataevents, same 16 MiB) deliver 76 chunks of ~220 KiB with this PR against 127 on the current release, so no reserve change is needed there.Note for anyone reproducing in a container as root: uid 0 can end up with 8 KiB pipes there (pipe-user-pages-soft), which hides the chunking difference; run the pipeline as another uid.