Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 47 additions & 18 deletions src/io/PipeReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2027,7 +2027,18 @@ impl WindowsBufferedReader {
pub fn close_impl<const CALL_DONE: bool>(&mut self) {
if let Some(source) = self.source.take() {
match source {
Source::SyncFile(file) | Source::File(file) => {
Source::SyncFile(mut file) | Source::File(mut file) => {
// An in-flight read op's `iov` targets `_buffer`'s spare
// capacity, and `uv_cancel` cannot stop an op already
// running on the threadpool: move the allocation into the
// File box so the op writes into live memory. It is freed
// when the callback reclaims the box.
Comment thread
robobun marked this conversation as resolved.
if matches!(
file.state,
crate::source::FileState::Operating | crate::source::FileState::Canceling
) {
file.orphaned_buffer = mem::take(&mut self._buffer);
}
// Hand the Box off to libuv: detach() leaves either an
// in-flight uv_fs_read (on_file_read) or a scheduled
// uv_fs_close (on_close_complete) pending; the callback
Expand Down Expand Up @@ -2083,6 +2094,23 @@ impl WindowsBufferedReader {
}
}

/// If a threadpool file op is still in flight, transfer fd-close duty to
/// the File source: `complete()` honors `close_after_operation` once the
/// op's callback fires, and `on_close_complete` then reclaims the box.
/// Returns false when idle; the caller then still owns closing the fd.
Comment thread
robobun marked this conversation as resolved.
pub fn close_fd_after_pending_op(&mut self) -> bool {
if let Some(Source::File(file) | Source::SyncFile(file)) = self.source.as_mut() {
if matches!(
file.state,
crate::source::FileState::Operating | crate::source::FileState::Canceling
) {
file.close_after_operation = true;
return true;
}
}
false
}

/// Close the reader and call the done callback.
/// If a file operation is in progress, defers the done callback until
/// the operation completes to ensure proper cleanup ordering.
Expand Down Expand Up @@ -2111,24 +2139,25 @@ impl WindowsBufferedReader {
/// before Drop; both paths are idempotent over an already-taken source.
pub fn deinit(&mut self) {
MaxBuf::remove_from_pipereader(&mut self.maxbuf);
self._buffer = Vec::new();
let Some(source) = self.source.take() else {
return;
};
if !source.is_closed() {
// closeImpl will take care of freeing the source.
// Dropping the `Box<Pipe>` here would free a uv_pipe_t still
// linked into the loop's handle queue → UAF. Restore the source so
// close_impl can do the proper take + hand-off to libuv
// (into_raw + uv_close).
self.source = Some(source);
self.close_impl::<false>();
} else {
// Already closing/closed: a uv close callback may still be pending
// on this allocation; dropping the Box would free memory libuv
// still owns, so leak it instead.
core::mem::forget(source);
if let Some(source) = self.source.take() {
if !source.is_closed() {
// closeImpl will take care of freeing the source.
// Dropping the `Box<Pipe>` here would free a uv_pipe_t still
// linked into the loop's handle queue → UAF. Restore the source so
// close_impl can do the proper take + hand-off to libuv
// (into_raw + uv_close).
Comment thread
robobun marked this conversation as resolved.
self.source = Some(source);
self.close_impl::<false>();
} else {
// Already closing/closed: a uv close callback may still be pending
// on this allocation; dropping the Box would free memory libuv
// still owns, so leak it instead.
Comment thread
robobun marked this conversation as resolved.
core::mem::forget(source);
}
}
// After close_impl, which moves the allocation into the File box when
// an in-flight read op is still writing into it.
Comment thread
robobun marked this conversation as resolved.
self._buffer = Vec::new();
}

#[cfg(windows)]
Expand Down
12 changes: 12 additions & 0 deletions src/io/pipes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,18 @@ impl PollOrFd {
{
self.close_impl(ctx, on_close_fn, true);
}

/// Unregister and free the `FilePoll` (releasing its event-loop active
/// ref) without closing the fd. For owners that cleared `CLOSE_HANDLE`
/// and close the fd themselves; the reader/writer teardown skips the
/// handle in that mode. No-op unless currently `Poll`. Not available on
/// Windows, where `close_impl` always closes a valid fd.
Comment thread
robobun marked this conversation as resolved.
#[cfg(not(windows))]
pub fn release_poll_keep_fd(&mut self) {
if matches!(self, PollOrFd::Poll(_)) {
self.close_impl(None, None::<fn(*mut c_void)>, false);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

// Sunk to `bun_io` so `FilePoll::file_type()` needs no aio→io edge; re-export
Expand Down
5 changes: 5 additions & 0 deletions src/io/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ pub struct File {

/// When true, file will close itself when the current operation completes.
pub(crate) close_after_operation: bool,

/// A detached reader's `_buffer`, moved here when an in-flight read op's
/// `iov` still targets it; freed when the box is reclaimed after the op.
Comment thread
robobun marked this conversation as resolved.
pub(crate) orphaned_buffer: Vec<u8>,
}

#[repr(u8)]
Expand All @@ -94,6 +98,7 @@ impl Default for File {
file: 0,
state: FileState::Deinitialized,
close_after_operation: false,
orphaned_buffer: Vec::new(),
}
}
}
Expand Down
23 changes: 19 additions & 4 deletions src/runtime/server/FileResponseStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,10 @@ impl FileResponseStream {
(self.on_complete.get())(self.ctx.get(), resp);
}

// On abort the read can still be parked on a poll that will never
// fire; no reader callback is coming to adopt the in-flight read ref.
Comment thread
robobun marked this conversation as resolved.
drop(self.take_read_ref());
Comment thread
robobun marked this conversation as resolved.

// Release the owner ref from `heap::into_raw` in `start()`. Every entry
// point that can reach here holds its own ref, so the free lands on
// that guard's drop, not here.
Expand Down Expand Up @@ -600,12 +604,23 @@ bun_io::impl_buffered_reader_parent! {
impl Drop for FileResponseStream {
fn drop(&mut self) {
bun_output::scoped_log!(FileResponseStream, "deinit");
// `self.reader` (BufferedReader) is torn down by its own `Drop` as a
// field — closes the poll handle. `bun.destroy(this)` is owned by
// `heap::take` in `deref`, not here.
// `start()` cleared CLOSE_HANDLE, so the reader's own `Drop` skips the
// handle; `auto_close` below owns the fd.
Comment thread
robobun marked this conversation as resolved.
#[cfg(unix)]
self.reader
.with_mut(|reader| reader.handle.release_poll_keep_fd());
if self.auto_close.get() {
// uv_cancel cannot stop a read already running on the threadpool,
// and Closer::close is an async uv_fs_close on that same pool:
// with an op in flight, the detached File closes the fd instead,
// after the op completes.
Comment thread
robobun marked this conversation as resolved.
#[cfg(windows)]
Closer::close(self.fd.get(), bun_sys::windows::libuv::Loop::get());
if !self
.reader
.with_mut(|reader| reader.close_fd_after_pending_op())
{
Closer::close(self.fd.get(), bun_sys::windows::libuv::Loop::get());
}
#[cfg(not(windows))]
Closer::close(self.fd.get(), ());
}
Expand Down
9 changes: 2 additions & 7 deletions src/runtime/shell/IOReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
//! *NOTE* This type is reference counted via `Arc`; see the `Drop` impl note.

use core::cell::UnsafeCell;
#[cfg(not(windows))]
use core::ffi::c_void;

use bun_sys::{self as sys, Fd};

Expand Down Expand Up @@ -402,11 +400,8 @@ impl Drop for IOReader {
#[cfg(not(windows))]
{
// We cleared CLOSE_HANDLE in init(), so reader Drop will not
// return the FilePoll to its pool. Do it explicitly (without
// closing the fd — we own that and close it ourselves below).
if matches!(r.handle, bun_io::pipes::PollOrFd::Poll(_)) {
r.handle.close_impl(None, None::<fn(*mut c_void)>, false);
}
// return the FilePoll to its pool; we own and close the fd.
r.handle.release_poll_keep_fd();
let _ = sys::close(s.fd);
}
}
Expand Down
22 changes: 3 additions & 19 deletions src/runtime/shell/IOWriter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@

use bun_collections::VecExt;
use core::cell::UnsafeCell;
#[cfg(not(windows))]
use core::ffi::c_void;

#[cfg(windows)]
use bun_io::pipe_writer::BaseWindowsPipeWriter as _;
Expand Down Expand Up @@ -386,11 +384,7 @@ impl IOWriter {
s.flags.pollable = false;
s.flags.nonblock = false;
s.flags.is_socket = false;
if matches!(s.writer.handle, bun_io::pipes::PollOrFd::Poll(_)) {
s.writer
.handle
.close_impl(None, None::<fn(*mut c_void)>, false);
}
s.writer.handle.release_poll_keep_fd();
s.writer.handle = bun_io::pipes::PollOrFd::Closed;
return self.__start();
}
Expand All @@ -402,11 +396,7 @@ impl IOWriter {
s.flags.pollable = false;
s.flags.nonblock = false;
s.flags.is_socket = false;
if matches!(s.writer.handle, bun_io::pipes::PollOrFd::Poll(_)) {
s.writer
.handle
.close_impl(None, None::<fn(*mut c_void)>, false);
}
s.writer.handle.release_poll_keep_fd();
s.writer.handle = bun_io::pipes::PollOrFd::Closed;
return self.__start();
}
Expand Down Expand Up @@ -1210,13 +1200,7 @@ impl Drop for IOWriter {
let s = self.state.get_mut();
crate::shell_log!("IOWriter(fd={}) deinit", s.fd);
#[cfg(not(windows))]
{
if matches!(s.writer.handle, bun_io::pipes::PollOrFd::Poll(_)) {
s.writer
.handle
.close_impl(None, None::<fn(*mut c_void)>, false);
}
}
s.writer.handle.release_poll_keep_fd();
#[cfg(windows)]
{
s.writer.close();
Expand Down
Loading