Repository navigation
io: fix WindowsBufferedReader.deinit source handling + ReadableStreamSource Strong cycle - #29440
dylan-conway wants to merge 6 commits into
Conversation
|
Updated 3:38 AM PT - May 4th, 2026
@dylan-conway, your commit fab3241 is building: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReordered Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the two-line reorder is clearly correct: reads internally and nulls it itself, so the premature null was rendering the entire cleanup a no-op.
Extended reasoning...
Overview
Single-file change () in . The fix reorders two lines so happens after rather than before it.
Security Risks
None. This is a resource cleanup fix (fd/handle leak), not security-sensitive code.
Level of Scrutiny
Low. The diff is two lines swapped, the description fully explains the causal chain, and is readable in the same file confirming it checks itself and nulls it on the way out. The trailing after the correctly handles the already-closed early-out path that does not cover.
Other Factors
The PR description explains the regression risk thoroughly: skips vtable callbacks, and each source's close path detaches from the reader before scheduling async work, so there is no use-after-free risk. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — simple one-line reorder in deinit plus a clean gating fix in the lifecycle script runner.
Extended reasoning...
Overview
Two files changed: src/io/PipeReader.zig (1 line moved) and src/install/lifecycle_script_runner.zig (condition extracted + pipe allocation gated). Both touch Windows-only code paths.
Security risks
None. These are resource-cleanup and lifecycle fixes with no auth, crypto, or input-handling components.
Level of scrutiny
Low-to-medium. The core fix (this.source = null moved to after closeImpl(false)) is a single-line reorder that restores the original semantics — closeImpl already nulls the field itself, so the trailing assignment only serves the isClosed() early-out branch. The logic is easy to verify against the closeImpl body. The lifecycle runner change extracts a boolean that was already implicit in the three branches and uses it consistently; it is a refactor and latent-bug fix rolled together.
Other factors
No bugs were found by automated analysis. The PR description includes a detailed regression-risk analysis covering all call sites that set source directly. The change is Windows-only so Posix paths are unaffected.
| MaxBuf.removeFromPipereader(&this.maxbuf); | ||
| this.buffer().deinit(); | ||
| const source = this.source orelse return; | ||
| this.source = null; | ||
| if (!source.isClosed()) { | ||
| // closeImpl will take care of freeing the source | ||
| this.closeImpl(false); | ||
| } | ||
| this.source = null; | ||
| } | ||
|
|
||
| pub fn setRawMode(this: *WindowsBufferedReader, value: bool) bun.sys.Maybe(void) { |
There was a problem hiding this comment.
🟣 This is a pre-existing issue: in WindowsBufferedReader.deinit(), buffer().deinit() frees _buffer before closeImpl(false) runs file.detach()/uv_cancel(), leaving a theoretical window where an in-flight uv_fs_read thread-pool worker could write into freed memory. This PR does not introduce or worsen the race — the old code was strictly worse (closeImpl was a complete no-op due to the earlier this.source = null assignment). All current callers that use file sources either assert remaining_fds == 0 before deinit or call close() first, so the vulnerable code path is not reachable in practice.
Extended reasoning...
What the bug is and how it manifests
In WindowsBufferedReader.deinit() (PipeReader.zig ~line 938), this.buffer().deinit() is called first, freeing the heap memory backing _buffer. Then, if the source is not yet closed, closeImpl(false) is called, which for .file sources calls file.detach() → file.stop() → uv_cancel(). The libuv uv_fs_read API uses a thread-pool worker that calls ReadFile directly into file.iov.base, which points into the already-freed _buffer.allocatedSlice(). If the thread-pool worker is actively writing when deinit() frees the buffer, this is a write to freed memory (heap corruption / UAF).
The specific code path that triggers it
For a .file or .sync_file source with an in-flight uv_fs_read:
- Caller calls
deinit() this.buffer().deinit()→ frees_bufferheap allocationcloseImpl(false)→file.detach()→file.stop()→uv_cancel()- If
uv_cancelreturnsUV_EBUSY(I/O already submitted to kernel), the OSReadFilecompletes and writes into the freed memory region onFileReadfires, seesparent_ptr == null(detach setfs.data = null), and returns safely — but the buffer corruption already occurred at step 4
Why existing code doesn't prevent it
uv_cancel is not guaranteed to succeed for I/O that is already in-flight with the kernel. After uv_cancel returns, the thread-pool worker may still be in the middle of ReadFile, writing into the freed buffer memory.
Why this is pre-existing and not introduced by this PR
Before this PR, this.source = null was assigned BEFORE the closeImpl call, making closeImpl a complete no-op (it checks if (this.source) |source| immediately). This was strictly worse: the buffer was freed AND the in-flight read was never even attempted to be cancelled. Additionally, onFileRead would fire with a non-null parent_ptr pointing to potentially freed/reused struct memory — a UAF of the struct itself. The PR's fix makes closeImpl actually run and adds file.detach() which sets fs.data = null so onFileRead returns early without touching this, improving safety even though the buffer-memory ordering concern remains.
Impact and current exploitability
In practice, this is not reachable through any current caller. The lifecycle script runner uses .pipe sources on Windows (not .file) when buffer_output is true, so the uv_fs_read path is never taken. Additionally, resetPolls() asserts remaining_fds == 0 before calling deinit(), guaranteeing all reads have completed. Other callers either assert the source is closed/null before deinit, or call close() first which properly defers cleanup via the defer_done_callback mechanism.
How to fix it
Move buffer().deinit() to after closeImpl() completes — or defer the _buffer free to the onFileRead callback after cancel completes (similar to how defer_done_callback defers the done() call). A simpler short-term fix is to document the ordering invariant: callers must ensure no in-flight reads exist before calling deinit(), which is already enforced by the assertions in current callers.
|
Traced the review comment on
Opened #29472 to fix the Strong cycle and add the missing Windows non-lazy ref, since fixing the cycle alone exposes the UAF this comment describes (verified on Windows: |
…#29472) ## Summary Three related fixes; the second and third are required by the first. ### 1. `close_jsvalue` Strong → `onCloseCallback` cached slot (cross-platform) `setOnCloseFromJS` stored the callback in a `jsc.Strong`, which forms a rooted cycle: source-wrapper → `close_jsvalue` Strong → bound `#onClose` → `NativeReadableStreamSource` (`ReadableStreamInternals.ts:1972`) → `$stream` private prop (`:1959`) → source-wrapper. Because a Strong is a global GC root, the source survives even after every JS reference (including the outer `ReadableStream`) is dropped. It only becomes collectable when EOF/close runs the JS-side `callClose` (which clears `$stream`) or at VM shutdown. The codegen already declares `onCloseCallback` in `streams.classes.ts` `values`; `onDrain` already uses its cached slot. Switch `onClose` to the same `WriteBarrier`-backed storage and delete the Strong field. The cycle becomes an ordinary intra-heap cycle that mark-sweep collects. ### 2. Windows non-lazy `FileReader` across-read ref `FileReader.onStart` holds an `incrementCount()` until `onReaderDone` only on the lazy path (always) or the POSIX non-lazy path. The Windows non-lazy path — `fromPipe`, reached via `Bun.spawn().stdout`/`.stderr` — did not. With the cycle fix above, the source is now collectable while a `uv_read_start` IOCP read is pending, and `WindowsBufferedReader.deinit` would run with a live `.pipe` source whose `data` ptr is then dereferenced by the queued `onStreamRead`. Add a Windows arm matching the POSIX one. ### 3. Release the across-read ref in `onReaderError` too `onReaderDone` checks `waiting_for_onReaderDone` and decrements; `onReaderError` did not, so a read that ends in error (rather than EOF) leaked the ref taken in `onStart`. Pre-existing on the lazy and POSIX paths; commit 2 adds a Windows arm that would inherit the same gap. Mirror the release after `pending.run()`. ## Verification On Windows, with a child that spawns a detached grandchild inheriting stdout (so the pipe stays open after the direct child exits), repeatedly accessing `proc.stdout`, dropping it, and forcing GC: | | `*ReadableStreamSource` heap count after 30 iters | `WindowsBufferedReader.deinit` reached with live `.pipe` source | |---|---|---| | baseline | 31 (linear growth; 61 at 60 iters) | no — leak masks it | | commit 1 only | ~14 (plateaus at live-grandchild count) | **yes** — `FileReader.deinit` sees `src=pipe`, `closed=false` | | commits 1–3 | ~15 (plateaus; freed as pipes EOF; flat through 80 iters) | no | ## Relation to #29440 Found while verifying the review comment on #29440 about `WindowsBufferedReader.deinit` ordering. That comment correctly identified the buffer-free-before-detach as theoretical; this PR explains why (the Strong cycle pinned the source) and fixes the underlying leak plus the UAF that fixing the leak would have exposed.
dylan-conway
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/io/source.zig`:
- Around line 124-128: The synchronous destroy path in src/io/source.zig frees
the File struct inline when close_fd == false (the branch that calls
bun.default_allocator.destroy(this)), which causes a UAF in
PipeReader.onFileRead because onFileRead still dereferences `file` after
`file.complete()`; change the logic so destruction is deferred: either 1) in
onFileRead capture the no-close case (e.g., read and stash close_fd or a
"no_destroy" flag) before calling file.complete() and avoid any accesses after
complete(), or 2) change the close_fd == false branch in
file.complete()/source.zig to schedule destruction (enqueue a small
callback/mark-for-destruction) instead of calling
bun.default_allocator.destroy(this) synchronously; update references to
`file.complete()`, `close_fd`, and `bun.default_allocator.destroy(this)` to
ensure onFileRead never touches freed memory.
- Around line 56-58: openFile() currently initializes Source.File with
std.mem.zeroes(Source.File) which wipes field defaults (notably close_fd: bool =
true) causing close_fd to become false and file descriptors to leak when
startClose()/detach() skip uv_fs_close; change openFile() to initialize the
struct with an explicit struct literal (e.g. Source.File{ .fieldA = ..., .fieldB
= ..., /* omit close_fd to keep default true */ }) instead of std.mem.zeroes so
close_fd retains its default true and fd cleanup via startClose()/uv_fs_close
occurs correctly (affects PipeWriter when owns_fd=true and interactions with
BufferedReader.flags.close_handle).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7dd8c0b6-6056-40ac-88bc-700255d640c6
📒 Files selected for processing (2)
src/io/PipeReader.zigsrc/io/source.zig
| // (or just free the struct if the caller owns the fd). | ||
| file.close_fd = this.flags.close_handle; | ||
| file.detach(); |
There was a problem hiding this comment.
🟣 🟣 Pre-existing (not touched by this PR): the parallel writer path in WindowsPipeWriter.close() (src/io/PipeWriter.zig:829-835) leaks the heap Source.File when owns_fd=false — it does file.stop(); file.fs.data = null; without ever reaching finalize(), so the struct allocated in openFile() is never freed (reachable via shell/IOWriter.zig:26 on Windows). The close_fd mechanism added here is exactly what would fix it (file.close_fd = false; file.detach();), so it may be worth applying symmetrically while you're in this area.
Extended reasoning...
What the bug is
This PR adds Source.File.close_fd and threads it through WindowsBufferedReader.closeImpl (PipeReader.zig:1158-1160) so that when the caller owns the fd, file.detach() → finalize() frees the heap struct without calling uv_fs_close. The parallel code path in WindowsPipeWriter.close() was not updated and still leaks the heap Source.File struct in the !owns_fd branch:
// src/io/PipeWriter.zig:826-835
switch (source) {
.sync_file, .file => |file| {
if (this.owns_fd) {
file.detach();
} else {
// Don't own fd, just stop operations and detach parent
file.stop();
file.fs.data = null; // <-- struct never freed
}
},The specific code path that triggers it
shell/IOWriter.zig:26 sets .owns_fd = false on Windows. When such a writer's source is a .file/.sync_file (heap-allocated via bun.default_allocator.create in Source.openFile()), close() enters the else branch above.
Why existing code doesn't free it — step-by-step proof
close()is called withowns_fd == falseandsource == .file.file.stop(): if a write is in flight (state == .operating), attemptsuv_canceland may setstate = .canceling. It does not setclose_after_operation. If no op is in flight (state == .deinitialized),stop()is a no-op.file.fs.data = nullandthis.source = null. The writer drops its only pointer to the struct.
4a. No op in flight: nothing else ever touches thisFile*. Leaked outright.
4b. Op in flight:onFsWriteCompletefires (PipeWriter.zig:1025-1037 / 1355-1368). It readsparent_ptr = fs.data(now null), callsfile.complete(was_canceled). Insidecomplete()(source.zig:108-123),close_after_operationis stillfalse(onlydetach()sets it), so it doesfs.deinit(); state = .deinitialized;and returns without callingfinalize(). Back inonFsWriteComplete,parent_ptr == null→return. The struct is leaked.
In neither case does any path reach bun.default_allocator.destroy(file) or onCloseComplete.
Why this is pre-existing and unrelated to this PR
src/io/PipeWriter.zig is not in the diff. The PR adds no new callers of this path and does not change its reachability. The new close_fd/finalize() machinery has zero effect on the writer's !owns_fd branch because that branch never invokes detach() or finalize(). The leak existed before this PR and behaves identically after it.
Impact
A small (@sizeOf(Source.File), dominated by uv.fs_t) heap leak per shell IOWriter whose underlying handle is a regular file on Windows. Not a correctness issue — the fd itself is not leaked (the caller closes it).
How to fix
Replace the else branch with the same pattern this PR uses on the reader side:
} else {
// Don't own fd — free the struct without closing.
file.close_fd = false;
file.detach();
}detach() sets close_after_operation = true, calls stop(), and either finalizes immediately (idle) or via complete() after the in-flight write fires — and with close_fd = false, finalize() destroys the struct without uv_fs_close. This is safe with the existing onFsWriteComplete callbacks since they already capture parent_ptr before file.complete() and don't dereference file afterward on the detached branch.
…oven-sh#29472) ## Summary Three related fixes; the second and third are required by the first. ### 1. `close_jsvalue` Strong → `onCloseCallback` cached slot (cross-platform) `setOnCloseFromJS` stored the callback in a `jsc.Strong`, which forms a rooted cycle: source-wrapper → `close_jsvalue` Strong → bound `#onClose` → `NativeReadableStreamSource` (`ReadableStreamInternals.ts:1972`) → `$stream` private prop (`:1959`) → source-wrapper. Because a Strong is a global GC root, the source survives even after every JS reference (including the outer `ReadableStream`) is dropped. It only becomes collectable when EOF/close runs the JS-side `callClose` (which clears `$stream`) or at VM shutdown. The codegen already declares `onCloseCallback` in `streams.classes.ts` `values`; `onDrain` already uses its cached slot. Switch `onClose` to the same `WriteBarrier`-backed storage and delete the Strong field. The cycle becomes an ordinary intra-heap cycle that mark-sweep collects. ### 2. Windows non-lazy `FileReader` across-read ref `FileReader.onStart` holds an `incrementCount()` until `onReaderDone` only on the lazy path (always) or the POSIX non-lazy path. The Windows non-lazy path — `fromPipe`, reached via `Bun.spawn().stdout`/`.stderr` — did not. With the cycle fix above, the source is now collectable while a `uv_read_start` IOCP read is pending, and `WindowsBufferedReader.deinit` would run with a live `.pipe` source whose `data` ptr is then dereferenced by the queued `onStreamRead`. Add a Windows arm matching the POSIX one. ### 3. Release the across-read ref in `onReaderError` too `onReaderDone` checks `waiting_for_onReaderDone` and decrements; `onReaderError` did not, so a read that ends in error (rather than EOF) leaked the ref taken in `onStart`. Pre-existing on the lazy and POSIX paths; commit 2 adds a Windows arm that would inherit the same gap. Mirror the release after `pending.run()`. ## Verification On Windows, with a child that spawns a detached grandchild inheriting stdout (so the pipe stays open after the direct child exits), repeatedly accessing `proc.stdout`, dropping it, and forcing GC: | | `*ReadableStreamSource` heap count after 30 iters | `WindowsBufferedReader.deinit` reached with live `.pipe` source | |---|---|---| | baseline | 31 (linear growth; 61 at 60 iters) | no — leak masks it | | commit 1 only | ~14 (plateaus at live-grandchild count) | **yes** — `FileReader.deinit` sees `src=pipe`, `closed=false` | | commits 1–3 | ~15 (plateaus; freed as pipes EOF; flat through 80 iters) | no | ## Relation to oven-sh#29440 Found while verifying the review comment on oven-sh#29440 about `WindowsBufferedReader.deinit` ordering. That comment correctly identified the buffer-free-before-detach as theoretical; this PR explains why (the Strong cycle pinned the source) and fixes the underlying leak plus the UAF that fixing the leak would have exposed.
deinit() was setting `this.source = null` before calling closeImpl(false), but closeImpl reads `this.source` — so it became a no-op and the source (file/pipe/tty) was never detached or closed. For file sources this leaks the fd, the heap-allocated Source.File, and leaves any in-flight uv_fs_read uncanceled on the libuv threadpool. Restore the pre-#20198 ordering: call closeImpl first, then null the field (closeImpl already nulls it on the happy path; the trailing assignment covers the already-closed branch).
The lifecycle script runner allocated zero-initialized uv.Pipe handles on Windows unconditionally, but only passed them to spawn (and thus uv_pipe_init) when output is buffered. In foreground/verbose/silent modes the uninitialized pipe sat in reader.source and was leaked by deinit. After the previous commit made deinit actually close the source, that leak became a uv_close() on a handle with type=0, which aborts inside libuv. Gate the allocation on the same condition that selects the .buffer stdio arm.
…seImpl The Posix reader checks `flags.close_handle` before closing the fd; the Windows path didn't — `closeImpl` for `.file` always called `file.detach()` -> `startClose()` -> `uv_fs_close(fd)`, regardless of ownership. FileResponseStream sets `close_handle = false` (it owns the fd via `auto_close` and closes it itself with `Closer.close`). After the deinit-reorder commit made `closeImpl` actually fire from `deinit`, the eof_task path (where the reader is paused with a live `.file` source at deinit time) double-closed: `reader.deinit()` → `uv_fs_close(fd)`, then `Closer.close(fd)` again. Both queue to the threadpool synchronously; with several responses in a row the loser runs after the next response has already reused the fd, closing the wrong file → cascading corruption. Thread `flags.close_handle` through to a new `Source.File.close_fd` flag; when false, `startClose()` frees the struct without `uv_fs_close`.
…assert Review fixes for the close_handle commit: - openFile() initialized via std.mem.zeroes(Source.File), which zeroes close_fd (intended default true) → any caller that doesn't go through WindowsBufferedReader.closeImpl (e.g. PipeWriter with owns_fd=true) would skip uv_fs_close and leak the fd. Use a struct literal so the declared defaults apply. - onFileRead asserted `file.state == .closing` after `file.complete()` when detached, but with close_fd=false complete()→startClose() destroys the struct synchronously. Drop the assert; nothing else on that branch dereferences `file`.
startClose() was the wrong name once it could destroy `this` synchronously when close_fd=false — callers expected `this` to remain valid until onCloseComplete. Rename to finalize() with an explicit "do not touch `this` after this returns" contract, and document on detach()/complete() that they may free.
6e9e66c to
fab3241
Compare
| switch (source) { | ||
| .sync_file, .file => |file| { | ||
| // Detach - file will close itself after operation completes | ||
| // (or just free the struct if the caller owns the fd). | ||
| file.close_fd = this.flags.close_handle; | ||
| file.detach(); |
There was a problem hiding this comment.
🟣 🟣 Pre-existing (not introduced here, just adjacent to fix #3): the .pipe/.tty arms of closeImpl still call pipe.close()/tty.close() unconditionally, ignoring flags.close_handle. FileResponseStream sets close_handle = false and closes the fd itself via Closer.close(this.fd), so on Windows when the served fd resolves to a named pipe (uv_guess_handle → .pipe), both uv_close and uv_fs_close end up closing the same CRT fd. This was already reachable on the common EOF path before this PR (onRead(.eof) → close() → closeImpl(true)); the deinit reordering only adds the rarer read-error path. Worth a follow-up to gate .pipe/.tty on close_handle for symmetry, but not blocking.
Extended reasoning...
What the bug is
This PR's fix #3 threads flags.close_handle into the .file/.sync_file arm of WindowsBufferedReader.closeImpl (PipeReader.zig:1198-1203) via the new file.close_fd field, so that when the caller owns the fd the reader frees the heap struct without issuing uv_fs_close. The .pipe arm (lines 1205-1209) and .tty arm (lines 1210-1219) immediately below were not given the same treatment — they still call pipe.close(onPipeClose) / tty.close(onTTYClose) unconditionally, regardless of this.flags.close_handle.
The specific code path
FileResponseStream (src/runtime/server/FileResponseStream.zig:104) sets reader.flags.close_handle = false and later closes the fd itself in deinit (line 382) via bun.Async.Closer.close(this.fd, …) → uv_fs_close. On Windows, reader.start() calls Source.open() which dispatches on uv_guess_handle; if the served fd is a named pipe (e.g. serving Bun.file('\\\\.\\pipe\\foo') as an HTTP response body), the resulting source is .pipe, not .file.
When that pipe reaches closeImpl, the .pipe arm runs pipe.close(onPipeClose) → uv_close. libuv's pipe__close_pipe for a handle opened via uv_pipe_open calls _close(fd) on the stored CRT fd. Then FileResponseStream.deinit runs Closer.close(this.fd) → uv_fs_close → _close(fd) on the same CRT fd. That is a double close — exactly the class of bug fix #3 addresses for .file sources (a later open can reuse the fd between the two closes, so the second close hits an unrelated handle).
Why this is pre-existing, not introduced by this PR
The dominant trigger was already reachable before any change in this PR. On the normal EOF path: onStreamRead(UV_EOF) → onRead(.eof) → close(this) → closeImpl(true) → .pipe arm → pipe.close(onPipeClose). That call chain is untouched by the diff and never depended on the source = null ordering bug (that bug only affected deinit's closeImpl, not close()'s). So a FileResponseStream wrapping a Windows named pipe with auto_close=true was already calling both pipe.close() and Closer.close(fd) on every successful completion prior to this PR. Fix #1's deinit reordering merely adds the read-error path (onStreamRead error → onError → onReaderError → deref → deinit with a live .pipe source) as a second, rarer entry into the same pre-existing defect.
Step-by-step proof (EOF path, pre-PR and post-PR identical)
- Windows: user returns
new Response(Bun.file('\\\\.\\pipe\\name'))from aBun.servehandler;FileResponseStreamis created withauto_close=true(RequestContext.zig:992 / FileRoute.zig:339 default). reader.flags.close_handle = falseis set (FileResponseStream.zig:104);reader.start(fd, …)→Source.open→uv_guess_handlereturns.named_pipe→source = .{ .pipe = … }withuv_pipe_open(fd).- Writer closes the pipe → libuv fires
onStreamReadwithUV_EOF→onRead(.{.result=0}, "", .eof)→_onReadChunkthenclose(this). close()→stopReading()→closeImpl(true)→.pipearm:pipe.close(onPipeClose)→uv_closequeued; on the next loop tick libuv's pipe close runs_close(crt_fd).done()→onReaderDone→ … → response finishes →FileResponseStreamrefcount hits 0 →deinit→Closer.close(this.fd, loop)→uv_fs_close→ threadpool_close(crt_fd)again. Double close.
Impact
Edge-case (serving a Windows named-pipe handle as an HTTP body), but when hit it is the same fd-reuse corruption fix #3 was written to eliminate for regular files. The PR strictly improves the .file case and does not make the .pipe case meaningfully worse than it already was on the common path.
How to fix
Apply the same gating to the .pipe/.tty arms. Note that you cannot simply skip uv_close on a uv_pipe_t (the handle struct must be uv_closed before being freed, and uv_close on a pipe opened via uv_pipe_open will close the fd), so the cleaner fix is probably on the consumer side: when Source.open produced a .pipe/.tty for a close_handle=false reader, have FileResponseStream skip its own Closer.close (the reader has effectively taken ownership of the fd). Either way it's a sensible follow-up while in this area, not a blocker for this PR.
…oven-sh#29472) ## Summary Three related fixes; the second and third are required by the first. ### 1. `close_jsvalue` Strong → `onCloseCallback` cached slot (cross-platform) `setOnCloseFromJS` stored the callback in a `jsc.Strong`, which forms a rooted cycle: source-wrapper → `close_jsvalue` Strong → bound `#onClose` → `NativeReadableStreamSource` (`ReadableStreamInternals.ts:1972`) → `$stream` private prop (`:1959`) → source-wrapper. Because a Strong is a global GC root, the source survives even after every JS reference (including the outer `ReadableStream`) is dropped. It only becomes collectable when EOF/close runs the JS-side `callClose` (which clears `$stream`) or at VM shutdown. The codegen already declares `onCloseCallback` in `streams.classes.ts` `values`; `onDrain` already uses its cached slot. Switch `onClose` to the same `WriteBarrier`-backed storage and delete the Strong field. The cycle becomes an ordinary intra-heap cycle that mark-sweep collects. ### 2. Windows non-lazy `FileReader` across-read ref `FileReader.onStart` holds an `incrementCount()` until `onReaderDone` only on the lazy path (always) or the POSIX non-lazy path. The Windows non-lazy path — `fromPipe`, reached via `Bun.spawn().stdout`/`.stderr` — did not. With the cycle fix above, the source is now collectable while a `uv_read_start` IOCP read is pending, and `WindowsBufferedReader.deinit` would run with a live `.pipe` source whose `data` ptr is then dereferenced by the queued `onStreamRead`. Add a Windows arm matching the POSIX one. ### 3. Release the across-read ref in `onReaderError` too `onReaderDone` checks `waiting_for_onReaderDone` and decrements; `onReaderError` did not, so a read that ends in error (rather than EOF) leaked the ref taken in `onStart`. Pre-existing on the lazy and POSIX paths; commit 2 adds a Windows arm that would inherit the same gap. Mirror the release after `pending.run()`. ## Verification On Windows, with a child that spawns a detached grandchild inheriting stdout (so the pipe stays open after the direct child exits), repeatedly accessing `proc.stdout`, dropping it, and forcing GC: | | `*ReadableStreamSource` heap count after 30 iters | `WindowsBufferedReader.deinit` reached with live `.pipe` source | |---|---|---| | baseline | 31 (linear growth; 61 at 60 iters) | no — leak masks it | | commit 1 only | ~14 (plateaus at live-grandchild count) | **yes** — `FileReader.deinit` sees `src=pipe`, `closed=false` | | commits 1–3 | ~15 (plateaus; freed as pipes EOF; flat through 80 iters) | no | ## Relation to oven-sh#29440 Found while verifying the review comment on oven-sh#29440 about `WindowsBufferedReader.deinit` ordering. That comment correctly identified the buffer-free-before-detach as theoretical; this PR explains why (the Strong cycle pinned the source) and fixes the underlying leak plus the UAF that fixing the leak would have exposed.
…ead of Strong (#32582) ## Summary `NewSource.close_jsvalue` was a `jsc.Strong`, which rooted a cycle through the native heap: ``` source wrapper (JS{Blob,Bytes,File}InternalReadableStreamSource) -> m_ctx NewSource -> close_jsvalue (Strong root) -> bound #onClose -> NativeReadableStreamSource instance -> $stream private prop -> source wrapper ``` Because a `Strong` is a global GC root, the source wrapper survives even after every JS reference (including the outer `ReadableStream`) is dropped. The cycle only broke when EOF ran the JS-side `callClose` (which clears `$stream`) or the cancel algorithm ran `#cancel`. A stream that is read partially and then dropped (`reader.releaseLock()` without `cancel()`) never hits either path, so the source wrapper leaked one per abandoned stream until VM shutdown. ## Reproduction ```js import { heapStats } from "bun:jsc"; const payload = Buffer.alloc(8 * 1024 * 1024, "x"); for (let i = 0; i < 30; i++) { const reader = new Blob([payload]).stream().getReader(); await reader.read(); reader.releaseLock(); } Bun.gc(true); console.log(heapStats().objectTypeCounts.BlobInternalReadableStreamSource); // before: 35 (one per iteration + warmup) // after: 1 ``` The same pattern hits `fetch()` response bodies whose body exceeds one pull buffer. ## Fix The codegen already declares an `onCloseCallback` `WriteBarrier` slot in `streams.classes.ts` (`values: ["pendingPromise", "onCloseCallback", "onDrainCallback"]`); `onDrain` already uses its slot. Switch `onClose` to the same storage and delete the `Strong` field. The cycle becomes an ordinary intra-heap cycle that mark-sweep collects. Also take the across-read ref on the Windows non-lazy `FileReader.on_start` path (`fromPipe` via `Bun.spawn().stdout`/`.stderr`), matching the existing POSIX arm. Without the Strong cycle masking it, the source is now collectable while a `uv_read_start` IOCP read is pending; the ref keeps it alive until `on_reader_done`/`on_reader_error` releases it. ## Relation to #29472 / #29440 This re-applies the fix originally landed as #29472 (Zig, reverted in bf2e2ce) and re-opened as #29440 (still targets the uncompiled `.zig` reference files), to the Rust port. The `WindowsBufferedReader.deinit` ordering fix that #29440 bundles is already present in `src/io/PipeReader.rs`. ## Verification ``` # without fix (fail) native ReadableStream source is collectable after partial read + releaseLock Expected: < 8 Received: 30 (fail) fetch body native source is collectable after partial read + releaseLock Expected: < 8 Received: 30 # with fix (pass) native ReadableStream source is collectable after partial read + releaseLock (pass) fetch body native source is collectable after partial read + releaseLock (pass) native ReadableStream source is collectable after full consumption (pass) native source onClose callback still fires after switching to cached slot ``` --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
2 participants
Summary
Consolidates the Windows
BufferedReaderfd-ownership fixes with theReadableStreamSourceStrong-cycle leak fix (originally #29472, reverted in bf2e2ce because it shifted timing enough to expose the third bug below). They're entangled — fixing the leak makes sources collectable, which exposes paths whereWindowsBufferedReader.deinitruns with a live source for the first time.1.
WindowsBufferedReader.deinit: don't nullsourcebeforecloseImpl(#20198 regression)deinitsetthis.source = nullbefore callingcloseImpl(false), butcloseImplreadsthis.source— so the call was a no-op. For.file: fd + heapSource.Fileleak + uncanceled threadpool read. For.pipe/.tty: handle leak. Restore the pre-#20198 ordering.2.
lifecycle_script_runner: only allocate stdio pipes when buffering on WindowsAllocated zero-init
uv.Pipehandles unconditionally but onlyuv_pipe_init'd them when output is buffered. With (1) restored,uv_closeon atype=0handle aborts inside libuv. Gate the allocation onbuffer_output.3.
closeImpl: honorflags.close_handlefor.filesourcesThe Posix reader checks
close_handlebefore closing; Windows didn't —closeImplcalledfile.detach()→startClose()→uv_fs_close(fd)unconditionally.FileResponseStreamsetsclose_handle = falseand closes viaCloser.close(this.fd)itself. With (1) restored, the eof_task path (reader paused with live.filesource at deinit) double-closed: bothuv_fs_close(fd)queue to the threadpool back-to-back; with several responses in a row the loser runs after the next response has reused the fd → cascading corruption (thebun-install.test.ts"chooses" segfault). Threadclose_handlethrough toSource.File.close_fd; when false, free the struct withoutuv_fs_close.4.
close_jsvalueStrong →onCloseCallbackcached slot (cross-platform)setOnCloseFromJSstored the callback in ajsc.Strong, forming a rooted cycle (source → Strong → bound#onClose→NativeReadableStreamSource.$stream→ source). Switch to the codegen-providedWriteBarrierslot (mirroringonDrain); the cycle becomes a normal intra-heap cycle.5. Windows non-lazy
FileReader.onStart: hold an across-read reffromPipe(e.g.Bun.spawn().stdout) skipped theincrementCount()on Windows. With (4), the source becomes collectable while auv_read_startIOCP read is pending. Add the Windows arm matching POSIX.6.
FileReader.onReaderError: release the across-read refonReaderDonedecrements;onReaderErrordidn't. Mirror it.Verification (Windows)
bun-install.test.ts -t "chooses"(11 tests): all pass, everyuv_fs_close(N) = 0, no BADF (vsSTATUS_BREAKPOINTbefore commit 3).*ReadableStreamSourceheap count plateaus at ~15 (= live grandchildren) instead of growing linearly to 31/61/...FileReader.deinit src=pipe closed=falsepanic with only commit 4).