diff --git a/src/jsc/JSSecrets.rs b/src/jsc/JSSecrets.rs index 4c58fc528526..a1a6c9d0dac2 100644 --- a/src/jsc/JSSecrets.rs +++ b/src/jsc/JSSecrets.rs @@ -39,14 +39,16 @@ impl crate::JobContext for SecretsJob { type OffThread = Self; type Js = Strong; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, vm: &crate::vm_handle::Borrow, done: crate::Completion, ) -> Option> { - // SAFETY: the creating global, alive under the borrow; C++ only threads it through. - let global = unsafe { this.global.under_borrow(vm) }; - Bun__SecretsJobOptions__runTask(SecretsJobOptions::opaque_mut(this.options.0), global); + // SAFETY: fn contract; both fields are copied out of the live job. The + // global is the creating one, alive under the borrow; C++ only threads + // it through. + let (options, global) = unsafe { ((*this).options.0, (*this).global.under_borrow(vm)) }; + Bun__SecretsJobOptions__runTask(SecretsJobOptions::opaque_mut(options), global); Some(done) } diff --git a/src/jsc/job.rs b/src/jsc/job.rs index d71a8b480ee0..40a286e50443 100644 --- a/src/jsc/job.rs +++ b/src/jsc/job.rs @@ -238,9 +238,19 @@ pub trait JobContext: Sized + 'static { /// Return `done` to complete now; keep it (e.g. across async I/O that /// finishes on another thread) and call [`Completion::finish`] later to /// complete then. Work that outlives this call runs under no borrow and - /// must touch only `off`. - fn run( - off: &mut Self::OffThread, + /// must touch only the off-thread part. + /// + /// `off` is a pointer, not `&mut`: a body that keeps `done` hands the job + /// on before returning, and the JS thread then frees it through the job's + /// own pointer (`Job::complete`), which is UB while a reference argument + /// is still protected here. Such a body reborrows only for work that ends + /// before the hand-over, which is its last access. + /// + /// # Safety + /// `off` is the live job's off-thread part and nothing else touches it + /// until this returns `Some(done)` or the body hands the job on. + unsafe fn run( + off: *mut Self::OffThread, vm: &Borrow, done: Completion, ) -> Option>; @@ -388,7 +398,9 @@ impl Job { return done.finish(); }; // SAFETY: as above; the borrow keeps the VM (and any JsPtr target) alive. - if let Some(done) = C::run(unsafe { &mut (*this).off }, &vm, done) { + // On `None` the job may already be freed; nothing below touches it + // (`vm` is released through our own `handle` clone). + if let Some(done) = unsafe { C::run(&raw mut (*this).off, &vm, done) } { drop(vm); done.finish(); } @@ -541,7 +553,7 @@ pub enum Never {} impl JobContext for Never { type OffThread = (); type Js = (); - fn run(_: &mut (), _: &Borrow, done: Completion) -> Option> { + unsafe fn run(_: *mut (), _: &Borrow, done: Completion) -> Option> { Some(done) } fn then(_: (), _: (), _: &JsThread<'_>) -> JsResult<()> { diff --git a/src/runtime/api/Archive.rs b/src/runtime/api/Archive.rs index 33922cf458e1..253fb78d0144 100644 --- a/src/runtime/api/Archive.rs +++ b/src/runtime/api/Archive.rs @@ -687,12 +687,14 @@ pub struct AsyncTask(core::marker::PhantomData); impl bun_jsc::JobContext for AsyncTask { type OffThread = C; type Js = JSPromiseStrong; - fn run( - ctx: &mut C, + unsafe fn run( + ctx: *mut C, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - ctx.run(); + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*ctx).run() }; Some(done) } fn then(mut ctx: C, mut promise: JSPromiseStrong, cx: &bun_jsc::JsThread<'_>) -> JsResult<()> { diff --git a/src/runtime/api/BunObject.rs b/src/runtime/api/BunObject.rs index fdedb2be588f..a4be6697f349 100644 --- a/src/runtime/api/BunObject.rs +++ b/src/runtime/api/BunObject.rs @@ -2852,54 +2852,63 @@ pub mod JSZstd { pub error_message: Option<&'static [u8]>, } - impl jsc::JobContext for ZstdJob { - type OffThread = Self; - type Js = jsc::JSPromiseStrong; + impl ZstdJob { + /// Pool thread: fills `output`, or sets `error_message`. + fn run(&mut self) { + let input = self.buffer.slice(); - fn run( - this: &mut Self, - _vm: &jsc::vm_handle::Borrow, - done: bun_jsc::Completion, - ) -> Option> { - let input = this.buffer.slice(); - - if this.is_compress { + if self.is_compress { let max_size = bun_zstd::compress_bound(input.len()); // Surface OOM as a rejected promise instead of aborting. The // zero-fill is output-irrelevant (zstd overwrites the prefix it reports). let mut output: Vec = Vec::new(); if output.try_reserve_exact(max_size).is_err() { - this.error_message = Some(b"Out of memory"); - return Some(done); + self.error_message = Some(b"Out of memory"); + return; } output.resize(max_size, 0); - this.output = output; + self.output = output; - this.output = match bun_zstd::compress(&mut this.output, input, Some(this.level)) { + self.output = match bun_zstd::compress(&mut self.output, input, Some(self.level)) { bun_zstd::Result::Success(size) => 'blk: { - if size < this.output.len() { - let mut out = core::mem::take(&mut this.output); + if size < self.output.len() { + let mut out = core::mem::take(&mut self.output); out.truncate(size); out.shrink_to_fit(); break 'blk out; } - break 'blk core::mem::take(&mut this.output); + break 'blk core::mem::take(&mut self.output); } bun_zstd::Result::Err(err) => { - this.output = Vec::new(); - this.error_message = Some(err); - return Some(done); + self.output = Vec::new(); + self.error_message = Some(err); + return; } }; } else { - this.output = match bun_zstd::decompress_alloc(input) { + self.output = match bun_zstd::decompress_alloc(input) { Ok(v) => v, Err(_) => { - this.error_message = Some(b"Decompression failed"); - return Some(done); + self.error_message = Some(b"Decompression failed"); + return; } }; } + } + } + + impl jsc::JobContext for ZstdJob { + type OffThread = Self; + type Js = jsc::JSPromiseStrong; + + unsafe fn run( + this: *mut Self, + _vm: &jsc::vm_handle::Borrow, + done: bun_jsc::Completion, + ) -> Option> { + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*this).run() }; Some(done) } diff --git a/src/runtime/api/JSTranspiler.rs b/src/runtime/api/JSTranspiler.rs index 27b16aa07e26..ca753090bd90 100644 --- a/src/runtime/api/JSTranspiler.rs +++ b/src/runtime/api/JSTranspiler.rs @@ -677,12 +677,14 @@ pub(crate) struct TransformJs { impl jsc::JobContext for TransformTask { type OffThread = Self; type Js = TransformJs; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, vm: &jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - TransformTask::run(this, vm); + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*this).run(vm) }; Some(done) } fn then(mut this: Self, mut js: TransformJs, cx: &jsc::JsThread<'_>) -> JsResult<()> { diff --git a/src/runtime/api/glob.rs b/src/runtime/api/glob.rs index a44ab7a77e96..1755a428bfca 100644 --- a/src/runtime/api/glob.rs +++ b/src/runtime/api/glob.rs @@ -243,25 +243,34 @@ impl WalkTaskErr { } } +impl WalkTask { + /// Pool thread: runs the walk, recording a failure in `err`. + fn run(&mut self) { + let result = match self.walker.walk() { + Ok(r) => r, + Err(err) => { + self.err = Some(WalkTaskErr::Unknown(err.into())); + return; + } + }; + if let bun_sys::Result::Err(err) = result { + self.err = Some(WalkTaskErr::Syscall(err)); + } + } +} + impl JobContext for WalkTask { type OffThread = Self; type Js = WalkJs; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - let result = match this.walker.walk() { - Ok(r) => r, - Err(err) => { - this.err = Some(WalkTaskErr::Unknown(err.into())); - return Some(done); - } - }; - if let bun_sys::Result::Err(err) = result { - this.err = Some(WalkTaskErr::Syscall(err)); - } + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*this).run() }; Some(done) } diff --git a/src/runtime/crypto/PBKDF2.rs b/src/runtime/crypto/PBKDF2.rs index e70ffa4f1a70..82e997955c61 100644 --- a/src/runtime/crypto/PBKDF2.rs +++ b/src/runtime/crypto/PBKDF2.rs @@ -266,30 +266,39 @@ pub(crate) struct Pbkdf2Job { pub err: bool, } -impl JobContext for Pbkdf2Job { - type OffThread = Self; - type Js = JSPromiseStrong; - - fn run( - this: &mut Self, - _vm: &bun_jsc::vm_handle::Borrow, - done: bun_jsc::Completion, - ) -> Option> { - let len = usize::try_from(this.pbkdf2.length).expect("int cast"); +impl Pbkdf2Job { + /// Pool thread: derives into `output`, or sets `err`. + fn run(&mut self) { + let len = usize::try_from(self.pbkdf2.length).expect("int cast"); // `Vec` allocation aborts on OOM; use try_reserve to surface an error instead. let mut buf = Vec::new(); if buf.try_reserve_exact(len).is_err() { - this.err = true; - return Some(done); + self.err = true; + return; } buf.resize(len, 0); - this.output = buf; + self.output = buf; - if !this.pbkdf2.run(&mut this.output) { - this.err = true; + if !self.pbkdf2.run(&mut self.output) { + self.err = true; boringssl::ERR_clear_error(); - this.output = Vec::new(); + self.output = Vec::new(); } + } +} + +impl JobContext for Pbkdf2Job { + type OffThread = Self; + type Js = JSPromiseStrong; + + unsafe fn run( + this: *mut Self, + _vm: &bun_jsc::vm_handle::Borrow, + done: bun_jsc::Completion, + ) -> Option> { + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*this).run() }; Some(done) } diff --git a/src/runtime/crypto/PasswordObject.rs b/src/runtime/crypto/PasswordObject.rs index 2b2bb24c255b..c30a93202f54 100644 --- a/src/runtime/crypto/PasswordObject.rs +++ b/src/runtime/crypto/PasswordObject.rs @@ -567,12 +567,14 @@ impl Drop for PasswordJob { impl bun_jsc::JobContext for PasswordJob { type OffThread = Self; type Js = JSPromiseStrong; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - this.value = Some(this.op.compute(&this.password)); + // SAFETY: fn contract; the job is not handed on, so the reborrows are + // exclusive for the statement. + unsafe { (*this).value = Some((*this).op.compute(&(*this).password)) }; Some(done) } fn then( diff --git a/src/runtime/dns_jsc/dns.rs b/src/runtime/dns_jsc/dns.rs index d6a9b5e20c41..c2b6aa72c0a4 100644 --- a/src/runtime/dns_jsc/dns.rs +++ b/src/runtime/dns_jsc/dns.rs @@ -1010,12 +1010,14 @@ pub mod get_addr_info_request { impl bun_jsc::JobContext for LibcLookup { type OffThread = Self; type Js = LibcRequest; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - this.backend.run(); + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*this).backend.run() }; Some(done) } fn then( diff --git a/src/runtime/image/Image.rs b/src/runtime/image/Image.rs index 7a02e6a4d7ff..9a90641615a1 100644 --- a/src/runtime/image/Image.rs +++ b/src/runtime/image/Image.rs @@ -1465,12 +1465,14 @@ impl Drop for PendingTask { impl jsc::JobContext for PipelineTask { type OffThread = Self; type Js = PipelineJs; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - this.run(); + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*this).run() }; Some(done) } fn then(this: Self, js: PipelineJs, cx: &jsc::JsThread<'_>) -> jsc::JsResult<()> { diff --git a/src/runtime/node/node_crypto_binding.rs b/src/runtime/node/node_crypto_binding.rs index ac3c88150877..f76c19bc834c 100644 --- a/src/runtime/node/node_crypto_binding.rs +++ b/src/runtime/node/node_crypto_binding.rs @@ -125,16 +125,17 @@ macro_rules! extern_crypto_job { type OffThread = Self; type Js = Strong; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, vm: &Borrow, done: bun_jsc::Completion, ) -> Option> { - // SAFETY: the creating global, alive under the borrow; C++ - // only threads it through to error reporting state. - ctx_run_task(Ctx::opaque_ref(this.ctx.0), unsafe { - this.global.under_borrow(vm) - }); + // SAFETY: fn contract; both fields are copied out of the live + // job. The global is the creating one, alive under the + // borrow; C++ only threads it through to error reporting + // state. + let (ctx, global) = unsafe { ((*this).ctx.0, (*this).global.under_borrow(vm)) }; + ctx_run_task(Ctx::opaque_ref(ctx), global); Some(done) } @@ -231,16 +232,10 @@ pub mod random { }; const MAX_RANGE: i64 = 0xffff_ffff_ffff; - impl JobContext for RandomFillJob { - type OffThread = Self; - type Js = RandomFillJs; - - fn run( - this: &mut Self, - vm: &Borrow, - done: bun_jsc::Completion, - ) -> Option> { - match this { + impl RandomFillJob { + /// Pool thread: fills the scratch buffer, or the ArrayBuffer itself. + fn fill(&mut self, vm: &Borrow) { + match self { RandomFillJob::Scratch { scratch, .. } => boringssl::rand_bytes(scratch), RandomFillJob::InPlace { bytes, length } => { // SAFETY: `bytes` points into the ArrayBuffer `value` keeps alive; @@ -255,6 +250,21 @@ pub mod random { boringssl::rand_bytes(slice); } } + } + } + + impl JobContext for RandomFillJob { + type OffThread = Self; + type Js = RandomFillJs; + + unsafe fn run( + this: *mut Self, + vm: &Borrow, + done: bun_jsc::Completion, + ) -> Option> { + // SAFETY: fn contract; the job is not handed on, so the reborrow is + // exclusive for the call. + unsafe { (*this).fill(vm) }; Some(done) } @@ -1062,14 +1072,18 @@ mod _impl { type OffThread = Self; type Js = ScryptJs; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, vm: &Borrow, done: bun_jsc::Completion, ) -> Option> { - // SAFETY: `result` is `buf`'s backing store (kept by the Js side); VM alive under the borrow. - let key = unsafe { this.result.under_borrow(vm) }; - this.err = this.params.run_task_impl(key); + // SAFETY: fn contract; the job is not handed on, so the reborrows are + // exclusive for their statements. `result` is `buf`'s backing store + // (kept by the Js side); VM alive under the borrow. + unsafe { + let key = (*this).result.under_borrow(vm); + (*this).err = (*this).params.run_task_impl(key); + } Some(done) } diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index f0941840d3d2..3487db3d3423 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -1252,13 +1252,18 @@ mod _async_tasks { type OffThread = Self; type Js = AsyncFSJs; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { let mut node_fs = NodeFS::default(); - this.result = NodeFS::dispatch::(&mut node_fs, &this.args, Flavor::Async); + // SAFETY: fn contract; the job is not handed on, so the reborrows are + // exclusive for the statement. + unsafe { + (*this).result = + NodeFS::dispatch::(&mut node_fs, &(*this).args, Flavor::Async); + } // `sys::Error::path` is `Box<[u8]>` boxed at the `errno_sys_p` // construction site, so no clone is needed — `node_fs` may drop. Some(done) @@ -2183,23 +2188,27 @@ mod _async_tasks { type OffThread = Self; type Js = AsyncFSJs; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - this.done = Some(done); let mut buf = PathBuffer::uninit(); - let root_path_z = { - let bytes: &'static [u8] = - // SAFETY: `root_path` is a NUL-terminated `Box<[u8]>` fixed for the - // task's lifetime; `perform_work` mutates other fields only. - unsafe { bun_ptr::detach_lifetime(&this.root_path[..]) }; + // SAFETY: fn contract. `root_path` is a NUL-terminated `Box<[u8]>` + // fixed for the task's lifetime; `perform_work` mutates other fields + // only. + let root_path_z = unsafe { + (*this).done = Some(done); + let root_path: &[u8] = &(*this).root_path; + let bytes: &'static [u8] = bun_ptr::detach_lifetime(root_path); ZStr::from_buf(bytes, bytes.len() - 1) }; // May finish synchronously (no subdirectories) or fan out; the last - // subtask finishes the token. - this.perform_work(root_path_z, &mut buf, true); + // subtask finishes the token. The hand-over happens inside this call: + // `perform_work` and the steps under it still take `&mut self`, so + // this frame only stops adding a protected reference of its own. + // SAFETY: fn contract; nothing touches `*this` here afterwards. + unsafe { (*this).perform_work(root_path_z, &mut buf, true) }; None } diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index 006cf841e312..3a8d20642c30 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -6846,6 +6846,17 @@ bun_jsc::jsc_host_abi! { // FileOpener / FileCloser // ────────────────────────────────────────────────────────────────────────── +/// [`FileOpener::get_fd`]'s continuation: gets the fd, or `Fd::INVALID` with +/// `errno` / `system_error` set, and takes the task over. It ends by handing +/// the task on (`ReadFile`, `WriteFile`) or freeing it (`ReadFileUV`), hence a +/// pointer rather than a `&mut` that would still be protected then, here and +/// in the `get_fd` frames that invoke it (as for [`bun_jsc::JobContext::run`]). +/// +/// # Safety +/// `this` is the live task `get_fd` was given; the caller does not touch it +/// afterwards. +pub type OpenCallback = unsafe fn(this: *mut T, fd: Fd); + // TODO: move to bun_sys? /// Generic file-open helper used by ReadFile/WriteFile/CopyFile state machines, /// modeled as a trait the target implements. @@ -6880,11 +6891,14 @@ pub trait FileOpener: Sized { /// 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)); + fn set_open_callback(&mut self, cb: OpenCallback); #[cfg(windows)] - fn open_callback(&self) -> fn(&mut Self, Fd); + fn open_callback(&self) -> OpenCallback; - fn get_fd_by_opening(&mut self, callback: fn(&mut Self, Fd)) { + /// Opens `pathlike()`; returns the fd it recorded in `opened_fd`, or + /// `Fd::INVALID` after recording `errno` / `system_error`. + #[cfg(not(windows))] + fn open_pathlike(&mut self) -> Fd { let mut buf = bun_paths::PathBuffer::uninit(); let path_string = match self.pathlike() { PathOrFileDescriptor::Path(p) => p.clone(), @@ -6892,61 +6906,97 @@ pub trait FileOpener: Sized { }; let path = path_string.slice_z(&mut buf); + 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); + return fd; + } + 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); + return Fd::INVALID; + } + 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); + return Fd::INVALID; + } + } + } + } + + /// [`get_fd`](Self::get_fd) for a task whose `pathlike()` is a path. + /// + /// # Safety + /// As [`get_fd`](Self::get_fd). + unsafe fn get_fd_by_opening(this: *mut Self, callback: OpenCallback) { #[cfg(windows)] { use bun_sys::ReturnCodeExt as _; - // Monomorphic libuv completion thunk — recovers `*mut Self` from - // `req.data`. + // Monomorphic libuv completion thunk; `req.data` carries the task. 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 }; + // SAFETY: `req` is the request queued below, `data` the task it + // belongs to, untouched since. The request is finished with + // before the task is touched; `cb` takes the task over last. + unsafe { + let this = (*req).data.cast::(); + let result = (*req).result; + bun_libuv_sys::uv_fs_req_cleanup(req); if let Some(err_enum) = result.err_enum_e() { - let path_string_2 = match self_.pathlike() { + let path_string = match (*this).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( + (*this).set_errno(bun_errno::from_errno(err_enum as i32).into()); + (*this).set_system_error( bun_sys::Error::from_code(err_enum, bun_sys::Tag::open) - .with_path(path_string_2.slice()) + .with_path(path_string.slice()) .to_system_error() .into(), ); - self_.set_opened_fd(bun_sys::Fd::INVALID); + (*this).set_opened_fd(bun_sys::Fd::INVALID); } else { - self_.set_opened_fd(Fd::from_uv(result.to_fd())); + (*this).set_opened_fd(Fd::from_uv(result.to_fd())); } + let cb = (*this).open_callback(); + let fd = (*this).opened_fd(); + cb(this, fd); } - 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 mut buf = bun_paths::PathBuffer::uninit(); + // SAFETY: fn contract; the reborrow ends with the call. + let path_string = match unsafe { (*this).pathlike() } { + PathOrFileDescriptor::Path(p) => p.clone(), + PathOrFileDescriptor::Fd(_) => unreachable!(), + }; + let path = path_string.slice_z(&mut buf); + + // SAFETY: fn contract; each reborrow ends with its accessor call. + // `req` is the task's own request, untouched from here until + // `wrapped_callback`; `data` is set before queueing because a + // synchronous failure runs `callback` (which may free the task). let rc = unsafe { + (*this).set_open_callback(callback); + let loop_ = (*this).loop_(); + let req: *mut bun_libuv_sys::uv_fs_t = (*this).req(); + (*req).data = this.cast(); bun_libuv_sys::uv_fs_open( loop_, req, @@ -6957,75 +7007,53 @@ pub trait FileOpener: Sized { ) }; 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; + // SAFETY: fn contract; libuv did not keep the request, and + // `callback` is the last access. + unsafe { + (*this).set_errno(bun_errno::from_errno(errno as i32).into()); + (*this).set_system_error( + bun_sys::Error::from_code(errno, bun_sys::Tag::open) + .with_path(path_string.slice()) + .to_system_error() + .into(), + ); + (*this).set_opened_fd(bun_sys::Fd::INVALID); + callback(this, bun_sys::Fd::INVALID); + } } - 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 => {} - } - } - 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()); + // SAFETY: fn contract; the reborrow ends with the call. + let fd = unsafe { (*this).open_pathlike() }; + // SAFETY: fn contract, passed through; this is the last access. + unsafe { callback(this, fd) } } } - fn get_fd(&mut self, callback: fn(&mut Self, Fd)) { - if self.opened_fd() != Fd::INVALID { - callback(self, self.opened_fd()); - return; - } + /// Finds the task's fd (already opened, the store's own, or by opening the + /// path) and continues with `callback`, which takes the task over. + /// + /// # Safety + /// `this` is the live task and nothing else is using it; the caller does + /// not touch it afterwards ([`OpenCallback`] hands it on or frees it). + unsafe fn get_fd(this: *mut Self, callback: OpenCallback) { + // SAFETY: fn contract; each reborrow ends with its accessor call, and + // `callback` is the last access on the paths that run it. + unsafe { + let fd = (*this).opened_fd(); + if fd != Fd::INVALID { + return callback(this, fd); + } - if let PathOrFileDescriptor::Fd(fd) = self.pathlike() { - let fd = *fd; - self.set_opened_fd(fd); - callback(self, fd); - return; - } + if let PathOrFileDescriptor::Fd(fd) = *(*this).pathlike() { + (*this).set_opened_fd(fd); + return callback(this, fd); + } - self.get_fd_by_opening(callback); + Self::get_fd_by_opening(this, callback) + } } } diff --git a/src/runtime/webcore/CompressionStreamCoder.rs b/src/runtime/webcore/CompressionStreamCoder.rs index 647a3387cd5c..486898e3e3f4 100644 --- a/src/runtime/webcore/CompressionStreamCoder.rs +++ b/src/runtime/webcore/CompressionStreamCoder.rs @@ -834,14 +834,20 @@ impl bun_jsc::JobContext for CompressionAsyncCtx { type OffThread = Self; type Js = CompressionAsyncJs; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - // SAFETY: `coder` is kept alive by the reference this ctx holds (the - // cell's finalizer only releases its own); see the field doc. - this.error = unsafe { (*this.coder).transform(this.input.slice(), this.finish) }.err(); + // SAFETY: fn contract; the job is not handed on, so the reborrows are + // exclusive for the statement. `coder` is kept alive by the reference + // this ctx holds (the cell's finalizer only releases its own); see the + // field doc. + unsafe { + (*this).error = (*(*this).coder) + .transform((*this).input.slice(), (*this).finish) + .err(); + } Some(done) } diff --git a/src/runtime/webcore/blob/copy_file.rs b/src/runtime/webcore/blob/copy_file.rs index a4b016417992..636b0fb2741f 100644 --- a/src/runtime/webcore/blob/copy_file.rs +++ b/src/runtime/webcore/blob/copy_file.rs @@ -75,12 +75,14 @@ unsafe impl Send for CopyFile {} impl jsc::JobContext for CopyFile { type OffThread = Self; type Js = jsc::JSPromiseStrong; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - this.run_async(); + // SAFETY: fn contract; the copy is synchronous and the job is not handed + // on, so the reborrow is exclusive for the call. + unsafe { (*this).run_async() }; Some(done) } fn then( diff --git a/src/runtime/webcore/blob/read_file.rs b/src/runtime/webcore/blob/read_file.rs index 8bc36a6b5805..9055c4344ed1 100644 --- a/src/runtime/webcore/blob/read_file.rs +++ b/src/runtime/webcore/blob/read_file.rs @@ -10,6 +10,8 @@ use crate::Error; use crate::webcore::Lifetime; #[cfg(not(windows))] use crate::webcore::blob::ClosingState; +#[cfg(windows)] +use crate::webcore::blob::OpenCallback; use crate::webcore::blob::store::{Bytes as ByteStore, Data, File as FileStore}; use crate::webcore::blob::{Blob, FileCloser, FileOpener, MAX_SIZE, SizeType, StoreRef}; use crate::webcore::node_types::PathOrFileDescriptor; @@ -223,13 +225,13 @@ impl bun_jsc::JobContext for ReadFile { /// Where the bytes go: completed by `then`, or cancelled when the VM releases the JS sides of /// its live jobs at teardown (a refused or unrun read then frees only this off-thread part). type Js = ReadFileCompletionFns; - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - // Starts the read; finishes from the io loop via the token. - this.run(done); + // SAFETY: fn contract, passed through; `run_async` hands `*this` on. + unsafe { ReadFile::run_async(this, done) }; None } fn then( @@ -292,6 +294,15 @@ pub struct ReadFile { bun_threading::intrusive_work_task!(ReadFile, task); bun_io::intrusive_io_request!(ReadFile, io_request); +/// The step `run_async_with_fd` performs once `prepare_read`'s `&mut self` has ended. +#[cfg(not(windows))] +#[derive(Clone, Copy)] +enum Next { + ReadLoop, + WaitForReadable, + Finish, +} + // The default methods on the FileOpener/FileCloser traits provide the bodies. impl FileOpener for ReadFile { fn opened_fd(&self) -> Fd { @@ -318,11 +329,11 @@ impl FileOpener for ReadFile { unreachable!("ReadFile is POSIX-only; see ReadFileUV") } #[cfg(windows)] - fn set_open_callback(&mut self, _cb: fn(&mut Self, Fd)) { + fn set_open_callback(&mut self, _cb: OpenCallback) { unreachable!() } #[cfg(windows)] - fn open_callback(&self) -> fn(&mut Self, Fd) { + fn open_callback(&self) -> OpenCallback { unreachable!() } } @@ -599,26 +610,31 @@ impl ReadFile { Ok(()) } - pub(crate) fn run(&mut self, task: ReadFileTask) { - self.run_async(task); - } - - fn run_async(&mut self, task: ReadFileTask) { + /// First pool step: keeps the token and starts the read; `get_fd` hands `*this` on. + /// + /// # Safety + /// [`bun_jsc::JobContext::run`]'s contract. + unsafe fn run_async(this: *mut Self, task: ReadFileTask) { #[cfg(windows)] { // Windows reads go through ReadFileUV, never the pool. + let _ = this; let _ = task; unreachable!("ReadFile on the work pool (Windows uses ReadFileUV)"); } #[cfg(not(windows))] { - self.io_task = Some(task); + // SAFETY: fn contract; no reference outlives the block. + unsafe { + (*this).io_task = Some(task); - if self.file_store.pathlike.is_fd() { - self.opened_fd = self.file_store.pathlike.fd(); + if (*this).file_store.pathlike.is_fd() { + (*this).opened_fd = (*this).file_store.pathlike.fd(); + } } - self.get_fd(Self::run_async_with_fd); + // SAFETY: fn contract, passed through. + unsafe { Self::get_fd(this, Self::run_async_with_fd) } } } @@ -700,16 +716,35 @@ impl ReadFile { } } + /// The read's continuation: `prepare_read`'s reborrow has ended by the + /// time the step it chose hands `*this` on. + /// + /// # Safety + /// [`OpenCallback`](crate::webcore::blob::OpenCallback)'s contract. + #[cfg(not(windows))] + unsafe fn run_async_with_fd(this: *mut Self, fd: Fd) { + // SAFETY: fn contract; the reborrow ends with the call. + let next = unsafe { (*this).prepare_read(fd) }; + // SAFETY: fn contract; the step is this thread's last access. + unsafe { + match next { + Next::ReadLoop => (*this).do_read_loop(), + Next::WaitForReadable => (*this).wait_for_readable(), + Next::Finish => (*this).on_finish(), + } + } + } + + /// Stat, buffer sizing and the initial readability check. #[cfg(not(windows))] - fn run_async_with_fd(&mut self, fd: Fd) { + fn prepare_read(&mut self, fd: Fd) -> Next { if self.errno.is_some() { - self.on_finish(); - return; + return Next::Finish; } self.resolve_size_and_last_modified(fd); if self.errno.is_some() { - return self.on_finish(); + return Next::Finish; } // Special files might report a size of > 0, and be wrong. @@ -720,8 +755,7 @@ impl ReadFile { // default — `then()` reads `self.buffer` directly. self.byte_store = ByteStore::default(); - self.on_finish(); - return; + return Next::Finish; } // add an extra 16 bytes to the buffer to avoid having to resize it for trailing extra data @@ -735,8 +769,7 @@ impl ReadFile { .to_system_error() .into(), ); - self.on_finish(); - return; + return Next::Finish; } self.buffer = v; } @@ -753,14 +786,11 @@ impl ReadFile { // // If we immediately call read(), it will block until stdin is // readable. - if self.could_block { - if bun_core::is_readable(fd) == bun_core::Pollable::NotReady { - self.wait_for_readable(); - return; - } + if self.could_block && bun_core::is_readable(fd) == bun_core::Pollable::NotReady { + return Next::WaitForReadable; } - self.do_read_loop(); + Next::ReadLoop } fn do_read_loop_task(task: *mut WorkPoolTask) { @@ -924,7 +954,7 @@ pub struct ReadFileUV<'a> { pub(crate) req: libuv::fs_t, /// Stash for the open completion callback across the libuv async hop. - open_callback: fn(&mut Self, Fd), + open_callback: OpenCallback, } #[cfg(windows)] @@ -950,10 +980,10 @@ impl<'a> FileOpener for ReadFileUV<'a> { 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)) { + fn set_open_callback(&mut self, cb: OpenCallback) { self.open_callback = cb; } - fn open_callback(&self) -> fn(&mut Self, Fd) { + fn open_callback(&self) -> OpenCallback { self.open_callback } } @@ -1062,10 +1092,9 @@ impl<'a> ReadFileUV<'a> { // 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; + // SAFETY: freshly boxed and nothing else holds it; from here it belongs + // to the libuv request chain, until `finalize` frees it. + unsafe { Self::get_fd(this_ptr, Self::on_file_open) }; } pub fn finalize(this: *mut Self) { @@ -1127,40 +1156,41 @@ impl<'a> ReadFileUV<'a> { Self::finalize(core::ptr::from_mut(self)); } - pub(crate) fn on_file_open(&mut self, opened_fd: Fd) { + /// The read's continuation: queues the fstat, or finishes (freeing the task). + /// + /// # Safety + /// [`OpenCallback`]'s contract. + unsafe fn on_file_open(this: *mut Self, opened_fd: Fd) { log!("ReadFileUV.onFileOpen"); - if self.errno.is_some() { - self.on_finish(); - return; - } + // SAFETY: fn contract; `on_finish` is the last access on its paths. The + // FFI call gets the live VM loop and the task's own deinit'd `fs_t`, + // whose `data` lets `on_file_initial_stat` recover the task. + unsafe { + if (*this).errno.is_some() { + return (*this).on_finish(); + } + + (*this).req.deinit(); + (*this).req.data = this.cast::(); - self.req.deinit(); - self.req.data = core::ptr::from_mut(self).cast::(); - - // SAFETY: FFI — `loop_` is the live VM uv loop, `self.req` is a freshly - // deinit'd `fs_t` owned by `self`, `opened_fd.uv()` is the just-opened fd, - // and `on_file_initial_stat` is a valid `uv_fs_cb` that recovers `self` - // from `req.data` (set above). - let rc = unsafe { - libuv::uv_fs_fstat( - self.loop_, - &mut self.req, + let rc = libuv::uv_fs_fstat( + (*this).loop_, + &raw mut (*this).req, opened_fd.uv(), Some(Self::on_file_initial_stat), - ) - }; - if let Some(errno) = rc.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(), ); - self.on_finish(); - return; - } + if let Some(errno) = rc.err_enum_e() { + (*this).errno = Some(bun_errno::from_errno(errno as i32).into()); + (*this).system_error = Some( + bun_sys::Error::from_code(errno, bun_sys::Tag::fstat) + .to_system_error() + .into(), + ); + return (*this).on_finish(); + } - self.req.data = core::ptr::from_mut(self).cast::(); + (*this).req.data = this.cast::(); + } } extern "C" fn on_file_initial_stat(req: *mut libuv::fs_t) { diff --git a/src/runtime/webcore/blob/write_file.rs b/src/runtime/webcore/blob/write_file.rs index 293d064020a5..1921af8dbf62 100644 --- a/src/runtime/webcore/blob/write_file.rs +++ b/src/runtime/webcore/blob/write_file.rs @@ -15,6 +15,8 @@ use bun_jsc::{self as jsc, JSGlobalObject, JSPromise, JSValue, JsTerminated, Sys use bun_sys::{self as sys, Fd}; use bun_threading::{IntrusiveWorkTask as _, WorkPool, WorkPoolTask}; +#[cfg(windows)] +use crate::webcore::blob::OpenCallback; use crate::webcore::blob::{ self, Blob, FileOpener, MkdirpTarget, Retry, SizeType, mkdir_if_not_exists, }; @@ -48,13 +50,13 @@ impl bun_jsc::JobContext for WriteFile { type OffThread = Self; /// The completion is delivered through `on_complete_callback(ctx, ..)`. type Js = (); - fn run( - this: &mut Self, + unsafe fn run( + this: *mut Self, _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - // Starts the write; finishes from the io loop via the token. - this.run(done); + // SAFETY: fn contract, passed through; `run_async` hands `*this` on. + unsafe { WriteFile::run_async(this, done) }; None } fn then(this: Self, _: (), cx: &bun_jsc::JsThread<'_>) -> jsc::JsResult<()> { @@ -98,6 +100,15 @@ pub struct WriteFile { bun_threading::intrusive_work_task!(WriteFile, task); bun_io::intrusive_io_request!(WriteFile, io_request); +/// The step `run_with_fd` performs once `prepare_write`'s `&mut self` has ended. +#[cfg(not(windows))] +#[derive(Clone, Copy)] +enum Next { + WriteLoop, + WaitForWritable, + Finish, +} + // ────────────────────────────────────────────────────────────────────────── // FileOpener / FileCloser // ────────────────────────────────────────────────────────────────────────── @@ -146,11 +157,11 @@ impl FileOpener for WriteFile { unreachable!("WriteFile is POSIX-only") } #[cfg(windows)] - fn set_open_callback(&mut self, _cb: fn(&mut Self, Fd)) { + fn set_open_callback(&mut self, _cb: OpenCallback) { unreachable!() } #[cfg(windows)] - fn open_callback(&self) -> fn(&mut Self, Fd) { + fn open_callback(&self) -> OpenCallback { unreachable!() } } @@ -353,25 +364,27 @@ impl WriteFile { Ok(()) } - pub(crate) fn run(&mut self, task: WriteFileTask) { + /// First pool step: keeps the token and starts the write; `get_fd` hands `*this` on. + /// + /// # Safety + /// [`bun_jsc::JobContext::run`]'s contract. + unsafe fn run_async(this: *mut Self, task: WriteFileTask) { #[cfg(windows)] { // Windows writes go through WriteFileWindows, never the pool. + let _ = this; let _ = task; unreachable!("WriteFile on the work pool (Windows uses WriteFileWindows)"); } #[cfg(not(windows))] { - self.io_task = Some(task); - self.run_async(); + // SAFETY: fn contract; a statement-scoped field write. + unsafe { (*this).io_task = Some(task) }; + // SAFETY: fn contract, passed through. + unsafe { Self::get_fd(this, Self::run_with_fd) } } } - #[cfg(not(windows))] - fn run_async(&mut self) { - self.get_fd(Self::run_with_fd); - } - #[cfg(not(windows))] pub(crate) fn is_allowed_to_close(&self) -> bool { self.file_blob @@ -400,11 +413,30 @@ impl WriteFile { } } + /// The write's continuation: `prepare_write`'s reborrow has ended by the + /// time the step it chose hands `*this` on. + /// + /// # Safety + /// [`OpenCallback`](crate::webcore::blob::OpenCallback)'s contract. #[cfg(not(windows))] - fn run_with_fd(&mut self, fd_: Fd) { + unsafe fn run_with_fd(this: *mut Self, fd: Fd) { + // SAFETY: fn contract; the reborrow ends with the call. + let next = unsafe { (*this).prepare_write(fd) }; + // SAFETY: fn contract; the step is this thread's last access. + unsafe { + match next { + Next::WriteLoop => (*this).do_write_loop(), + Next::WaitForWritable => (*this).wait_for_writable(), + Next::Finish => (*this).on_finish(), + } + } + } + + /// Blocking-ness, preallocation and the initial writability check. + #[cfg(not(windows))] + fn prepare_write(&mut self, fd_: Fd) -> Next { if fd_ == Fd::INVALID || self.errno.is_some() { - self.on_finish(); - return; + return Next::Finish; } let fd = self.opened_fd; @@ -448,8 +480,7 @@ impl WriteFile { // } if self.could_block && bun_core::is_writable(fd) == bun_core::Pollable::NotReady { - self.wait_for_writable(); - return; + return Next::WaitForWritable; } #[cfg(any(target_os = "linux", target_os = "android"))] @@ -469,7 +500,7 @@ impl WriteFile { } } - self.do_write_loop(); + Next::WriteLoop } fn do_write_loop_task(task: *mut WorkPoolTask) { diff --git a/test/internal/source-lints/self-receiver-job-start.test.ts b/test/internal/source-lints/self-receiver-job-start.test.ts new file mode 100644 index 000000000000..5fd2a8cce876 --- /dev/null +++ b/test/internal/source-lints/self-receiver-job-start.test.ts @@ -0,0 +1,358 @@ +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"; + +// The two entry points through which a pool job reaches the code that hands +// it to another thread must take the object by pointer, not by reference: +// +// - `JobContext::run` (src/jsc/job.rs), the trait declaration and every +// `impl .. JobContext for ..`: its first parameter is the job's off-thread +// part. `Job::run_on_pool` calls it; a body that keeps the `Completion` +// (ReadFile, WriteFile, the recursive readdir scan) publishes the object to +// the io thread or to further pool tasks, or finishes the token so the JS +// thread frees it, before it returns. +// - `FileOpener::get_fd` / `get_fd_by_opening` (src/runtime/webcore/Blob.rs) +// and the continuation they invoke, `OpenCallback`: the continuation +// takes the task over and ends the same way (ReadFile / WriteFile hand it +// to the io thread, ReadFileUV frees it), so neither the continuation's +// own parameter nor the `get_fd` frame invoking it may be a reference. +// +// A reference passed as an argument is protected until the call returns, under +// both aliasing models (Tree Borrows is what `bun run rust:miri` uses). Once +// the object has been handed on, the thread that finishes it reads and frees +// the job through the job's own pointer (`Job::complete`), which is foreign to +// every reference still protected on the publishing thread's stack, and +// `ReadFileUV` frees itself outright; a foreign access to a protected +// reference's memory, and any deallocation of it, is UB whether or not the +// reference is used again. Every frame between `run_on_pool` and the hand-over +// used to add one such reference (`C::run(&mut (*job).off, ..)` -> +// `ReadFile::run(&mut self)` -> `get_fd(&mut self, ..)` -> `callback(self, fd)` +// -> `run_async_with_fd(&mut self)`), and the JS thread only has to get to the +// completion before this thread has returned through them, which for a read +// that finishes in its first step (an empty file, an open error) is the normal +// case. The converted chain carries `*mut`, does its own work through reborrows +// scoped to a statement or to a `&mut self` helper that returns before the +// hand-over (`prepare_read` / `prepare_write`), and makes the hand-over its +// last access; `ReadFile::run_async` and `FileOpener::get_fd` are the templates. +// +// Scope, four checks: +// 1. the first parameter of `fn run` inside a `JobContext` trait or impl block; +// 2. the first parameter of `get_fd` / `get_fd_by_opening` inside the +// `FileOpener` trait block; +// 3. the definition of `OpenCallback`, which has to read +// `unsafe fn(*mut T, Fd)`: every continuation (`run_async_with_fd`, +// `run_with_fd`, `ReadFileUV::on_file_open`) is passed as a fn item where +// this alias is expected, so its signature is what holds theirs to `*mut`; +// 4. as a net under 3, a continuation type spelled out as `fn(&mut X, Fd)` +// anywhere (the pre-conversion spelling, in whatever parameter syntax). +// Each anchored check also records what it examined and asserts it found the +// declarations it is about, so renaming `run` / `JobContext` / `FileOpener` / +// `OpenCallback` fails here (update the lint) instead of emptying it. +// +// Not covered: the steps below these entry points. For ReadFile / WriteFile +// those are `wait_for_*`, `on_finish`, `do_close` and the loops; for the +// recursive readdir scan they are `perform_work` and everything under it, which +// still take `&mut self` and perform the hand-over inside that borrow, so for +// that job only the entry frame is converted. Those conversions carry their own +// guards. +// +// Siblings: self-receiver-reclaim.test.ts (freeing the receiver), +// fn-long-mut-reborrow.test.ts, frozen-nonnull-reborrow.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)); +})(); + +/** One check's findings in one piece of source: byte offsets of what it examined and of what it rejects. */ +interface Scan { + checked: { offset: number; name: string }[]; + offenders: number[]; +} + +// The line an `impl` / `trait` item starts on. A rustfmt-wrapped header puts +// the trait name a line or two below this, so the block's indentation is read +// from here, and the block ends at the first `}` back on that indentation. +const ITEM_START = /^[ \t]*(?:pub(?:\([^)]*\))?[ \t]+)?(?:unsafe[ \t]+)?(?:impl|trait)\b/gm; + +/** The text of the trait / impl block whose header contains `headerIndex`, and where it starts. */ +function itemBlock(stripped: string, headerIndex: number): { start: number; block: string } | null { + const lineEnd = stripped.indexOf("\n", headerIndex); + const upToHeaderLine = stripped.slice(0, lineEnd === -1 ? stripped.length : lineEnd); + let item: RegExpExecArray | null = null; + for (const m of upToHeaderLine.matchAll(ITEM_START)) item = m; + if (item === null) return null; + const indent = /^[ \t]*/.exec(item[0])![0]; + const rest = stripped.slice(headerIndex); + const end = rest.search(new RegExp(`^${indent}\\}`, "m")); + return { start: headerIndex, block: end === -1 ? rest : rest.slice(0, end) }; +} + +// A first parameter that is a raw pointer: `this: *mut Self`, `off: *mut +// Self::OffThread`, `ctx: *mut C`, `_: *mut ()`. Anything else (a `&mut self` +// receiver, `this: &mut Self`, `ctx: &mut C`) is the banned shape. +const POINTER_PARAM = /^(?:mut\s+)?\w+\s*:\s*\*mut\b/; + +/** `fn (` (name restricted to `names`) followed by its first parameter. */ +function fnWithFirstParam(names: string): RegExp { + return new RegExp(String.raw`\bfn\s+(${names})\s*\(\s*([^,)]*)`, "g"); +} + +/** Checks 1 and 2: the named fns inside every block introduced by `header`. */ +function scanEntries(stripped: string, header: RegExp, fns: string): Scan { + const scan: Scan = { checked: [], offenders: [] }; + for (const h of stripped.matchAll(header)) { + const item = itemBlock(stripped, h.index); + if (item === null) continue; + for (const f of item.block.matchAll(fnWithFirstParam(fns))) { + const offset = item.start + f.index; + scan.checked.push({ offset, name: f[1] }); + if (!POINTER_PARAM.test(f[2].trim())) scan.offenders.push(offset); + } + } + return scan; +} + +// 1. `JobContext::run`: the declaration and every implementation. +const JOB_CONTEXT = /\btrait\s+JobContext\b|\bJobContext\s+for\b/g; +function scanJobRun(stripped: string): Scan { + return scanEntries(stripped, JOB_CONTEXT, "run"); +} + +// 2. `FileOpener::get_fd` / `get_fd_by_opening`, the frames that invoke the continuation. +const FILE_OPENER = /\btrait\s+FileOpener\b/g; +function scanFileOpener(stripped: string): Scan { + return scanEntries(stripped, FILE_OPENER, "get_fd|get_fd_by_opening"); +} + +// 3. The alias itself. `[^;]*` spans a rustfmt-wrapped right-hand side. +const OPEN_CALLBACK_DEF = /\btype\s+OpenCallback\s*<[^>]*>\s*=\s*([^;]*);/g; +const POINTER_FN_TYPE = /^unsafe\s+fn\s*\(\s*(?:\w+\s*:\s*)?\*mut\b/; +function scanOpenCallbackDef(stripped: string): Scan { + const scan: Scan = { checked: [], offenders: [] }; + for (const m of stripped.matchAll(OPEN_CALLBACK_DEF)) { + scan.checked.push({ offset: m.index, name: "OpenCallback" }); + if (!POINTER_FN_TYPE.test(m[1].trim())) scan.offenders.push(m.index); + } + return scan; +} + +// 4. A continuation type taking the task by reference, with or without +// parameter names and however `Fd` is qualified: `fn(&mut Self, Fd)`, +// `fn(this: &mut T, fd: bun_sys::Fd)`. +const OPEN_CONTINUATION_BY_REF = + /\bfn\s*\(\s*(?:\w+\s*:\s*)?&\s*(?:'\w+\s+)?mut\s+\w+\s*,\s*(?:\w+\s*:\s*)?(?:[\w:]+::)?Fd\s*\)/g; +function openContinuationOffenders(stripped: string): number[] { + return [...stripped.matchAll(OPEN_CONTINUATION_BY_REF)].map(m => m.index); +} + +function lineOf(text: string, offset: number): number { + return text.slice(0, offset).split("\n").length; +} + +const found = { jobRun: [] as string[], fileOpener: [] as string[], openCallbackDef: [] as string[] }; +const offenders = { + jobRun: [] as string[], + fileOpener: [] as string[], + openCallbackDef: [] as string[], + openContinuation: [] as 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 (including the in-tree comments + // describing this hazard) don't count. `[ \t]*`, not `\s*`: `\s` crosses + // newlines and would swallow blank lines, shifting the reported line numbers. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + const at = (offset: number) => `${source}:${lineOf(stripped, offset)}`; + for (const [check, scan] of [ + ["jobRun", scanJobRun(stripped)], + ["fileOpener", scanFileOpener(stripped)], + ["openCallbackDef", scanOpenCallbackDef(stripped)], + ] as const) { + for (const c of scan.checked) found[check].push(check === "fileOpener" ? `${source} ${c.name}` : at(c.offset)); + for (const offset of scan.offenders) offenders[check].push(at(offset)); + } + for (const offset of openContinuationOffenders(stripped)) offenders.openContinuation.push(at(offset)); +} + +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 bans below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("the JobContext::run pattern matches the banned shapes and nothing else", () => { + const impl = (params: string, fn = "fn") => + `impl bun_jsc::JobContext for Foo {\n type OffThread = Self;\n type Js = ();\n ${fn} run(\n ${params}\n done: Completion,\n ) -> Option> {\n Some(done)\n }\n}\n`; + const banned = [ + // The declaration and the implementations as they were. + "pub trait JobContext: Sized + 'static {\n type OffThread: Send;\n fn run(\n off: &mut Self::OffThread,\n vm: &Borrow,\n ) -> Option>;\n}\n", + impl("this: &mut Self,\n _vm: &Borrow,"), + "impl bun_jsc::JobContext for AsyncTask {\n type OffThread = C;\n fn run(ctx: &mut C, _vm: &Borrow, done: Completion) -> Option> {\n Some(done)\n }\n}\n", + // `Never`. + "impl JobContext for Never {\n fn run(_: &mut (), _: &Borrow, done: Completion) -> Option> {\n Some(done)\n }\n}\n", + // A rustfmt-wrapped header with a where clause (`AsyncFSTask`): the block + // is bounded by the `impl` line's indentation, not the header line's. + " impl\n bun_jsc::JobContext for AsyncFSTask\n where\n Op<{ F }>: NodeFSDispatch,\n {\n type OffThread = Self;\n fn run(\n this: &mut Self,\n _vm: &Borrow,\n ) -> Option> {\n Some(done)\n }\n }\n", + // A pointer that is not the first parameter does not help. + impl("this: &mut Self,\n extra: *mut u8,", "unsafe fn"), + ]; + const allowed = [ + // The converted shapes. + "pub trait JobContext: Sized + 'static {\n type OffThread: Send;\n unsafe fn run(\n off: *mut Self::OffThread,\n vm: &Borrow,\n ) -> Option>;\n}\n", + impl("this: *mut Self,\n _vm: &Borrow,", "unsafe fn"), + "impl bun_jsc::JobContext for AsyncTask {\n unsafe fn run(ctx: *mut C, _vm: &Borrow, done: Completion) -> Option> {\n Some(done)\n }\n}\n", + "impl JobContext for Never {\n unsafe fn run(_: *mut (), _: &Borrow, done: Completion) -> Option> {\n Some(done)\n }\n}\n", + // The `&mut self` helper the implementation delegates to lives outside the + // impl block, before or after it, and is not what this lint is about. + "impl Foo {\n fn run(&mut self) {}\n}\n\n" + impl("this: *mut Self,\n _vm: &Borrow,", "unsafe fn"), + impl("this: *mut Self,\n _vm: &Borrow,", "unsafe fn") + "\nimpl Foo {\n fn run(&mut self) {}\n}\n", + ]; + expect(banned.map(s => scanJobRun(s).offenders.length)).toEqual(banned.map(() => 1)); + expect(allowed.map(s => scanJobRun(s).offenders.length)).toEqual(allowed.map(() => 0)); + // Every fixture above contains exactly one `run` this check is about, and + // an unrelated trait's `run` is not examined at all. + expect([...banned, ...allowed].map(s => scanJobRun(s).checked.length)).toEqual([...banned, ...allowed].map(() => 1)); + expect(scanJobRun("impl TaskContext for Foo {\n fn run(&mut self) {}\n}\n")).toEqual({ + checked: [], + offenders: [], + }); +}); + +test("the FileOpener entry-point pattern matches the banned shapes and nothing else", () => { + const opener = (body: string) => `pub trait FileOpener: Sized {\n fn opened_fd(&self) -> Fd;\n${body}}\n`; + const banned = [ + // `get_fd` / `get_fd_by_opening` as they were. + opener( + " fn get_fd(&mut self, callback: fn(&mut Self, Fd)) {\n callback(self, self.opened_fd());\n }\n", + ), + opener(" fn get_fd_by_opening(&mut self, callback: fn(&mut Self, Fd)) {}\n"), + // A pointer-typed continuation invoked from a frame that still holds a reference. + opener( + " unsafe fn get_fd(&mut self, callback: OpenCallback) {\n unsafe { callback(self, fd) }\n }\n", + ), + opener(" unsafe fn get_fd(this: &mut Self, callback: OpenCallback) {}\n"), + ]; + const allowed = [ + opener(" unsafe fn get_fd(this: *mut Self, callback: OpenCallback) {}\n"), + opener( + " #[cfg(not(windows))]\n unsafe fn get_fd_by_opening(this: *mut Self, callback: OpenCallback) {}\n", + ), + ]; + expect(banned.map(s => scanFileOpener(s).offenders.length)).toEqual(banned.map(() => 1)); + expect(allowed.map(s => scanFileOpener(s).offenders.length)).toEqual(allowed.map(() => 0)); + expect([...banned, ...allowed].map(s => scanFileOpener(s).checked.length)).toEqual( + [...banned, ...allowed].map(() => 1), + ); + // The accessors the entry points call may take `self` (they return before + // the hand-over), and a `get_fd` outside the trait block (the sinks have + // one) is something else: neither is examined. + const notExamined = [ + opener( + " fn set_opened_fd(&mut self, fd: Fd);\n fn open_pathlike(&mut self) -> Fd {\n Fd::INVALID\n }\n", + ), + "impl Sink {\n fn get_fd(&self) -> i32 {\n self.fd\n }\n}\n", + opener("") + "\nimpl Reader {\n fn get_fd(&self) -> Fd {\n self.fd\n }\n}\n", + ]; + expect(notExamined.map(s => scanFileOpener(s))).toEqual(notExamined.map(() => ({ checked: [], offenders: [] }))); +}); + +test("the OpenCallback patterns match the banned shapes and nothing else", () => { + const bannedDefs = [ + // The alias pointed back at the old continuation shape, in any spelling. + "pub type OpenCallback = fn(&mut T, Fd);", + "pub type OpenCallback = unsafe fn(this: &mut T, fd: Fd);", + "pub type OpenCallback =\n unsafe fn(this: &mut T, fd: bun_sys::Fd);", + // A safe fn over the pointer: callers could then pass anything. + "pub type OpenCallback = fn(*mut T, Fd);", + ]; + const allowedDefs = [ + "pub type OpenCallback = unsafe fn(this: *mut T, fd: Fd);", + "pub(crate) type OpenCallback = unsafe fn(*mut T, Fd);", + "pub type OpenCallback =\n unsafe fn(this: *mut T, fd: bun_sys::Fd);", + ]; + expect(bannedDefs.map(s => scanOpenCallbackDef(s).offenders.length)).toEqual(bannedDefs.map(() => 1)); + expect(allowedDefs.map(s => scanOpenCallbackDef(s).offenders.length)).toEqual(allowedDefs.map(() => 0)); + expect([...bannedDefs, ...allowedDefs].map(s => scanOpenCallbackDef(s).checked.length)).toEqual( + [...bannedDefs, ...allowedDefs].map(() => 1), + ); + // Uses of the alias, and other callback aliases, are not definitions of it. + const notDefs = [ + "open_callback: OpenCallback,", + "pub type RequestCallback = unsafe fn(*mut Request) -> Action;", + ]; + expect(notDefs.map(s => scanOpenCallbackDef(s))).toEqual(notDefs.map(() => ({ checked: [], offenders: [] }))); + + const bannedSpellings = [ + "fn get_fd(&mut self, callback: fn(&mut Self, Fd)) {", + "fn set_open_callback(&mut self, cb: fn(&mut Self, Fd));", + "fn open_callback(&self) -> fn(&mut Self, Fd);", + "open_callback: fn(&mut Self, Fd),", + "open_callback: fn(&'a mut Self, Fd),", + "cb: fn( &mut Self , Fd ),", + // Named parameters, a concrete task type, a qualified `Fd`. + "= unsafe fn(this: &mut T, fd: Fd);", + "open_callback: fn(&mut ReadFileUV, Fd),", + "cb: fn(&mut Self, bun_sys::Fd),", + ]; + const allowedSpellings = [ + "pub type OpenCallback = unsafe fn(this: *mut T, fd: Fd);", + "open_callback: OpenCallback,", + "cb: unsafe fn(*mut Self, Fd),", + // A predicate over the receiver, not a continuation that takes it over. + "validate: fn(&mut Self, usize) -> bool,", + "fn(&mut Self)", + // A function item is not a fn-pointer type. + "fn run_with_fd(&mut self, fd: Fd) {", + ]; + expect(bannedSpellings.map(s => openContinuationOffenders(s).length)).toEqual(bannedSpellings.map(() => 1)); + expect(allowedSpellings.map(s => openContinuationOffenders(s).length)).toEqual(allowedSpellings.map(() => 0)); +}); + +test("the anchored checks still find the declarations they are about", () => { + // If one of these goes empty or changes shape, the trait / alias was renamed + // or moved and the anchors above need updating, not the bans below. + expect(found.jobRun.some(entry => entry.startsWith("src/jsc/job.rs:"))).toBeTrue(); + expect(found.jobRun.length).toBeGreaterThanOrEqual(10); + expect(found.fileOpener.toSorted()).toEqual([ + "src/runtime/webcore/Blob.rs get_fd", + "src/runtime/webcore/Blob.rs get_fd_by_opening", + ]); + expect(found.openCallbackDef).toHaveLength(1); +}); + +test("every JobContext::run takes the off-thread part by pointer", () => { + expect(offenders.jobRun).toEqual([]); +}); + +test("FileOpener's entry points take the task by pointer", () => { + expect(offenders.fileOpener).toEqual([]); +}); + +test("OpenCallback is an unsafe fn over the task's pointer", () => { + expect(offenders.openCallbackDef).toEqual([]); +}); + +test("no continuation type takes the task by reference", () => { + expect(offenders.openContinuation).toEqual([]); +});