diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index 006cf841e31..580afb2e7f1 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -6849,6 +6849,8 @@ bun_jsc::jsc_host_abi! { // TODO: move to bun_sys? /// Generic file-open helper used by ReadFile/WriteFile/CopyFile state machines, /// modeled as a trait the target implements. +/// `get_fd` is POSIX-only: the Windows tasks (`ReadFileUV`, `WriteFileWindows`) +/// open through libuv themselves, since their completions may free them. pub trait FileOpener: Sized { /// Override if you need different open flags; defaults to RDONLY. const OPEN_FLAGS: i32 = bun_sys::O::RDONLY; @@ -6872,18 +6874,8 @@ pub trait FileOpener: Sized { ) -> Retry { Retry::No } - #[cfg(windows)] - fn loop_(&self) -> *mut bun_libuv_sys::uv_loop_t; - #[cfg(windows)] - fn req(&mut self) -> &mut bun_libuv_sys::uv_fs_t; - /// Stash/retrieve the open completion callback across the libuv async hop. - /// Rust can't const-generic over fn - /// pointers, so the implementor stores it on `self` (e.g. next to `req`). - #[cfg(windows)] - fn set_open_callback(&mut self, cb: fn(&mut Self, Fd)); - #[cfg(windows)] - fn open_callback(&self) -> fn(&mut Self, Fd); + #[cfg(not(windows))] fn get_fd_by_opening(&mut self, callback: fn(&mut Self, Fd)) { let mut buf = bun_paths::PathBuffer::uninit(); let path_string = match self.pathlike() { @@ -6892,126 +6884,43 @@ pub trait FileOpener: Sized { }; let path = path_string.slice_z(&mut buf); - #[cfg(windows)] - { - use bun_sys::ReturnCodeExt as _; - // Monomorphic libuv completion thunk — recovers `*mut Self` from - // `req.data`. - extern "C" fn wrapped_callback(req: *mut bun_libuv_sys::uv_fs_t) { - use bun_sys::ReturnCodeExt as _; - // SAFETY: `req.data` was set to `self as *mut Self` below before - // `uv_fs_open` was queued; libuv guarantees `req` is valid here. - let self_: &mut S = unsafe { bun_ptr::callback_ctx::((*req).data) }; - { - // SAFETY: req points into self_.req(); cleanup before reuse. - scopeguard::defer! { unsafe { bun_libuv_sys::uv_fs_req_cleanup(req); } } - // SAFETY: req is the live uv_fs_t from the open request. - let result = unsafe { (*req).result }; - if let Some(err_enum) = result.err_enum_e() { - let path_string_2 = match self_.pathlike() { - PathOrFileDescriptor::Path(p) => p.clone(), - PathOrFileDescriptor::Fd(_) => unreachable!(), - }; - self_.set_errno(bun_errno::from_errno(err_enum as i32).into()); - self_.set_system_error( - bun_sys::Error::from_code(err_enum, bun_sys::Tag::open) - .with_path(path_string_2.slice()) - .to_system_error() - .into(), - ); - self_.set_opened_fd(bun_sys::Fd::INVALID); - } else { - self_.set_opened_fd(Fd::from_uv(result.to_fd())); - } + loop { + match bun_sys::open( + path, + Self::OPEN_FLAGS | Self::OPENER_FLAGS, + crate::node::fs::DEFAULT_PERMISSION, + ) { + bun_sys::Result::Ok(fd) => { + self.set_opened_fd(fd); + break; } - let cb = self_.open_callback(); - cb(self_, self_.opened_fd()); - } - - self.set_open_callback(callback); - let loop_ = self.loop_(); - let self_ptr: *mut Self = core::ptr::from_mut(self); - // Derive `req` THROUGH `self_ptr` rather than via a fresh `self.req()` - // reborrow. Under Stacked Borrows, a direct `self.req()` here would - // create a sibling `&mut` that pops `self_ptr`'s tag, making the - // later deref in `wrapped_callback` (via `req.data`) UB. Going - // through the raw pointer keeps the reborrow as a child of - // `self_ptr`, so its provenance survives until the callback fires. - // SAFETY: `self_ptr` was just derived from a live `&mut self`. - let req = unsafe { (*self_ptr).req() }; - // Stash `self` on the request BEFORE dispatch. libuv never touches - // `req.data`, so pre-setting is safe; doing it after `uv_fs_open` - // is a UAF when the call fails synchronously and `callback` frees - // `self` (ReadFileUV::on_finish → finalize → heap::take). - req.data = self_ptr.cast(); - // SAFETY: loop_/req are live for the duration of the async open; - // req.data is consumed by `wrapped_callback::` above. - let rc = unsafe { - bun_libuv_sys::uv_fs_open( - loop_, - req, - path.as_ptr(), - Self::OPEN_FLAGS | Self::OPENER_FLAGS, - node::fs::DEFAULT_PERMISSION as i32, - Some(wrapped_callback::), - ) - }; - if let Some(errno) = rc.err_enum_e() { - self.set_errno(bun_errno::from_errno(errno as i32).into()); - self.set_system_error( - bun_sys::Error::from_code(errno, bun_sys::Tag::open) - .with_path(path_string.slice()) - .to_system_error() - .into(), - ); - self.set_opened_fd(bun_sys::Fd::INVALID); - // `callback` may free `self` (see comment above) — must be the - // last thing we touch on this path. - callback(self, bun_sys::Fd::INVALID); - return; - } - return; - } - - #[cfg(not(windows))] - { - loop { - match bun_sys::open( - path, - Self::OPEN_FLAGS | Self::OPENER_FLAGS, - crate::node::fs::DEFAULT_PERMISSION, - ) { - bun_sys::Result::Ok(fd) => { - self.set_opened_fd(fd); - break; - } - bun_sys::Result::Err(err) => { - if err.get_errno() == bun_sys::E::ENOENT { - match self.try_mkdirp(err.clone(), path, path_string.slice()) { - Retry::Continue => continue, - Retry::Fail => { - // `mkdir_if_not_exists` already populated - // `errno`/`system_error` on the impl. - self.set_opened_fd(Fd::INVALID); - break; - } - Retry::No => {} + bun_sys::Result::Err(err) => { + if err.get_errno() == bun_sys::E::ENOENT { + match self.try_mkdirp(err.clone(), path, path_string.slice()) { + Retry::Continue => continue, + Retry::Fail => { + // `mkdir_if_not_exists` already populated + // `errno`/`system_error` on the impl. + self.set_opened_fd(Fd::INVALID); + break; } + Retry::No => {} } - self.set_errno(bun_errno::from_errno(err.errno as i32).into()); - self.set_system_error(jsc::SysErrorJsc::to_system_error( - &err.with_path(path_string.slice()), - )); - self.set_opened_fd(Fd::INVALID); - break; } + self.set_errno(bun_errno::from_errno(err.errno as i32).into()); + self.set_system_error(jsc::SysErrorJsc::to_system_error( + &err.with_path(path_string.slice()), + )); + self.set_opened_fd(Fd::INVALID); + break; } } - - callback(self, self.opened_fd()); } + + callback(self, self.opened_fd()); } + #[cfg(not(windows))] fn get_fd(&mut self, callback: fn(&mut Self, Fd)) { if self.opened_fd() != Fd::INVALID { callback(self, self.opened_fd()); diff --git a/src/runtime/webcore/blob/copy_file.rs b/src/runtime/webcore/blob/copy_file.rs index a4b01641799..cd6db3311b0 100644 --- a/src/runtime/webcore/blob/copy_file.rs +++ b/src/runtime/webcore/blob/copy_file.rs @@ -1115,6 +1115,18 @@ impl Default for ReadWriteLoop { } } +/// What a step of the copy asks [`CopyFileWindows::finish`] to do with the task. The steps take +/// `&mut self`, which has to stay valid until they return, so none of them frees the task; the +/// entry points hold the heap pointer and hand the step to `finish`, which does. +#[cfg(windows)] +#[must_use] +enum Step { + /// A libuv request or pool task is in flight; its completion runs the next step. + Pending, + /// The copy is over: settle the promise with this outcome and free the task. + Done(bun_sys::Result), +} + // `ReadWriteLoop` is a subobject of `CopyFileWindows`, so passing both as // `&mut` would be aliasing UB. These are hoisted onto `CopyFileWindows` so the // borrow checker can see `self.read_write_loop` / `self.io_request` / `self.event_loop` @@ -1166,196 +1178,198 @@ impl<'a> CopyFileWindows<'a> { } } +/// Queues closes for the descriptors `prepare_pathlike` opened (always libuv-owned, see there); +/// a store's own descriptor is left alone. #[cfg(windows)] -impl ReadWriteLoop { - pub fn close(&mut self) { +impl Drop for ReadWriteLoop { + fn drop(&mut self) { if self.must_close_source_fd { - match self.source_fd.make_libuv_owned() { - Ok(fd) => { - aio::Closer::close(fd, aio::Loop::get()); - } - Err(_) => { - self.source_fd.close(); - } - } - self.must_close_source_fd = false; - self.source_fd = Fd::INVALID; + aio::Closer::close(self.source_fd, aio::Loop::get()); } - if self.must_close_destination_fd { - match self.destination_fd.make_libuv_owned() { - Ok(fd) => { - aio::Closer::close(fd, aio::Loop::get()); - } - Err(_) => { - self.destination_fd.close(); - } - } - self.must_close_destination_fd = false; - self.destination_fd = Fd::INVALID; + aio::Closer::close(self.destination_fd, aio::Loop::get()); } - - self.read_buf = Vec::new(); // clearAndFree() } } +/// Releases the loop reference `init` took (a request or pool task is in flight whenever the +/// task outlives a return to the loop); descriptors and store references go with the fields. #[cfg(windows)] -extern "C" fn on_read(req: *mut libuv::fs_t) { - // SAFETY: `req->data` was set to `core::ptr::from_mut(self)` (whole-struct - // provenance) before scheduling. Recover the parent from `data` rather than - // `from_field_ptr!(.., io_request, req)`: the `req` pointer libuv hands back was - // produced from a `&mut self.io_request` reborrow whose provenance covers only the - // `io_request` field, so `container_of`-style subtraction would yield a - // `*mut CopyFileWindows` with out-of-bounds provenance (UB under Stacked/Tree - // Borrows). After forming `this`, access the request via `this.io_request` — never - // through `(*req)`, which would alias the live `&mut`. - let this: &mut CopyFileWindows = unsafe { &mut *(*req).data.cast::() }; - debug_assert!(core::ptr::addr_of_mut!(this.io_request) == req); - - let source_fd = this.read_write_loop.source_fd; - let destination_fd = this.read_write_loop.destination_fd; - // reshaped for borrowck — `read_buf.items` is `Vec` len-slice. - let read_buf = &mut this.read_write_loop.read_buf; - - let event_loop = this.event_loop; - - let rc = this.io_request.result; - - bun_sys::syslog!( - "uv_fs_read({}, {}) = {}", - source_fd, - read_buf.len(), - rc.int() - ); - if let Some(err) = rc.to_error(bun_sys::Tag::read) { - this.err = Some(err); - this.on_read_write_loop_complete(); - return; +impl Drop for CopyFileWindows<'_> { + fn drop(&mut self) { + self.io_request.deinit(); + self.event_loop.unref_keep_alive(); } +} - let n = usize::try_from(rc.int()).expect("int cast"); - // SAFETY: libuv wrote `n` bytes into the buffer's capacity. - unsafe { read_buf.set_len(n) }; - this.read_write_loop.uv_buf = libuv::uv_buf_t::init(read_buf.as_slice()); +/// Body of the libuv completions below: run `step` on the task that owns `req`, then `finish`. +#[cfg(windows)] +fn on_uv_complete<'a>(req: *mut libuv::fs_t, step: fn(&mut CopyFileWindows<'a>) -> Step) { + // SAFETY: `data` was set to the task pointer before the request was queued. It is used + // instead of `from_field_ptr!` because `req` only carries the `io_request` field's + // provenance; the request is accessed through the task from here on. + let this: *mut CopyFileWindows<'a> = unsafe { (*req).data.cast::>() }; + // SAFETY: `this` is live; only a field address is taken. + debug_assert!(unsafe { core::ptr::addr_of_mut!((*this).io_request) } == req); + // SAFETY: `this` is live and nothing else borrows it; no step frees it. + let step = unsafe { step(&mut *this) }; + // SAFETY: `this` is still live; only `finish` frees it. + unsafe { CopyFileWindows::finish(this, step) }; +} - if rc.int() == 0 { - // Handle EOF. We can't read any more. - this.on_read_write_loop_complete(); - return; - } +#[cfg(windows)] +extern "C" fn on_read(req: *mut libuv::fs_t) { + on_uv_complete(req, CopyFileWindows::on_read_complete); +} - // Re-use the fs request. - this.io_request.deinit(); - // SAFETY: FFI — `io_request` was just cleaned via `deinit()`, `uv_buf` points into - // `read_buf` (len set above), and `on_write` is a valid `uv_fs_cb`. - let rc2 = unsafe { - libuv::uv_fs_write( - event_loop.uv_loop(), - &mut this.io_request, - destination_fd.uv(), - core::ptr::from_mut(&mut this.read_write_loop.uv_buf), - 1, - -1, - Some(on_write), - ) - }; - this.io_request.data = core::ptr::from_mut(this).cast::(); +#[cfg(windows)] +extern "C" fn on_write(req: *mut libuv::fs_t) { + on_uv_complete(req, CopyFileWindows::on_write_complete); +} - if let Some(err) = rc2.to_error(bun_sys::Tag::write) { - this.err = Some(err); - this.on_read_write_loop_complete(); - return; - } +#[cfg(windows)] +extern "C" fn on_copy_file(req: *mut libuv::fs_t) { + on_uv_complete(req, CopyFileWindows::on_copyfile_complete); } #[cfg(windows)] -extern "C" fn on_write(req: *mut libuv::fs_t) { - // SAFETY: see `on_read` — recover from `req->data` (whole-struct provenance), - // not `from_field_ptr!`; then access the request only via `this.io_request`. - let this: &mut CopyFileWindows = unsafe { &mut *(*req).data.cast::() }; - debug_assert!(core::ptr::addr_of_mut!(this.io_request) == req); - let buf_len = this.read_write_loop.read_buf.len(); - - let destination_fd = this.read_write_loop.destination_fd; - - let rc = this.io_request.result; - - bun_sys::syslog!( - "uv_fs_write({}, {}) = {}", - destination_fd, - buf_len, - rc.int() - ); - - if let Some(err) = rc.to_error(bun_sys::Tag::write) { - this.err = Some(err); - this.on_read_write_loop_complete(); - return; - } +extern "C" fn on_chmod(req: *mut libuv::fs_t) { + on_uv_complete(req, CopyFileWindows::on_chmod_complete); +} - let wrote: u32 = u32::try_from(rc.int()).expect("int cast"); +#[cfg(windows)] +impl<'a> CopyFileWindows<'a> { + fn on_read_complete(&mut self) -> Step { + let source_fd = self.read_write_loop.source_fd; + let destination_fd = self.read_write_loop.destination_fd; + // reshaped for borrowck — `read_buf.items` is `Vec` len-slice. + let read_buf = &mut self.read_write_loop.read_buf; - this.read_write_loop.written += wrote as usize; + let event_loop = self.event_loop; - if (wrote as usize) < buf_len { - if wrote == 0 { - // Handle EOF. We can't write any more. - this.on_read_write_loop_complete(); - return; + let rc = self.io_request.result; + + bun_sys::syslog!( + "uv_fs_read({}, {}) = {}", + source_fd, + read_buf.len(), + rc.int() + ); + if let Some(err) = rc.to_error(bun_sys::Tag::read) { + self.err = Some(err); + return self.on_read_write_loop_complete(); + } + + let n = usize::try_from(rc.int()).expect("int cast"); + // SAFETY: libuv wrote `n` bytes into the buffer's capacity. + unsafe { read_buf.set_len(n) }; + self.read_write_loop.uv_buf = libuv::uv_buf_t::init(read_buf.as_slice()); + + if rc.int() == 0 { + // Handle EOF. We can't read any more. + return self.on_read_write_loop_complete(); } // Re-use the fs request. - this.io_request.deinit(); - this.io_request.data = core::ptr::from_mut(this).cast::(); - - let prev = this.read_write_loop.uv_buf.slice(); - this.read_write_loop.uv_buf = libuv::uv_buf_t::init(&prev[wrote as usize..]); - // SAFETY: FFI — `io_request` was just cleaned via `deinit()`, `uv_buf` is a tail - // slice of the previous write buffer (still backed by `read_buf`), and - // `on_write` is a valid `uv_fs_cb`. + self.io_request.deinit(); + // SAFETY: FFI — `io_request` was just cleaned via `deinit()`, `uv_buf` points into + // `read_buf` (len set above), and `on_write` is a valid `uv_fs_cb`. let rc2 = unsafe { libuv::uv_fs_write( - this.event_loop.uv_loop(), - &mut this.io_request, + event_loop.uv_loop(), + &mut self.io_request, destination_fd.uv(), - core::ptr::from_mut(&mut this.read_write_loop.uv_buf), + core::ptr::from_mut(&mut self.read_write_loop.uv_buf), 1, -1, Some(on_write), ) }; + self.io_request.data = core::ptr::from_mut(self).cast::(); if let Some(err) = rc2.to_error(bun_sys::Tag::write) { - this.err = Some(err); - this.on_read_write_loop_complete(); - return; + self.err = Some(err); + return self.on_read_write_loop_complete(); } - return; + Step::Pending } - this.io_request.deinit(); - match this.read_write_loop_read() { - bun_sys::Result::Err(err) => { - this.err = Some(err); - this.on_read_write_loop_complete(); + fn on_write_complete(&mut self) -> Step { + let buf_len = self.read_write_loop.read_buf.len(); + + let destination_fd = self.read_write_loop.destination_fd; + + let rc = self.io_request.result; + + bun_sys::syslog!( + "uv_fs_write({}, {}) = {}", + destination_fd, + buf_len, + rc.int() + ); + + if let Some(err) = rc.to_error(bun_sys::Tag::write) { + self.err = Some(err); + return self.on_read_write_loop_complete(); } - bun_sys::Result::Ok(()) => {} - } -} -#[cfg(windows)] -impl<'a> CopyFileWindows<'a> { - pub(crate) fn on_read_write_loop_complete(&mut self) { - self.event_loop.unref_keep_alive(); + let wrote: u32 = u32::try_from(rc.int()).expect("int cast"); + + self.read_write_loop.written += wrote as usize; + + if (wrote as usize) < buf_len { + if wrote == 0 { + // Handle EOF. We can't write any more. + return self.on_read_write_loop_complete(); + } + + // Re-use the fs request. + self.io_request.deinit(); + self.io_request.data = core::ptr::from_mut(self).cast::(); + + let prev = self.read_write_loop.uv_buf.slice(); + self.read_write_loop.uv_buf = libuv::uv_buf_t::init(&prev[wrote as usize..]); + // SAFETY: FFI — `io_request` was just cleaned via `deinit()`, `uv_buf` is a tail + // slice of the previous write buffer (still backed by `read_buf`), and + // `on_write` is a valid `uv_fs_cb`. + let rc2 = unsafe { + libuv::uv_fs_write( + self.event_loop.uv_loop(), + &mut self.io_request, + destination_fd.uv(), + core::ptr::from_mut(&mut self.read_write_loop.uv_buf), + 1, + -1, + Some(on_write), + ) + }; + + if let Some(err) = rc2.to_error(bun_sys::Tag::write) { + self.err = Some(err); + return self.on_read_write_loop_complete(); + } + + return Step::Pending; + } + + self.io_request.deinit(); + match self.read_write_loop_read() { + bun_sys::Result::Err(err) => { + self.err = Some(err); + self.on_read_write_loop_complete() + } + bun_sys::Result::Ok(()) => Step::Pending, + } + } + fn on_read_write_loop_complete(&mut self) -> Step { if let Some(err) = self.err.take() { - self.throw(err); - return; + return Step::Done(Err(err)); } let written = self.read_write_loop.written; - self.on_complete(written); + self.on_complete(written) } pub(crate) fn new(init: CopyFileWindows<'a>) -> Box> { @@ -1372,7 +1386,9 @@ impl<'a> CopyFileWindows<'a> { ) -> JSValue { // destination_file_store.ref() / source_file_store.ref() — Arc clone let global = event_loop.global_ref(); - let result = bun_core::heap::into_raw(CopyFileWindows::new(CopyFileWindows { + // Balanced by `Drop`. + event_loop.ref_keep_alive(); + let this = bun_core::heap::into_raw(CopyFileWindows::new(CopyFileWindows { destination_file_store, source_file_store, promise: jsc::JSPromiseStrong::init(global), @@ -1387,13 +1403,13 @@ impl<'a> CopyFileWindows<'a> { err: None, read_write_loop: ReadWriteLoop::default(), })); - // SAFETY: result was just allocated above - let result_ref = unsafe { &mut *result }; - let promise = result_ref.promise.value(); - - // On error, this function might free the CopyFileWindows struct. - // So we can no longer reference it beyond this point. - result_ref.copyfile(); + // SAFETY: `this` was allocated just above and nothing else holds it yet. The promise is + // read out first because `finish` frees the task if `copyfile` fails synchronously. + let promise = unsafe { (*this).promise.value() }; + // SAFETY: as above; `copyfile` does not free the task. + let step = unsafe { (*this).copyfile() }; + // SAFETY: as above; after this call the task belongs to libuv / the pool, or is gone. + unsafe { Self::finish(this, step) }; promise } @@ -1438,7 +1454,7 @@ impl<'a> CopyFileWindows<'a> { } } - fn prepare_read_write_loop(&mut self) { + fn prepare_read_write_loop(&mut self) -> Step { // Open the destination first, so that if we need to call // mkdirp(), we don't spend extra time opening the file handle for // the source. @@ -1454,12 +1470,10 @@ impl<'a> CopyFileWindows<'a> { bun_sys::Result::Ok(fd) => fd, bun_sys::Result::Err(err) => { if self.mkdirp_if_not_exists && err.get_errno() == bun_sys::E::ENOENT { - self.mkdirp(); - return; + return self.mkdirp(); } - self.throw(err); - return; + return Step::Done(Err(err)); } }; @@ -1470,29 +1484,23 @@ impl<'a> CopyFileWindows<'a> { ) { bun_sys::Result::Ok(fd) => fd, bun_sys::Result::Err(err) => { - self.throw(err); - return; + return Step::Done(Err(err)); } }; match self.read_write_loop_start() { - bun_sys::Result::Err(err) => { - self.throw(err); - } - bun_sys::Result::Ok(()) => { - self.event_loop.ref_keep_alive(); - } + bun_sys::Result::Err(err) => Step::Done(Err(err)), + bun_sys::Result::Ok(()) => Step::Pending, } } - fn copyfile(&mut self) { + fn copyfile(&mut self) -> Step { // This is for making it easier for us to test this code path if bun_core::env_var::feature_flag::BUN_FEATURE_FLAG_DISABLE_UV_FS_COPYFILE .get() .unwrap_or(false) { - self.prepare_read_write_loop(); - return; + return self.prepare_read_write_loop(); } let mut pathbuf1 = PathBuffer::uninit(); @@ -1520,20 +1528,17 @@ impl<'a> CopyFileWindows<'a> { let fd = *fd; match bun_sys::File::borrow(&fd).kind() { bun_sys::Result::Err(err) => { - self.throw(err); - return; + return Step::Done(Err(err)); } bun_sys::Result::Ok(kind) => match kind { bun_sys::FileKind::Directory => { - self.throw(bun_sys::Error::from_code( + return Step::Done(Err(bun_sys::Error::from_code( bun_sys::E::EISDIR, bun_sys::Tag::open, - )); - return; + ))); } bun_sys::FileKind::CharacterDevice => { - self.prepare_read_write_loop(); - return; + return self.prepare_read_write_loop(); } _ => { let out = match bun_sys::get_fd_path(fd, &mut pathbuf1) { @@ -1542,8 +1547,7 @@ impl<'a> CopyFileWindows<'a> { // This case can happen when either: // - NUL device // - Pipe. `cat foo.txt | bun bar.ts` - self.prepare_read_write_loop(); - return; + return self.prepare_read_write_loop(); } }; let len = out.len(); @@ -1565,20 +1569,17 @@ impl<'a> CopyFileWindows<'a> { let fd = *fd; match bun_sys::File::borrow(&fd).kind() { bun_sys::Result::Err(err) => { - self.throw(err); - return; + return Step::Done(Err(err)); } bun_sys::Result::Ok(kind) => match kind { bun_sys::FileKind::Directory => { - self.throw(bun_sys::Error::from_code( + return Step::Done(Err(bun_sys::Error::from_code( bun_sys::E::EISDIR, bun_sys::Tag::open, - )); - return; + ))); } bun_sys::FileKind::CharacterDevice => { - self.prepare_read_write_loop(); - return; + return self.prepare_read_write_loop(); } _ => { let out = match bun_sys::get_fd_path(fd, &mut pathbuf2) { @@ -1587,8 +1588,7 @@ impl<'a> CopyFileWindows<'a> { // This case can happen when either: // - NUL device // - Pipe. `cat foo.txt | bun bar.ts` - self.prepare_read_write_loop(); - return; + return self.prepare_read_write_loop(); } }; let len = out.len(); @@ -1619,7 +1619,7 @@ impl<'a> CopyFileWindows<'a> { }; if let Some(errno) = rc.errno() { - self.throw(bun_sys::Error { + return Step::Done(Err(bun_sys::Error { // #6336 errno: if errno == bun_sys::SystemErrno::EPERM as u16 { bun_sys::SystemErrno::ENOENT as u16 @@ -1629,32 +1629,69 @@ impl<'a> CopyFileWindows<'a> { syscall: bun_sys::Tag::copyfile, path: old_path.as_bytes().into(), ..Default::default() - }); - return; + })); } - self.event_loop.ref_keep_alive(); + Step::Pending } - pub fn throw(&mut self, err: bun_sys::Error) { - let global_this = self.event_loop.global_ref(); - // `swap()` returns a `&mut JSPromise` into a GC-owned cell (not into - // `self`), but its lifetime is elided to `&mut self`. Decay to a raw pointer so - // borrowck doesn't tie it to `self` across `destroy` below. - let promise = JSPromise::opaque_mut(self.promise.swap()); - let err_instance = err.to_js_with_async_stack(global_this, promise); + fn on_copyfile_complete(&mut self) -> Step { + let rc = self.io_request.result; + + bun_sys::syslog!("uv_fs_copyfile() = {}", rc); + if let Some(errno) = rc.err_enum_e() { + // ENOENT from uv_fs_copyfile can mean either the source file or the + // destination directory is missing. Disambiguate so a missing source + // rejects directly instead of entering the mkdirp+retry path. Only an + // ENOENT from the probe counts as "missing"; any other error leaves + // the mkdirp+retry path available. + let source_missing = errno == bun_sys::E::ENOENT + && match &self.source_file_store.data.as_file().pathlike { + PathOrFileDescriptor::Path(p) => { + let mut buf = bun_paths::path_buffer_pool::get(); + matches!( + bun_sys::access(p.slice_z(&mut buf), 0), + bun_sys::Result::Err(e) if e.get_errno() == bun_sys::E::ENOENT + ) + } + PathOrFileDescriptor::Fd(_) => false, + }; - // SAFETY: VM-owned event loop is valid for the process lifetime; `enter_scope` - // calls enter() now and exit() on drop. - let _guard = unsafe { - jsc::event_loop::EventLoop::enter_scope(self.event_loop as *const _ as *mut _) - }; - // SAFETY: self was heap-allocated in init(); destroy reclaims and drops it. self is not accessed afterward. - unsafe { Self::destroy(core::ptr::from_mut(self)) }; - // `promise` points to a GC-owned `JSPromise` cell, not into `self`; valid after `destroy`. - let _ = promise.reject(global_this, err_instance); // TODO: properly propagate exception upwards + if self.mkdirp_if_not_exists && errno == bun_sys::E::ENOENT && !source_missing { + self.io_request.deinit(); + return self.mkdirp(); + } + + let mut err = bun_sys::Error::from_code( + // #6336 + if errno == bun_sys::E::EPERM { + bun_sys::E::ENOENT + } else { + errno + }, + bun_sys::Tag::copyfile, + ); + let store = if source_missing { + &self.source_file_store + } else { + &self.destination_file_store + }; + match &store.data.as_file().pathlike { + PathOrFileDescriptor::Path(p) => { + err = err.with_path(p.slice()); + } + PathOrFileDescriptor::Fd(fd) => { + err = err.with_fd(*fd); + } + } + + return Step::Done(Err(err)); + } + + let size = self.io_request.statbuf.size(); + self.on_complete(size as usize) } - pub(crate) fn on_complete(&mut self, written_actual: usize) { + fn on_complete(&mut self, written_actual: usize) -> Step { let mut written = written_actual; if written != usize::try_from(self.size).expect("int cast") && self.size != MAX_SIZE { self.truncate(); @@ -1713,32 +1750,59 @@ impl<'a> CopyFileWindows<'a> { if let PathOrFileDescriptor::Path(p) = &destination.pathlike { err = err.with_path(p.slice()); } - self.throw(err); - return; + return Step::Done(Err(err)); } - self.event_loop.ref_keep_alive(); - return; + return Step::Pending; } } - self.resolve_promise(written); + Step::Done(Ok(written)) } - fn resolve_promise(&mut self, written: usize) { - let global_this = self.event_loop.global_ref(); - // see `throw` — re-type the GC cell via the ZST opaque deref so it - // outlives `destroy(self)` for borrowck. - let promise = JSPromise::opaque_mut(self.promise.swap()); + fn on_chmod_complete(&mut self) -> Step { + let rc = self.io_request.result; + if let Some(errno) = rc.err_enum_e() { + let mut err = bun_sys::Error::from_code(errno, bun_sys::Tag::chmod); + let destination = &self.destination_file_store.data.as_file(); + if let PathOrFileDescriptor::Path(p) = &destination.pathlike { + err = err.with_path(p.slice()); + } + return Step::Done(Err(err)); + } + + Step::Done(Ok(self.written_bytes)) + } + + /// Takes `*mut Self`, not `&mut self`: on [`Step::Done`] this reclaims the `Box` `init` + /// leaked and drops it, which a `&mut self` argument would have to outlive. + /// + /// # Safety + /// `this` is the live task from `init` and nothing borrows it; on `Done` it is freed here. + unsafe fn finish(this: *mut Self, step: Step) { + let Step::Done(result) = step else { return }; + + // SAFETY: caller contract. + let mut task = unsafe { bun_core::heap::take(this) }; + let event_loop = task.event_loop; + let mut promise = task.promise.take(); + let global_this = event_loop.global_ref(); + let settled = match result { + Ok(written) => Ok(JSValue::js_number_from_uint64(written as u64)), + Err(err) => Err(err.to_js_with_async_stack(global_this, promise.get())), + }; + // SAFETY: VM-owned event loop is valid for the process lifetime; `enter_scope` // calls enter() now and exit() on drop. let _guard = unsafe { - jsc::event_loop::EventLoop::enter_scope(self.event_loop as *const _ as *mut _) + jsc::event_loop::EventLoop::enter_scope(core::ptr::from_ref(event_loop).cast_mut()) + }; + // Only queues the closes and releases the loop reference; script runs when `_guard` + // drops, after the settle. + drop(task); + let _ = match settled { + Ok(written) => promise.resolve(global_this, written), + Err(err) => promise.reject(global_this, err), }; - - // SAFETY: self was heap-allocated in init(); destroy reclaims and drops it. self is not accessed afterward. - unsafe { Self::destroy(core::ptr::from_mut(self)) }; - // `promise` points to a GC-owned `JSPromise` cell, not into `self`; valid after `destroy`. - let _ = promise.resolve(global_this, JSValue::js_number_from_uint64(written as u64)); // TODO: properly propagate exception upwards } #[cold] @@ -1756,20 +1820,7 @@ impl<'a> CopyFileWindows<'a> { ); } - /// SAFETY: `this` must have been produced by `heap::alloc` in `init()` and - /// not yet destroyed. After this call `this` is dangling. - pub(crate) unsafe fn destroy(this: *mut Self) { - // SAFETY: caller contract — `this` is a live `heap::alloc`-ed pointer. - unsafe { - (*this).read_write_loop.close(); - // destination_file_store.deref() / source_file_store.deref() — Arc Drop on Box drop - // promise.deinit() — handled by JscStrong's Drop on Box drop - (*this).io_request.deinit(); - drop(bun_core::heap::take(this)); - } - } - - fn mkdirp(&mut self) { + fn mkdirp(&mut self) -> Step { bun_sys::syslog!("mkdirp"); self.mkdirp_if_not_exists = false; // Borrowck: compute the raw path slice pointer up-front so the @@ -1778,160 +1829,66 @@ impl<'a> CopyFileWindows<'a> { let path: *const [u8] = { let destination = &self.destination_file_store.data.as_file(); if !matches!(destination.pathlike, PathOrFileDescriptor::Path(_)) { - self.throw(bun_sys::Error { + return Step::Done(Err(bun_sys::Error { errno: bun_sys::SystemErrno::EINVAL as u16, syscall: bun_sys::Tag::mkdir, ..Default::default() - }); - return; + })); } let path_slice = destination.pathlike.path().slice(); // BORROW: not owned — `destination_file_store` (and thus its path) is held in - // `self`, which outlives the workpool task (completion runs `copyfile`/`throw` - // on `self` before any `destroy`). + // `self`, which outlives the workpool task (the completion runs + // `on_mkdirp_complete` on `self` before `finish` can free it). bun_paths::dirname(path_slice) // this shouldn't happen .unwrap_or(path_slice) as *const [u8] }; - self.event_loop.ref_keep_alive(); node_fs::async_::AsyncMkdirp::schedule(node_fs::async_::AsyncMkdirp { completion: on_mkdirp_complete_concurrent, completion_ctx: core::ptr::from_mut(self).cast::<()>(), path, ..Default::default() }); + Step::Pending } - fn on_mkdirp_complete(&mut self) { - self.event_loop.unref_keep_alive(); - + fn on_mkdirp_complete(&mut self) -> Step { if let Some(err) = self.err.take() { - // `bun_sys::Error.path` is an owned `Box<[u8]>` and is dropped with - // `err` inside `throw`. - self.throw(err); - return; - } - - self.copyfile(); - } -} - -#[cfg(windows)] -extern "C" fn on_copy_file(req: *mut libuv::fs_t) { - // SAFETY: see `on_read` — recover from `req->data` (whole-struct provenance), - // not `from_field_ptr!`; then access the request only via `this.io_request`. - let this: &mut CopyFileWindows = unsafe { &mut *(*req).data.cast::() }; - debug_assert!(core::ptr::addr_of_mut!(this.io_request) == req); - - let event_loop = this.event_loop; - event_loop.unref_keep_alive(); - let rc = this.io_request.result; - - bun_sys::syslog!("uv_fs_copyfile() = {}", rc); - if let Some(errno) = rc.err_enum_e() { - // ENOENT from uv_fs_copyfile can mean either the source file or the - // destination directory is missing. Disambiguate so a missing source - // rejects directly instead of entering the mkdirp+retry path. Only an - // ENOENT from the probe counts as "missing"; any other error leaves - // the mkdirp+retry path available. - let source_missing = errno == bun_sys::E::ENOENT - && match &this.source_file_store.data.as_file().pathlike { - PathOrFileDescriptor::Path(p) => { - let mut buf = bun_paths::path_buffer_pool::get(); - matches!( - bun_sys::access(p.slice_z(&mut buf), 0), - bun_sys::Result::Err(e) if e.get_errno() == bun_sys::E::ENOENT - ) - } - PathOrFileDescriptor::Fd(_) => false, - }; - - if this.mkdirp_if_not_exists && errno == bun_sys::E::ENOENT && !source_missing { - this.io_request.deinit(); - this.mkdirp(); - return; + return Step::Done(Err(err)); } - let mut err = bun_sys::Error::from_code( - // #6336 - if errno == bun_sys::E::EPERM { - bun_sys::E::ENOENT - } else { - errno - }, - bun_sys::Tag::copyfile, - ); - let store = if source_missing { - &this.source_file_store - } else { - &this.destination_file_store - }; - match &store.data.as_file().pathlike { - PathOrFileDescriptor::Path(p) => { - err = err.with_path(p.slice()); - } - PathOrFileDescriptor::Fd(fd) => { - err = err.with_fd(*fd); - } - } - - this.throw(err); - return; + self.copyfile() } - - let size = this.io_request.statbuf.size(); - this.on_complete(size as usize); -} - -#[cfg(windows)] -extern "C" fn on_chmod(req: *mut libuv::fs_t) { - // SAFETY: see `on_read` — recover from `req->data` (whole-struct provenance), - // not `from_field_ptr!`; then access the request only via `this.io_request`. - let this: &mut CopyFileWindows = unsafe { &mut *(*req).data.cast::() }; - debug_assert!(core::ptr::addr_of_mut!(this.io_request) == req); - - let event_loop = this.event_loop; - event_loop.unref_keep_alive(); - - let rc = this.io_request.result; - if let Some(errno) = rc.err_enum_e() { - let mut err = bun_sys::Error::from_code(errno, bun_sys::Tag::chmod); - let destination = &this.destination_file_store.data.as_file(); - if let PathOrFileDescriptor::Path(p) = &destination.pathlike { - err = err.with_path(p.slice()); - } - this.throw(err); - return; - } - - this.resolve_promise(this.written_bytes); } #[cfg(windows)] fn on_mkdirp_complete_concurrent(ctx: *mut (), err_: bun_sys::Maybe<()>) { bun_sys::syslog!("mkdirp complete"); - // SAFETY: `ctx` is the `*mut CopyFileWindows` stored in `AsyncMkdirp.completion_ctx` - // by `mkdirp` above; sole owner on this concurrent path. - let this = unsafe { bun_ptr::callback_ctx::(ctx.cast()) }; - debug_assert!(this.err.is_none()); - this.err = match err_ { - bun_sys::Result::Err(e) => Some(e), - bun_sys::Result::Ok(()) => None, + let this = ctx.cast::(); + // SAFETY: `ctx` is the task `mkdirp` stored in `AsyncMkdirp.completion_ctx`, and this pool + // thread is the only thing touching it until the hop below is queued. The handle is + // cloned out because the JS thread may free the task as soon as that happens, before + // `post_task` returns, so nothing pointing into the task may be live across the post. + let loop_handle = unsafe { + debug_assert!((*this).err.is_none()); + (*this).err = err_.err(); + (*this).loop_handle.clone() }; // callback signature to match `ManagedTask::new`'s `fn(*mut T) -> JsResult<()>`. fn call_erased(this: *mut CopyFileWindows<'_>) -> bun_event_loop::JsResult<()> { - // SAFETY: `this` is the heap-allocated `CopyFileWindows` passed to - // `ManagedTask::new` below; `on_mkdirp_complete` may free it via `throw`, so we - // do not touch `this` afterward. - unsafe { (*this).on_mkdirp_complete() }; + // SAFETY: `this` is the heap-allocated `CopyFileWindows` passed to `ManagedTask::new` + // below, back on the JS thread; `on_mkdirp_complete` does not free it. + let step = unsafe { (*this).on_mkdirp_complete() }; + // SAFETY: as above; `finish` may free the task, and nothing touches it afterwards. + unsafe { CopyFileWindows::finish(this, step) }; Ok(()) } let ct = jsc::ConcurrentTask::create(jsc::ManagedTask::ManagedTask::new::( this, call_erased, )); - if let jsc::vm_handle::Posted::Refused(ct) = this.loop_handle.post_task(ct) { + if let jsc::vm_handle::Posted::Refused(ct) = loop_handle.post_task(ct) { // VM torn down: nobody will settle the promise; free the hop. // SAFETY: refused ⇒ we own the task box. unsafe { bun_event_loop::ConcurrentTask::ConcurrentTask::release_refused(ct) }; diff --git a/src/runtime/webcore/blob/read_file.rs b/src/runtime/webcore/blob/read_file.rs index 8bc36a6b580..5085602f5b4 100644 --- a/src/runtime/webcore/blob/read_file.rs +++ b/src/runtime/webcore/blob/read_file.rs @@ -7,6 +7,8 @@ use core::sync::atomic::AtomicU8; use core::sync::atomic::Ordering; use crate::Error; +#[cfg(windows)] +use crate::node::types::PathLikeExt as _; use crate::webcore::Lifetime; #[cfg(not(windows))] use crate::webcore::blob::ClosingState; @@ -309,22 +311,6 @@ impl FileOpener for ReadFile { fn pathlike(&self) -> &PathOrFileDescriptor { &self.file_store.pathlike } - #[cfg(windows)] - fn loop_(&self) -> *mut bun_libuv_sys::uv_loop_t { - unreachable!("ReadFile is POSIX-only; see ReadFileUV") - } - #[cfg(windows)] - fn req(&mut self) -> &mut bun_libuv_sys::uv_fs_t { - unreachable!("ReadFile is POSIX-only; see ReadFileUV") - } - #[cfg(windows)] - fn set_open_callback(&mut self, _cb: fn(&mut Self, Fd)) { - unreachable!() - } - #[cfg(windows)] - fn open_callback(&self) -> fn(&mut Self, Fd) { - unreachable!() - } } crate::webcore::blob::impl_file_closer!(ReadFile); @@ -923,38 +909,28 @@ pub struct ReadFileUV<'a> { pub(crate) is_regular_file: bool, pub(crate) req: libuv::fs_t, - /// Stash for the open completion callback across the libuv async hop. - open_callback: fn(&mut Self, Fd), } +/// What a step of the read asks [`ReadFileUV::finish`] to do with the task. The steps take +/// `&mut self`, which has to stay valid until they return, so none of them frees the task; the +/// entry points hold the heap pointer and hand the step to `finish`, which does. #[cfg(windows)] -impl<'a> FileOpener for ReadFileUV<'a> { - fn opened_fd(&self) -> Fd { - self.opened_fd - } - fn set_opened_fd(&mut self, fd: Fd) { - self.opened_fd = fd; - } - fn set_errno(&mut self, e: crate::Error) { - self.errno = Some(e); - } - fn set_system_error(&mut self, e: jsc::SystemError) { - self.system_error = Some(e); - } - fn pathlike(&self) -> &PathOrFileDescriptor { - &self.file_store.pathlike - } - fn loop_(&self) -> *mut bun_libuv_sys::uv_loop_t { - self.loop_ - } - fn req(&mut self) -> &mut bun_libuv_sys::uv_fs_t { - &mut self.req - } - fn set_open_callback(&mut self, cb: fn(&mut Self, Fd)) { - self.open_callback = cb; - } - fn open_callback(&self) -> fn(&mut Self, Fd) { - self.open_callback +#[derive(Clone, Copy)] +#[must_use] +enum Step { + /// A libuv request is in flight; its completion runs the next step. + Pending, + /// The read is over; `system_error` / `byte_store` hold the outcome to deliver. + Done, +} + +/// Releases the loop reference `start_with_ctx` took; the store reference (and the completion, +/// which cancels itself if the read never finished) go with the fields. +#[cfg(windows)] +impl Drop for ReadFileUV<'_> { + fn drop(&mut self) { + self.req.deinit(); + self.event_loop.unref_keep_alive(); } } @@ -1032,9 +1008,11 @@ impl<'a> ReadFileUV<'a> { log!("ReadFileUV.start"); // SAFETY: `event_loop` is the per-thread `EventLoop` singleton owned by // the VM (`global.bun_vm().event_loop()`); it strictly outlives this - // async op, which additionally holds a keep-alive on it below. + // async op, which additionally holds a keep-alive on it. let event_loop: &'a EventLoop = unsafe { &*event_loop }; let file_store = store.data.as_file().clone(); + // Balanced by `Drop`. + event_loop.ref_keep_alive(); let this = Box::new(ReadFileUV { // Projected through the helper to avoid materializing a // `&VirtualMachine`. @@ -1057,51 +1035,99 @@ impl<'a> ReadFileUV<'a> { completion: Some(completion), is_regular_file: false, req: bun_core::ffi::zeroed(), - open_callback: Self::on_file_open, }); - // Keep the event loop alive while the async operation is pending - event_loop.ref_keep_alive(); - let this_ptr: *mut ReadFileUV = bun_core::heap::into_raw(this); - // SAFETY: this_ptr is freshly boxed and uniquely owned by the async op. - unsafe { (*this_ptr).get_fd(Self::on_file_open) }; - // ownership now lives with the libuv request chain until finalize(). - let _ = this_ptr; + let this: *mut ReadFileUV = bun_core::heap::into_raw(this); + // SAFETY: just allocated, nothing else holds it; `get_fd` does not free it. + let step = unsafe { (*this).get_fd() }; + // SAFETY: as above; afterwards the task belongs to its libuv requests, or is gone. + unsafe { Self::finish(this, step) }; + } + + /// Uses a `Bun.file(fd)` store's descriptor as is; opens a path through libuv (`on_open`). + fn get_fd(&mut self) -> Step { + if let PathOrFileDescriptor::Fd(fd) = &self.file_store.pathlike { + self.opened_fd = *fd; + return self.on_file_open(); + } + + self.req.data = core::ptr::from_mut(self).cast::(); + let mut buf = bun_paths::path_buffer_pool::get(); + let path = self.file_store.pathlike.path().slice_z(&mut buf); + // SAFETY: FFI — `loop_` is the live VM uv loop, `self.req` is the zeroed `fs_t` owned + // by `self`, `path` is NUL-terminated (libuv copies it before returning), and `on_open` + // is a valid `uv_fs_cb` that recovers `self` from `req.data` (set above). + let rc = unsafe { + libuv::uv_fs_open( + self.loop_, + &mut self.req, + path.as_ptr(), + libuv::O::RDONLY, + crate::node::fs::DEFAULT_PERMISSION as i32, + Some(Self::on_open), + ) + }; + if let Some(errno) = rc.err_enum_e() { + self.set_open_error(errno); + return self.on_file_open(); + } + + Step::Pending + } + + fn set_open_error(&mut self, errno: bun_sys::E) { + self.errno = Some(bun_errno::from_errno(errno as i32).into()); + self.system_error = Some( + bun_sys::Error::from_code(errno, bun_sys::Tag::open) + .with_path(self.file_store.pathlike.path().slice()) + .to_system_error() + .into(), + ); } - pub fn finalize(this: *mut Self) { + /// Body of the libuv completions below: run `step` on the task that owns `req`, then `finish`. + fn on_uv_complete(req: *mut libuv::fs_t, step: fn(&mut Self) -> Step) { + // SAFETY: `data` was set to the task pointer before the request was queued; the + // request is accessed through the task from here on. + let this: *mut Self = unsafe { (*req).data.cast::() }; + // SAFETY: `this` is live; only a field address is taken. + debug_assert!(unsafe { core::ptr::addr_of_mut!((*this).req) } == req); + // SAFETY: `this` is live and nothing else borrows it; no step frees it. + let step = unsafe { step(&mut *this) }; + // SAFETY: `this` is still live; only `finish` frees it. + unsafe { Self::finish(this, step) }; + } + + /// Takes `*mut Self`, not `&mut self`: on [`Step::Done`] this reclaims the `Box` + /// `start_with_ctx` leaked and drops it, which a `&mut self` argument would have to outlive. + /// + /// # Safety + /// `this` is the live task from `start_with_ctx` and nothing borrows it; on `Done` it is + /// freed here. + unsafe fn finish(this: *mut Self, step: Step) { + let Step::Done = step else { return }; log!("ReadFileUV.finalize"); - // SAFETY: `this` was heap-allocated in start(); we reclaim ownership here. - let mut this_box = unsafe { bun_core::heap::take(this) }; - let event_loop = this_box.event_loop; + // SAFETY: caller contract. + let mut task = unsafe { bun_core::heap::take(this) }; - let completion = this_box - .completion - .take() - .expect("a ReadFileUV completes once"); + let completion = task.completion.take().expect("a ReadFileUV completes once"); - let result = if let Some(err) = this_box.system_error.take() { + let result = if let Some(err) = task.system_error.take() { ReadFileResultType::Err(err) } else { - // Move byte_store out so dropping `this_box` below does not free the + // Move byte_store out so dropping `task` below does not free the // buffer we hand to the callback. Normalize to `Box<[u8]>` so the // `is_temporary` consumer (Body.rs / Blob.rs) can soundly reclaim // via `heap::take` — handing out `(ptr, len)` from a ByteStore // whose `cap > len` would be a layout-mismatched dealloc. - let boxed = core::mem::take(&mut this_box.byte_store).into_boxed_slice(); + let boxed = core::mem::take(&mut task.byte_store).into_boxed_slice(); ReadFileResultType::Result(ReadFileRead { buf: bun_core::heap::into_raw(boxed), }) }; - // The completion must run BEFORE the cleanup below (store deref / req.deinit / - // box drop / event_loop.unref) — it may inspect store. + // The completion may inspect the store, which `task` holds a reference to. completion.complete(result); - - // store.deref runs via StoreRef's Drop when the Box drops. - this_box.req.deinit(); - drop(this_box); - // Release the event loop reference now that we're done - event_loop.unref_keep_alive(); + drop(task); log!("ReadFileUV.finalize destroy"); } @@ -1109,7 +1135,7 @@ impl<'a> ReadFileUV<'a> { self.file_store.pathlike.is_path() } - fn on_finish(&mut self) { + fn on_finish(&mut self) -> Step { log!("ReadFileUV.onFinish"); let fd = self.opened_fd; let needs_close = fd != Fd::INVALID; @@ -1117,21 +1143,31 @@ impl<'a> ReadFileUV<'a> { self.size = self.read_len.max(self.size); self.total_size = self.total_size.max(self.size); - if needs_close { - if self.do_close(self.is_allowed_to_close()) { - // we have to wait for the close to finish - return; - } + if needs_close && self.do_close(self.is_allowed_to_close()) { + // we have to wait for the close to finish + return Step::Pending; } - Self::finalize(core::ptr::from_mut(self)); + Step::Done } - pub(crate) fn on_file_open(&mut self, opened_fd: Fd) { + extern "C" fn on_open(req: *mut libuv::fs_t) { + Self::on_uv_complete(req, Self::on_open_complete); + } + + fn on_open_complete(&mut self) -> Step { + let result = self.req.result; + match result.err_enum_e() { + Some(errno) => self.set_open_error(errno), + None => self.opened_fd = Fd::from_uv(result.to_fd()), + } + self.on_file_open() + } + + fn on_file_open(&mut self) -> Step { log!("ReadFileUV.onFileOpen"); if self.errno.is_some() { - self.on_finish(); - return; + return self.on_finish(); } self.req.deinit(); @@ -1145,7 +1181,7 @@ impl<'a> ReadFileUV<'a> { libuv::uv_fs_fstat( self.loop_, &mut self.req, - opened_fd.uv(), + self.opened_fd.uv(), Some(Self::on_file_initial_stat), ) }; @@ -1156,35 +1192,33 @@ impl<'a> ReadFileUV<'a> { .to_system_error() .into(), ); - self.on_finish(); - return; + return self.on_finish(); } self.req.data = core::ptr::from_mut(self).cast::(); + Step::Pending } extern "C" fn on_file_initial_stat(req: *mut libuv::fs_t) { + Self::on_uv_complete(req, Self::on_initial_stat_complete); + } + + fn on_initial_stat_complete(&mut self) -> Step { log!("ReadFileUV.onFileInitialStat"); - // SAFETY: req.data was set to *mut Self in on_file_open(). - let this: &mut ReadFileUV = unsafe { bun_ptr::callback_ctx::((*req).data) }; - - // `req` aliases `this.req`; once `&mut ReadFileUV` exists, going through the - // raw `req` pointer would violate Stacked Borrows. Read via `this.req` instead. - if let Some(errno) = this.req.result.err_enum_e() { - this.errno = Some(bun_errno::from_errno(errno as i32).into()); - this.system_error = Some( + if let Some(errno) = self.req.result.err_enum_e() { + self.errno = Some(bun_errno::from_errno(errno as i32).into()); + self.system_error = Some( bun_sys::Error::from_code(errno, bun_sys::Tag::fstat) .to_system_error() .into(), ); - this.on_finish(); - return; + return self.on_finish(); } - let stat = this.req.statbuf; + let stat = self.req.statbuf; // keep in sync with resolveSizeAndLastModified - if let Data::File(file) = this.store.data_mut() { + if let Data::File(file) = self.store.data_mut() { // `uv_timespec_t` fields are `c_long` (i32 on Windows); widen to the // platform-width `isize` `to_js_time` expects. file.last_modified = @@ -1192,11 +1226,11 @@ impl<'a> ReadFileUV<'a> { } if bun_sys::S::ISDIR(u32::try_from(stat.mode()).expect("int cast")) { - this.errno = Some(crate::Error::Sys(bun_errno::SystemErrno::EISDIR)); - this.system_error = Some(SystemError { + self.errno = Some(crate::Error::Sys(bun_errno::SystemErrno::EISDIR)); + self.system_error = Some(SystemError { code: BunString::static_("EISDIR").into(), - path: if this.file_store.pathlike.is_path() { - BunString::clone_utf8(this.file_store.pathlike.path().slice()) + path: if self.file_store.pathlike.is_path() { + BunString::clone_utf8(self.file_store.pathlike.path().slice()) } else { BunString::EMPTY } @@ -1205,28 +1239,27 @@ impl<'a> ReadFileUV<'a> { syscall: BunString::static_("read").into(), ..Default::default() }); - this.on_finish(); - return; + return self.on_finish(); } // `uv_stat_t::st_size` is `u64` (never negative); clamp to MAX_SIZE // without a signed detour so a hypothetical >i64::MAX value isn't // wrapped to negative and then floored to 0. - this.total_size = stat.size().min(MAX_SIZE as u64) as SizeType; - this.is_regular_file = bun_sys::is_regular_file(stat.mode() as bun_sys::Mode); + self.total_size = stat.size().min(MAX_SIZE as u64) as SizeType; + self.is_regular_file = bun_sys::is_regular_file(stat.mode() as bun_sys::Mode); - log!("is_regular_file: {}", this.is_regular_file); + log!("is_regular_file: {}", self.is_regular_file); - if stat.size() > 0 && this.is_regular_file { - this.size = this.total_size.min(this.max_length); - } else if stat.size() == 0 && !this.is_regular_file { + if stat.size() > 0 && self.is_regular_file { + self.size = self.total_size.min(self.max_length); + } else if stat.size() == 0 && !self.is_regular_file { // read up to 4k at a time if they didn't explicitly set a size and // we're reading from something that's not a regular file. - this.size = this.max_length.min(4096); + self.size = self.max_length.min(4096); } - if this.offset > 0 { + if self.offset > 0 { // We DO support offset in Bun.file() - match bun_sys::set_file_offset(this.opened_fd, this.offset) { + match bun_sys::set_file_offset(self.opened_fd, self.offset) { // we ignore errors because it should continue to work even if its a pipe Err(_) | Ok(_) => {} } @@ -1234,43 +1267,40 @@ impl<'a> ReadFileUV<'a> { // Special files might report a size of > 0, and be wrong. // so we should check specifically that its a regular file before trusting the size. - if this.size == 0 && this.is_regular_file { + if self.size == 0 && self.is_regular_file { // buffer is empty here, // so move it (Vec) into the owning ByteStore rather than borrow. - this.byte_store = ByteStore::init(core::mem::take(&mut this.buffer)); - this.on_finish(); - return; + self.byte_store = ByteStore::init(core::mem::take(&mut self.buffer)); + return self.on_finish(); } // Out of memory we can't read more than 4GB at a time (ULONG) on Windows - if this.size as usize > bun_sys::windows::ULONG::MAX as usize { - this.errno = Some(bun_errno::from_errno(bun_sys::E::NOMEM as i32).into()); - this.system_error = Some( + if self.size as usize > bun_sys::windows::ULONG::MAX as usize { + self.errno = Some(bun_errno::from_errno(bun_sys::E::NOMEM as i32).into()); + self.system_error = Some( bun_sys::Error::from_code(bun_sys::E::NOMEM, bun_sys::Tag::read) .to_system_error() .into(), ); - this.on_finish(); - return; + return self.on_finish(); } // add an extra 16 bytes to the buffer to avoid having to resize it for trailing extra data let want = - ((this.size as usize).saturating_add(16)).min(bun_sys::windows::ULONG::MAX as usize); - if this.buffer.try_reserve_exact(want).is_err() { - this.errno = Some(crate::Error::Alloc(bun_alloc::AllocError)); - this.system_error = Some( + ((self.size as usize).saturating_add(16)).min(bun_sys::windows::ULONG::MAX as usize); + if self.buffer.try_reserve_exact(want).is_err() { + self.errno = Some(crate::Error::Alloc(bun_alloc::AllocError)); + self.system_error = Some( bun_sys::Error::from_code(bun_sys::E::NOMEM, bun_sys::Tag::read) .to_system_error() .into(), ); - this.on_finish(); - return; + return self.on_finish(); } - this.read_len = 0; - this.read_off = 0; + self.read_len = 0; + self.read_off = 0; - this.req.deinit(); + self.req.deinit(); - this.queue_read(); + self.queue_read() } fn remaining_buffer(&mut self) -> &mut [MaybeUninit] { @@ -1283,7 +1313,7 @@ impl<'a> ReadFileUV<'a> { &mut spare[..take] } - pub(crate) fn queue_read(&mut self) { + fn queue_read(&mut self) -> Step { // if not a regular file, buffer capacity is arbitrary, and running out doesn't mean we're // at the end of the file if (!self.remaining_buffer().is_empty() || !self.is_regular_file) @@ -1306,8 +1336,7 @@ impl<'a> ReadFileUV<'a> { .to_system_error() .into(), ); - self.on_finish(); - return; + return self.on_finish(); } } @@ -1346,53 +1375,52 @@ impl<'a> ReadFileUV<'a> { .to_system_error() .into(), ); - self.on_finish(); + return self.on_finish(); } + + Step::Pending } else { log!("ReadFileUV.queueRead done"); // We are done reading. let owned = core::mem::take(&mut self.buffer).into_boxed_slice(); self.byte_store = ByteStore::init_owned(owned); - self.on_finish(); + self.on_finish() } } - pub(crate) extern "C" fn on_read(req: *mut libuv::fs_t) { - // SAFETY: req.data was set to *mut Self in queue_read(). - let this: &mut ReadFileUV = unsafe { bun_ptr::callback_ctx::((*req).data) }; + extern "C" fn on_read(req: *mut libuv::fs_t) { + Self::on_uv_complete(req, Self::on_read_complete); + } - // `req` aliases `this.req`; once `&mut ReadFileUV` exists, going through the - // raw `req` pointer would violate Stacked Borrows. Read via `this.req` instead. - let result = this.req.result; + fn on_read_complete(&mut self) -> Step { + let result = self.req.result; if let Some(errno) = result.err_enum_e() { - this.errno = Some(bun_errno::from_errno(errno as i32).into()); - this.system_error = Some( + self.errno = Some(bun_errno::from_errno(errno as i32).into()); + self.system_error = Some( bun_sys::Error::from_code(errno, bun_sys::Tag::read) .to_system_error() .into(), ); - this.on_finish(); - return; + return self.on_finish(); } if result.int() == 0 { // We are done reading. - let owned = core::mem::take(&mut this.buffer).into_boxed_slice(); - this.byte_store = ByteStore::init_owned(owned); - this.on_finish(); - return; + let owned = core::mem::take(&mut self.buffer).into_boxed_slice(); + self.byte_store = ByteStore::init_owned(owned); + return self.on_finish(); } - this.read_off += SizeType::try_from(result.int()).expect("int cast"); + self.read_off += SizeType::try_from(result.int()).expect("int cast"); // SAFETY: libuv wrote result.int() bytes into remaining_buffer()'s spare slice. unsafe { - this.buffer + self.buffer .uv_commit(usize::try_from(result.int()).expect("int cast")) }; - this.req.deinit(); - this.queue_read(); + self.req.deinit(); + self.queue_read() } } diff --git a/src/runtime/webcore/blob/write_file.rs b/src/runtime/webcore/blob/write_file.rs index 293d064020a..2600d918fd1 100644 --- a/src/runtime/webcore/blob/write_file.rs +++ b/src/runtime/webcore/blob/write_file.rs @@ -137,22 +137,6 @@ impl FileOpener for WriteFile { ) -> Retry { mkdir_if_not_exists(self, &err, path, display_path) } - #[cfg(windows)] - fn loop_(&self) -> *mut bun_libuv_sys::uv_loop_t { - unreachable!("WriteFile is POSIX-only; see WriteFileWindows") - } - #[cfg(windows)] - fn req(&mut self) -> &mut bun_libuv_sys::uv_fs_t { - unreachable!("WriteFile is POSIX-only") - } - #[cfg(windows)] - fn set_open_callback(&mut self, _cb: fn(&mut Self, Fd)) { - unreachable!() - } - #[cfg(windows)] - fn open_callback(&self) -> fn(&mut Self, Fd) { - unreachable!() - } } impl MkdirpTarget for WriteFile { @@ -967,20 +951,23 @@ mod windows_impl { } fn on_mkdirp_complete_concurrent(ctx: *mut (), err_: bun_sys::Result<()>) { - // SAFETY: `ctx` is the `*mut Self` stored in `AsyncMkdirp.completion_ctx` - // by `mkdirp` above; sole owner on this concurrent path. - let this = unsafe { bun_ptr::callback_ctx::(ctx.cast()) }; bun_output::scoped_log!(WriteFile, "mkdirp complete"); - debug_assert!(this.err.is_none()); - this.err = match err_ { - bun_sys::Result::Err(e) => Some(e), - bun_sys::Result::Ok(()) => None, + let this = ctx.cast::(); + // SAFETY: `ctx` is the `*mut Self` stored in `AsyncMkdirp.completion_ctx` by + // `mkdirp` above, and this pool thread is the only thing touching it until the + // hop below is queued. The handle is cloned out because the JS thread may free + // the task as soon as that happens, before `post_task` returns, so nothing + // pointing into the task may be live across the post. + let loop_handle = unsafe { + debug_assert!((*this).err.is_none()); + (*this).err = err_.err(); + (*this).loop_handle.clone() }; let ct = ConcurrentTask::create(ManagedTask::new::( this, Self::on_mkdirp_complete_task, )); - if let bun_jsc::vm_handle::Posted::Refused(ct) = this.loop_handle.post_task(ct) { + if let bun_jsc::vm_handle::Posted::Refused(ct) = loop_handle.post_task(ct) { // VM torn down: nobody will settle the promise. Free the hop (the // ConcurrentTask owns the boxed ManagedTask); the operation's // buffers/fd go with the process's teardown of its owner. diff --git a/test/internal/source-lints/self-receiver-teardown.test.ts b/test/internal/source-lints/self-receiver-teardown.test.ts new file mode 100644 index 00000000000..df6f19c0d62 --- /dev/null +++ b/test/internal/source-lints/self-receiver-teardown.test.ts @@ -0,0 +1,227 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// A `&self` / `&mut self` method must not hand its own receiver to the type's +// teardown routine: `Self::destroy(ptr::from_mut(self))`, `Self::deinit(self as +// *mut _)`, `Self::finalize(ptr::from_mut(self))`, or the deferred form +// `scopeguard::guard(ptr::from_mut(self), |p| Self::destroy(p))` (which frees +// in the epilogue, while the receiver argument is still live) are banned. +// +// `destroy(this: *mut Self)` / `deinit(this: *mut Self)` / `finalize(this: *mut +// Self)` are this tree's names for "reclaim the Box" (`heap::take` / +// `heap::destroy` inside). Under Stacked and Tree Borrows a reference argument +// is protected for the whole call, and deallocating protected memory is UB +// regardless of whether the reference is used again afterwards ("deallocating +// while item is strongly protected" / "the strongly protected tag disallows +// deallocations" under Miri, the model `bun run rust:miri` checks; the +// protector is the model's counterpart of the `dereferenceable` attribute +// rustc puts on reference arguments, so this is not only a Miri concern). The +// function that frees has to take the allocation pointer (`this: *mut Self`) +// and reborrow per statement, with its caller (a dispatch arm, a C callback, a +// scope guard over the raw pointer) passing the pointer through; see the +// comment on `deinit(this: *mut Self)` in +// src/sql_jsc/postgres/PostgresSQLConnection.rs, and `finish(this: *mut Self)` +// in src/runtime/webcore/blob/{copy_file,read_file}.rs for a state machine +// whose `&mut self` steps report the outcome instead of freeing. +// +// Scope: the two single-expression shapes above, with the callee literally +// named `destroy`, `deinit` or `finalize` (the name list is the enforcement +// boundary; a new teardown routine under another name goes here too; +// `heap::destroy` counts as a `destroy` spelling). An in-place `finalize(&mut +// self)` called as `self.finalize()` never takes the receiver's address and so +// is not matched. Deliberately outside it: +// - the reclaim primitives themselves (`heap::take(from_mut(self))`, +// `Box::from_raw(self as *mut _)`), which self-receiver-reclaim.test.ts +// covers (a `heap::destroy()` is reported by both, on purpose); +// - refcount releases (`Self::deref(from_mut(self))`), which only free on the +// last count, so each site needs its own argument about who else holds one; +// - a self-derived pointer stashed in a local and freed later, and reference +// parameters (`fn f(this: &mut T)` freeing `this`). +// +// Sibling guards: self-receiver-reclaim.test.ts, fn-long-mut-reborrow.test.ts, +// frozen-nonnull-reborrow.test.ts, unsound-erased-box.test.ts. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const rustSources = globAllSources().rust.filter(p => p.endsWith(".rs")); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). Same guard as +// dead-code-escapes.test.ts. +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +// The ways of spelling "`self`, as a raw pointer", optionally followed by +// pointer-to-pointer conversions that keep it the same address (`.cast()`, +// `.cast_mut()`, `.as_ptr()`). `(?!\s*\.)` after `&raw mut *self` keeps a +// field's address (`&raw mut *self.inner`) out of it. +const SELF_AS_POINTER = + String.raw`(?:` + + [ + String.raw`(?:[\w:]+::)?from_(?:mut|ref)(?:::<[^>]*>)?\(\s*self\s*\)`, + String.raw`(?:[\w:]+::)?NonNull::from\(\s*self\s*\)`, + String.raw`&raw\s+(?:mut|const)\s+\*\s*self\b(?!\s*\.)`, + String.raw`(?:[\w:]+::)?addr_of(?:_mut)?!\s*\(\s*\*\s*self\s*\)`, + String.raw`self\s+as\s+\*(?:mut|const)\b[^,()]*`, + ].join("|") + + String.raw`)(?:\s*\.\s*(?:cast(?:_mut|_const)?(?:::<[^>]*>)?|as_ptr)\(\))*`; + +// `destroy(` / `deinit(` / `finalize(`, optionally path-qualified (`Self::`, +// `Worker::`, `bun_core::heap::`) and turbofished. `\s*` after the paren so a +// rustfmt-wrapped argument list still matches. +const TEARDOWN = String.raw`\b(?:[\w:]+::)?(?:destroy|deinit|finalize)(?:::<[^>]*>)?\s*\(\s*`; + +const DIRECT = new RegExp(`${TEARDOWN}${SELF_AS_POINTER}\\s*[,)]`, "g"); + +// `scopeguard::guard(, |p| { unsafe { Self::destroy(p) } })`: +// the guard's closure must apply the teardown routine to the guarded pointer +// (the back-reference), so a guard over `self`'s address that does something +// else with it (src/runtime/ffi/ffi_body.rs frees a field) does not count. +const DEFERRED = new RegExp( + String.raw`scopeguard::guard\(\s*${SELF_AS_POINTER}\s*,\s*(?:move\s+)?\|\s*(\w+)\s*\|\s*(?:\{\s*)?(?:unsafe\s*\{\s*)?${TEARDOWN}\1\s*\)`, + "g", +); + +// Documented, ratcheted exceptions: files allowed to keep exactly N of the +// shape while their conversion is in flight. Delete an entry when its file is +// converted; never raise one. +const ALLOW: Record = { + // `LifecycleScriptSubprocess::handle_exit` / `deinit_and_delete_package` + // (`&mut self`) free the subprocess at five sites; #37551 turns them into a + // disposition that the raw-pointer thunks act on. + "src/install/lifecycle_script_runner.rs": 5, + // `Worker::deinit_soon` (`&mut self`) frees itself inline when the worker + // was created off the pool; #37685 converts it. + "src/bundler/ThreadPool.rs": 1, + // `UVFSRequest::run_from_js_thread` (the deferred form) and the two + // completion paths of `NewAsyncCpTask::run_from_js_thread` (`&mut self`) + // free the task they run on; #37693 converts them. + "src/runtime/node/node_fs.rs": 3, +}; + +const counts: Record = {}; +const offenders: string[] = []; +let scanned = 0; +for (const abs of rustSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + // `src/cli` is a symlink into `src/runtime/cli`; count each file once under + // its canonical path. + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + const content = await file(abs).text(); + // Strip full-line comments so prose mentions (and SAFETY comments inside a + // guard's closure) don't count or break a match. `[ \t]*`, not `\s*`: `\s` + // crosses newlines and would swallow blank lines, shifting the reported line + // numbers. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + const hits = [DIRECT, DEFERRED] + .flatMap(re => [...stripped.matchAll(re)]) + .map(m => ({ line: stripped.slice(0, m.index).split("\n").length, text: m[0].replace(/\s+/g, " ") })) + .sort((a, b) => a.line - b.line); + if (hits.length === 0) continue; + counts[source] = hits.length; + for (const { line, text } of hits.slice(ALLOW[source] ?? 0)) { + offenders.push(`${source}:${line}: ${text}`); + } +} + +function matches(snippet: string): boolean { + DIRECT.lastIndex = 0; + DEFERRED.lastIndex = 0; + return DIRECT.test(snippet) || DEFERRED.test(snippet); +} + +test("scans a non-empty set of tracked Rust sources", () => { + // Guards against the tracked/realpath filters above over-firing and leaving + // nothing to scan, which would make the ban below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("the patterns recognize the spellings they claim to", () => { + const banned = [ + // The copy_file.rs / read_file.rs lines this lint was written for. + "unsafe { Self::destroy(core::ptr::from_mut(self)) };", + "Self::finalize(core::ptr::from_mut(self));", + // The same shape elsewhere in the tree. + "unsafe { Self::destroy(std::ptr::from_mut::(self)) };", + "let _deinit =\n scopeguard::guard(core::ptr::from_mut(self), |p| unsafe { Self::destroy(p) });", + "Self::finalize(std::ptr::from_mut::(self));", + // Other spellings of the receiver's address. + "unsafe { Self::deinit(std::ptr::from_mut::(self)) };", + "unsafe { Worker::deinit(self as *mut Self) };", + "unsafe { destroy(self as *const Self as *mut Self) }", + "unsafe { Self::destroy(&raw mut *self) }", + "unsafe { Self::destroy(core::ptr::addr_of_mut!(*self)) }", + "unsafe { Self::destroy(ptr::from_ref(self).cast_mut()) }", + "unsafe { Self::destroy(NonNull::from(self).as_ptr()) }", + "unsafe { Self::destroy::(std::ptr::from_mut(self)) }", + "unsafe { crate::node::fs::AsyncCpTask::destroy(std::ptr::from_mut(self)) }", + "unsafe { bun_core::heap::destroy(std::ptr::from_mut::(self)) };", + // Extra arguments after the pointer, and a rustfmt-wrapped call. + "unsafe { Self::deinit(std::ptr::from_mut(self), allocator) }", + "unsafe {\n Self::destroy(\n std::ptr::from_mut::(self),\n )\n}", + // Deferred: block body, `move`, a SAFETY comment already stripped to a blank line. + "let _g = scopeguard::guard(std::ptr::from_mut::(self), |this| {\n\n unsafe { Self::destroy(this) }\n});", + "let _g = scopeguard::guard(self as *mut Self, move |p| unsafe { Self::deinit(p) });", + ]; + const allowed = [ + // The converted shapes: the pointer comes in as a parameter. + "unsafe { Self::destroy(this) }", + "unsafe { Self::finalize(this) };", + "let _deinit = scopeguard::guard(this, |p| unsafe { Self::destroy(p) });", + "unsafe { Self::destroy(cast_ptr!(crate::node::fs::AsyncCpTask)) }", + "unsafe { FSWatchTask::deinit(t) };", + // Freeing something the receiver owns is fine. + "unsafe { Self::destroy(self.child) }", + "unsafe { Worker::deinit(self.worker.as_ptr()) }", + "unsafe { Self::destroy(&raw mut *self.inner) }", + "unsafe { TCC::State::destroy(s.as_ptr()) };", + // By-value / in-place teardown and UFCS forwarding of the reference itself + // are not this shape. + "self.deinit();", + "self.finalize();", + "self.io_request.deinit();", + "Self::deinit(self)", + "Self::deinit(self, id)", + "Self::finalize(self)", + // Producing the receiver's address for something other than teardown. + "self.req.data = core::ptr::from_mut(self).cast::();", + "unsafe { Self::deref_(std::ptr::from_mut::(self)) };", + "unsafe { Self::teardown(core::ptr::from_mut(self), Teardown::MainThreadExit) };", + // The other primitives are a separate population (see the scope note). + "unsafe { drop(bun_core::heap::take(std::ptr::from_mut::(self))) };", + // A guard over the receiver's address whose closure frees a field, or + // releases a refcount, is out of scope. + "let _guard = scopeguard::guard(std::ptr::from_mut::(self), |this_ptr| {\n if let Some(s) = unsafe { (*this_ptr).state.take() } {\n unsafe { TCC::State::destroy(s.as_ptr()) };\n }\n});", + "let _g = scopeguard::guard(std::ptr::from_mut::(self), |s| {\n unsafe { Self::deref_(s) }\n});", + // A guard that frees a different pointer than the one it guards. + "let _g = scopeguard::guard(std::ptr::from_mut::(self), |_p| unsafe { Self::destroy(other) });", + // Not the receiver. + "unsafe { Self::destroy(std::ptr::from_mut::(self_)) };", + "unsafe { Self::destroy(ptr::from_mut(task)) }", + ]; + expect(banned.filter(s => !matches(s))).toEqual([]); + expect(allowed.filter(matches)).toEqual([]); +}); + +test("no method hands its own receiver to destroy/deinit/finalize", () => { + expect(offenders).toEqual([]); +}); + +test("allowlisted files still carry exactly their documented count", () => { + // Ratchet: once an allowlisted file is converted, delete its entry so a new + // instance cannot take the old one's place. + for (const [f, n] of Object.entries(ALLOW)) { + expect(counts[f] ?? 0).toBe(n); + } +}); diff --git a/test/js/bun/io/bun-write.test.js b/test/js/bun/io/bun-write.test.js index f02bdf4f8e1..0ee6ceabc5a 100644 --- a/test/js/bun/io/bun-write.test.js +++ b/test/js/bun/io/bun-write.test.js @@ -216,6 +216,35 @@ const IS_UV_FS_COPYFILE_DISABLED = expect(exitCode).toBe(0); }); + it("Bun.write(dest, Bun.file(src), { mode }) applies the mode once the copy is done", async () => { + using dir = tempDir("bun-write-copy-mode", { + "src.txt": "copy me", + }); + // 0o444 is the one mode every platform can represent (the read-only + // attribute on Windows). The second copy also goes through the + // create-missing-directory retry before the chmod. + const fixture = ` + const { join } = require("path"); + const { statSync, chmodSync } = require("fs"); + const dir = ${JSON.stringify(String(dir))}; + for (const dest of [join(dir, "dest.txt"), join(dir, "a", "b", "dest.txt")]) { + await Bun.write(dest, Bun.file(join(dir, "src.txt")), { mode: 0o444 }); + const mode = statSync(dest).mode & 0o777; + chmodSync(dest, 0o644); + console.log(mode.toString(8) + " " + (await Bun.file(dest).text())); + } + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe("444 copy me\n444 copy me\n"); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + }); + it("Bun.write('out.txt', 'string')", async () => { using tmpbase = tempDir("bun-write-string", {}); const outpath = path.join(tmpbase, "out." + ((Math.random() * 102400) | 0).toString(32) + "txt"); diff --git a/test/js/bun/util/bun-file-fd-read.test.ts b/test/js/bun/util/bun-file-fd-read.test.ts index fe1e53191e3..c06fe553066 100644 --- a/test/js/bun/util/bun-file-fd-read.test.ts +++ b/test/js/bun/util/bun-file-fd-read.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { closeSync, openSync } from "fs"; -import { isWindows, tempDir } from "harness"; +import { tempDir } from "harness"; import { join } from "path"; // Reading a Bun.file() backed by a file descriptor goes through @@ -10,7 +10,9 @@ import { join } from "path"; // so an abnormal fstat size could trip integerOutOfBounds. Triggering that // directly requires fstat to report > 4.5 PB which is not achievable here, // but these tests lock in the fd-backed ReadFile path that the fuzzer hit. -describe.skipIf(isWindows)("Bun.file(fd) read", () => { +// On Windows the same reads take ReadFileUV's fd branch (no uv_fs_open, +// straight to the fstat request). +describe("Bun.file(fd) read", () => { async function withFd(path: string, fn: (fd: number) => Promise): Promise { const fd = openSync(path, "r"); try {