Bun.stdin: reject text/arrayBuffer/bytes/json when process.stdin already holds the reader - #35831
Bun.stdin: reject text/arrayBuffer/bytes/json when process.stdin already holds the reader#35831robobun wants to merge 5 commits into
Conversation
process.stdin is implemented on top of Bun.stdin.stream()'s reader. The
Blob read helpers on an fd-backed Blob (text/json/arrayBuffer/bytes) went
straight to do_read_file, which issues raw read(2) on fd 0, so a program
that had process.stdin.on('data') armed and also awaited
Bun.stdin.arrayBuffer() would see the piped input silently split between
the two consumers with no error.
.stream() already de-duplicates by caching the ReadableStream on the JS
Blob wrapper for fd-backed stores. Make the Blob read helpers consult that
slot first and, when a cached stream is present, consume it via the
existing readableStreamTo* path. A second consumer now rejects with
ERR_INVALID_STATE (ReadableStream is locked), matching
Bun.stdin.stream().getReader().
|
Warning Review limit reached
Next review available in: 10 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 |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/webcore/Blob.rs:1298-1306—get_form_data(immediately below) is the fifth Blob read helper following the sameneeds_to_read_file() → do_read_filepattern, soBun.stdin.formData()still reads fd 0 directly and racesprocess.stdin— the bytes are stolen before the encoding check runs.global_this.readable_stream_to_form_data(stream, content_type)already exists (used in Body.rs), so the samefd_cached_streamguard applies here too; per REVIEW.md's "fix the whole class" rule it's worth including (or noting the exclusion in the PR description).Extended reasoning...
What the bug is
The PR routes
get_text/get_json/get_array_buffer/get_bytesthrough the cached-stream slot so a second consumer onBun.stdinrejects withERR_INVALID_STATEinstead of silently splitting bytes withprocess.stdin. However the sibling methodget_form_dataat Blob.rs:1307 — declared immediately afterget_bytesin the trait — is left unchanged. It callsto_form_data(), which at Blob.rs:3242-3243 checksneeds_to_read_file()and goes straight todo_read_file::<ToFormDataWithBytesFn>(global), issuing rawread(2)on fd 0 exactly like the four fixed methods used to.The code path
Bun.stdin.formData() → get_form_data (Blob.rs:1307) — no fd_cached_stream() check → to_form_data (Blob.rs:3237) → needs_to_read_file() == true — Bun.stdin is an fd-backed Blob → do_read_file::<ToFormDataWithBytesFn> — schedules ReadFile on the work pool → bun_sys::read(fd 0, ...) — races process.stdin's FileReader → to_form_data_with_bytes (Blob.rs:2969) → get_form_data_encoding() — encoding checked only AFTER the readWhy existing code doesn't prevent it
One might expect this not to matter because
Bun.stdinhas no content-type, soformData()will reject with "Invalid encoding" anyway. But that check lives into_form_data_with_bytes(Blob.rs:2974), which runs afterdo_read_filehas already drained fd 0. So even though theformData()promise ultimately rejects,process.stdinhas still silently lost whatever bytes theReadFiletask consumed — the exact failure mode this PR fixes for the other four helpers.Step-by-step proof
process.stdin.on('data', c => n += c.length)— arms aFileReaderonBun.stdin.stream()and caches the stream in the wrapper's slot.Bun.stdin.formData()— entersget_form_data, which does not consultfd_cached_stream(callframe.this()).to_form_data()seesneeds_to_read_file()is true (fd-backed store) and callsdo_read_file, scheduling aReadFiletask that callsbun_sys::read(0, ...)on the work pool.- Both the work-pool
ReadFileandprocess.stdin'sFileReadernow issue reads against fd 0; piped input is split between them nondeterministically. - Once the
ReadFilecompletes,to_form_data_with_bytescallsget_form_data_encoding(), getsNone(no content-type on stdin), and rejects with "Invalid encoding" — but the bytes have already been stolen fromprocess.stdin.
Impact
process.stdin.on('data')armed alongsideawait Bun.stdin.formData()still silently loses bytes with no error. This is admittedly a niche scenario — piping multipart/form-data to stdin while also consumingprocess.stdinis far less plausible than the four fixed methods — hence nit severity rather than blocking.How to fix
The machinery already exists:
readable_stream_to_form_data(stream, content_type)is used by Body.rs:401. Add the same guard toget_form_data:fn get_form_data(&self, global_this: &JSGlobalObject, callframe: &CallFrame) -> JsResult<JSValue> { if let Some(stream) = self.fd_cached_stream(callframe.this()) { let content_type = /* JSValue derived from self.get_content_type() / get_form_data_encoding() */; return bun_jsc::from_js_host_call(global_this, || { global_this.readable_stream_to_form_data(stream, content_type) }); } ... }
Alternatively, if
formData()is being intentionally excluded (e.g. because the content-type plumbing makes it awkward and the trigger is implausible), REVIEW.md asks that the PR say so explicitly: "If a site is intentionally excluded, say so in the PR." The current description enumerates text/json/arrayBuffer/bytes and listsBun.file(0)under "Not covered", but doesn't mentionformData().
BuildArtifact forwards its callframe to the inner Blob's get_text/ get_json/get_array_buffer, so callframe.this() can be a JSBuildArtifact whose inner blob is fd-backed (OutputFileValue::Copy with a valid fd). JSBlob's stream_get_cached does an uncheckedDowncast<JSBlob>; reading it with a non-JSBlob receiver is a debug ASSERT and a wrong-offset read in release. Check js::from_js first so non-JSBlob receivers fall through to the existing do_read_file path.
|
Superseded by #35833, which also covers two concurrent |
| fn get_bytes(&self, global_this: &JSGlobalObject, callframe: &CallFrame) -> JsResult<JSValue> { | ||
| if let Some(stream) = self.fd_cached_stream(callframe.this()) { | ||
| return bun_jsc::from_js_host_call(global_this, || { | ||
| global_this.readable_stream_to_bytes(stream) | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔴 get_form_data (the fifth sibling, immediately below) still bypasses fd_cached_stream and goes to_form_data → needs_to_read_file() → do_read_file::<ToFormDataWithBytesFn> (Blob.rs:3236-3237), so Bun.stdin.formData() still races a raw read(2) on fd 0 against process.stdin — the exact defect this PR fixes for the other four. formData is on Blob's prototype (response.classes.ts:164) and readable_stream_to_form_data already exists (JSGlobalObject.rs:1188, used by Body.rs:401), so the same-shape fix applies; REVIEW.md makes sibling coverage required scope ("Fix the whole class in the same PR … Grep for every sibling site sharing the pattern").
Extended reasoning...
What the bug is
The PR routes get_text, get_json, get_array_buffer, and get_bytes through the new fd_cached_stream helper so that when process.stdin (built on Bun.stdin.stream()'s cached reader) already holds fd 0, they reject with ERR_INVALID_STATE instead of silently splitting bytes. But the fifth sibling read helper, get_form_data at Blob.rs:1301-1305 — sitting immediately after get_bytes in the same impl block — was not updated. It still calls self.to_form_data(...), which at Blob.rs:3236-3237 does:
if self.needs_to_read_file() {
return Ok(self.do_read_file::<ToFormDataWithBytesFn>(global));
}That is the identical raw-read(2)-on-fd-0 code path this PR set out to eliminate.
The code path that triggers it
Bun.stdin is an fd-backed Blob (PathOrFileDescriptor::Fd(0)), so needs_to_read_file() is true. formData is exposed on the Blob prototype at response.classes.ts:164 (formData: { fn: "getFormData", async: true }), so Bun.stdin.formData() is user-reachable. When a program has process.stdin.on('data', ...) armed and then awaits Bun.stdin.formData(), do_read_file schedules a ReadFile task that calls bun_sys::read(fd 0, ...) on the work pool, racing with process.stdin's FileReader — the exact silent-split defect described in the PR body.
Why existing code doesn't prevent it
The "stdin has no content-type, so formData() will reject anyway" objection doesn't help: to_form_data calls do_read_file before any encoding check — the content-type validation happens inside to_form_data_with_bytes, which runs after the file has been fully read. So even though the eventual FormData parse may reject, the raw read(2) has already consumed bytes from fd 0 and stolen them from process.stdin's reader. The user still sees their piped input silently split, plus an unrelated-looking encoding error.
Impact
Same class of bug the PR exists to fix, on the one sibling method it skipped. Per REVIEW.md this is required scope, not scope creep:
Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern … If a site is intentionally excluded, say so in the PR.
The PR description does not mention excluding formData. The trait signature at Blob.rs:229 (fn get_form_data(&self, global_this: &JSGlobalObject, _: &CallFrame)) even still has the anonymous _ for the callframe, while the other four were renamed to callframe in this diff — a visible sibling-missed marker.
Step-by-step proof
process.stdin.on('data', ...)— creates the readable-flowing wrapper, which callsBun.stdin.stream().getReader()and populates JSBlob's cachedm_streamslot; the stream is now locked.Bun.stdin.formData()→ generatedgetFormData→Blob::get_form_data(self, global, callframe)at Blob.rs:1301.- No
fd_cached_streamcheck — goes straight toself.to_form_data(g, Lifetime::Temporary). to_form_dataat Blob.rs:3236:self.needs_to_read_file()istrue(fd-backed store) →self.do_read_file::<ToFormDataWithBytesFn>(global).do_read_fileschedules aReadFiletask on the work pool that loopsbun_sys::read(fd 0, buf)until EOF.- Meanwhile
process.stdin'sFileReaderis also reading fd 0 on the event loop. The kernel hands each byte to whicheverread(2)arrives first — non-deterministic split, exactly what the PR body's repro shows forarrayBuffer(). - After the read completes,
to_form_data_with_bytesruns and (with no content-type) rejects — but the bytes are already gone fromprocess.stdin's view.
How to fix
Same shape as the other four. readable_stream_to_form_data already exists at JSGlobalObject.rs:1188 and is already used by Body.rs:400-401 for the Request/Response path:
fn get_form_data(&self, global_this: &JSGlobalObject, callframe: &CallFrame) -> JsResult<JSValue> {
if let Some(stream) = self.fd_cached_stream(callframe.this()) {
let content_type = self
.get_form_data_encoding()
.map(|fd| fd.encoding.to_js(global_this))
.unwrap_or(JSValue::undefined());
return bun_jsc::from_js_host_call(global_this, || {
global_this.readable_stream_to_form_data(stream, content_type)
});
}
let _store = self.store.get().clone();
Ok(JSPromise::wrap(global_this, |g| {
self.to_form_data(g, Lifetime::Temporary)
})?)
}(and add "formData" to the describe.each matrix in bun-stdin-locked.test.ts — REVIEW.md: "Cover the variant matrix … every sibling entry point receiving the same fix".)
Note this shares the same callframe.this()-receiver hazard already flagged for the other four (the JSBuildArtifact comment on line 1242) — whatever fix is applied there should apply here too.
Problem
process.stdinis built onBun.stdin.stream()'s reader. The Blob read helpers onBun.stdin(text(),json(),arrayBuffer(),bytes()) bypassed that stream and issued rawread(2)on fd 0 viado_read_file, so a program that hadprocess.stdin.on('data')armed and also awaitedBun.stdin.arrayBuffer()would see the piped input silently split between the two consumers with no error:Bun.stdin.stream().getReader()in the same position already rejected withERR_INVALID_STATE: ReadableStream is locked; the Blob read helpers did not.Cause
.stream()on an fd-backed Blob caches theReadableStreamon the JS wrapper so repeated calls return the same stream (and a secondgetReader()throws).get_text/get_json/get_array_buffer/get_bytesnever consulted that cached slot; they checkedneeds_to_read_file()and went straight todo_read_file, which schedules aReadFiletask that callsbun_sys::read(fd 0, ...)on the work pool, racing withprocess.stdin'sFileReader.Fix
For fd-backed Blobs (the same condition under which
.stream()caches), the four read helpers now check the cached stream slot first. When a cached stream exists they route throughreadableStreamTo{Text,JSON,ArrayBuffer,Bytes}instead ofdo_read_file, so a concurrent consumer rejects withERR_INVALID_STATErather than stealing bytes.When no stream has been materialised yet (the common
await Bun.stdin.text()on its own), the existingdo_read_filefast path is unchanged.The check is gated on
JSBlob::from_js(this)so other classes that delegate to an inner Blob (e.g.BuildArtifact, whose fd-backedOutputFileValue::Copyblobs reach the same methods with aJSBuildArtifactreceiver) fall through to the existing path instead of hitting JSBlob'suncheckedDowncastslot accessor.Not covered
Bun.file(0)returns a fresh Blob wrapper with its own stream slot, soBun.file(0).arrayBuffer()alongsideprocess.stdinstill reads fd 0 independently.Bun.file(0).stream()has the same issue (twoFileReaders on fd 0 can hang). Fixing that needs the stream cache to be shared across wrappers, which is a larger change.Bun.stdin.formData()is left on thedo_read_filepath. It would still drain fd 0 before rejecting on the missing content-type, but piping multipart to stdin while also consumingprocess.stdinis implausible and thereadable_stream_to_form_datacontent-type plumbing is not worth the extra surface here.Verification
New
test/js/bun/util/bun-stdin-locked.test.tscovers all four methods plus the standalone path;bun-stdin-slice.test.ts,bun-build-api.test.tsand theBun.stdinregression tests (07500,27849,29787) still pass.