Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls - #38886
Merged
Merged
Claude / Claude Code Review
completed
Aug 15, 2026 in 23m 5s
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/io/pipe_read_scratch.rs:27-46 |
Nested claim() forms aliased &mut PipeReadScratch while outer guard is live |
| 🟡 Nit | src/runtime/node/node_fs.rs:7124-7126 |
Stale comment references removed pipe_read_buffer |
| 🟡 Nit | src/io/pipe_read_scratch.rs:48-63 |
Safe Deref/DerefMut exposes uninitialized bytes as &[u8] |
Annotations
Check failure on line 46 in src/io/pipe_read_scratch.rs
claude / Claude Code Review
Nested claim() forms aliased &mut PipeReadScratch while outer guard is live
The nested-refusal path — the whole point of `state` — forms a fresh `&mut PipeReadScratch` (via `(*self.pipe_read_scratch()).claim()`) while the outer `PipeReadScratchGuard` still holds `&'a mut PipeReadScratch`, which is aliased-`&mut` UB under Stacked/Tree Borrows; on the Js path it also re-forms `&mut RareData`, of which the outer guard's borrow is a subobject. The old `thread_local! Cell<bool>` avoided this by keeping the flag off the buffer's borrow; the SAFETY comment on `claim_pipe_read_
Check warning on line 7126 in src/runtime/node/node_fs.rs
claude / Claude Code Review
Stale comment references removed pipe_read_buffer
Stale cross-reference: this comment still says "see `pipe_read_buffer` above", but this PR removes `RareData::pipe_read_buffer()` and the SAFETY block it pointed at. Update to `pipe_read_scratch` (or drop the pointer) so the comment doesn't send readers to a symbol that no longer exists.
Check warning on line 63 in src/io/pipe_read_scratch.rs
claude / Claude Code Review
Safe Deref/DerefMut exposes uninitialized bytes as &[u8]
The safe `Deref`/`DerefMut` on `PipeReadScratchGuard` hands out `&[u8]` over an uninitialized `Box::new_uninit_slice` allocation — the previous buffer was `boxed_zeroed`, so this is a soundness regression reachable from safe code (`PipeReadScratch::new().claim().unwrap()[0]`). The repo's own precedent for the identical cast, `bun_core::vec::spare_bytes_mut`, is deliberately `pub unsafe fn` with a documented write-only contract; either zero-init on first claim (`vec![0u8; N].into_boxed_slice()`)
Loading