From 4bd3d46b2e38fd3c082b648e7469463aab8ea44a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:08:58 +0000 Subject: [PATCH 1/3] Pass the job to JobContext::run and FileOpener::get_fd by pointer JobContext::run took the job's off-thread part as &mut, and FileOpener::get_fd / get_fd_by_opening and their fn(&mut Self, Fd) continuation took the task as &mut as well. ReadFile and WriteFile hand the object on from inside that chain (to the io thread, or by finishing the completion token, after which the JS thread reads and frees the job through its own pointer), and ReadFileUV's continuation can free it, so every one of those reference arguments was still protected on the publishing thread's stack while that happened. Job::run_on_pool now passes &raw mut (*job).off to an unsafe fn run(off: *mut OffThread, ..); the synchronous implementations reborrow for their work (inline, or through a &mut self helper) and return the token as before. FileOpener::get_fd / get_fd_by_opening take this: *mut Self and an OpenCallback = unsafe fn(*mut Self, Fd), do their accessor calls through call-scoped reborrows and invoke the continuation last; the POSIX open loop moves into open_pathlike(&mut self). ReadFile::run_async / WriteFile::run_async take the pointer, and the continuations (run_async_with_fd / run_with_fd) decide the next step in prepare_read / prepare_write under a reborrow that ends before the step runs, then perform it through the pointer. ReadFileUV's on_file_open is the same continuation shape on Windows. test/internal/source-lints/self-receiver-job-start.test.ts bans the reference shapes: a JobContext run (declaration or impl) whose first parameter is not a pointer, FileOpener entry points taking self, and an open continuation spelled fn(&mut Self, Fd). --- src/jsc/JSSecrets.rs | 12 +- src/jsc/job.rs | 30 ++- src/runtime/api/Archive.rs | 8 +- src/runtime/api/BunObject.rs | 57 ++-- src/runtime/api/JSTranspiler.rs | 8 +- src/runtime/api/glob.rs | 33 ++- src/runtime/crypto/PBKDF2.rs | 41 +-- src/runtime/crypto/PasswordObject.rs | 8 +- src/runtime/dns_jsc/dns.rs | 8 +- src/runtime/image/Image.rs | 8 +- src/runtime/node/node_crypto_binding.rs | 58 ++-- src/runtime/node/node_fs.rs | 33 ++- src/runtime/webcore/Blob.rs | 240 ++++++++++------- src/runtime/webcore/CompressionStreamCoder.rs | 16 +- src/runtime/webcore/blob/copy_file.rs | 8 +- src/runtime/webcore/blob/read_file.rs | 169 +++++++----- src/runtime/webcore/blob/write_file.rs | 79 ++++-- .../self-receiver-job-start.test.ts | 252 ++++++++++++++++++ 18 files changed, 764 insertions(+), 304 deletions(-) create mode 100644 test/internal/source-lints/self-receiver-job-start.test.ts 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..94a0e6b0d0dc 100644 --- a/src/jsc/job.rs +++ b/src/jsc/job.rs @@ -238,9 +238,26 @@ 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 rather than `&mut` because a body that keeps `done` + /// hands the job on before it returns (to an io loop, to sub-tasks, or, by + /// finishing `done`, to the JS thread), and whoever ends up finishing it + /// makes the JS thread read and free the allocation through the job's own + /// pointer (`Job::complete`), possibly before this thread has returned + /// from here. A reference argument is protected for the whole call, and an + /// access through another pointer to memory a protected reference covers + /// is UB under the aliasing model whether or not the reference is used + /// again; a raw pointer makes no such claim. A body that completes + /// synchronously just reborrows for the work and returns `done`; one that + /// hands the job on does its own work through reborrows that end before the + /// hand-over, and makes the hand-over 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 +405,10 @@ 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 body handed the job on (it may already be freed), and + // nothing below touches `*this`: releasing `vm` goes through our own + // `handle` clone, not the job's. + if let Some(done) = unsafe { C::run(&raw mut (*this).off, &vm, done) } { drop(vm); done.finish(); } @@ -541,7 +561,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..d31fce9ad4c1 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,25 @@ 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, so this is the hand-over. + // 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..c1e7020539eb 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -6846,6 +6846,20 @@ bun_jsc::jsc_host_abi! { // FileOpener / FileCloser // ────────────────────────────────────────────────────────────────────────── +/// What [`FileOpener::get_fd`] continues with once the task's fd is known: the +/// fd, or `Fd::INVALID` with `errno` / `system_error` set when the open failed. +/// It takes the task over: the implementations end by handing it on +/// (`ReadFile`, `WriteFile`; the JS thread then reads and frees the job through +/// the job's own pointer) or by freeing it right there (`ReadFileUV`). Neither +/// is allowed while a `&mut Self` argument is still protected, i.e. until the +/// continuation has returned (see [`bun_jsc::JobContext::run`]); hence the +/// pointer, here and in the `get_fd` frames that invoke it. +/// +/// # Safety +/// `this` is the live task `get_fd` was given, and 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 +6894,15 @@ 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 the path in `pathlike()` and records the outcome: the fd in + /// `opened_fd`, or `Fd::INVALID` plus `errno` / `system_error`. Returns + /// what it recorded. + #[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 +6910,101 @@ 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 live request queued below, whose `data` + // is the task's pointer; the task was left alone until this + // completion. The request is done with before the task is + // touched, each reborrow of the task ends with its accessor + // call, and `cb` (which takes the task over) is the last access. + 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, so it is live for as long as + // the open is in flight, and nothing touches it from here until + // `wrapped_callback` runs. `req.data` is set before the open is + // queued because a synchronous failure runs `callback` (which may + // free the task) right below. 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 +7015,57 @@ 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. The + // reborrows end with their accessor calls, and `callback` is the + // last access to `*this`. + 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; nothing here touches `*this` + // afterwards. + 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 (`opened_fd` if it is already set, the descriptor + /// of an fd-backed `pathlike()`, or else 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, since `callback` hands it on or frees it (see + /// [`OpenCallback`]). + 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 to `*this` 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..8f4ec8e5640f 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,15 @@ 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); + // Starts the read, which hands `*this` on (to the io thread, or to the + // JS thread by finishing the token); nothing here touches it afterwards. + // SAFETY: fn contract, passed through. + unsafe { ReadFile::run_async(this, done) }; None } fn then( @@ -292,6 +296,16 @@ pub struct ReadFile { bun_threading::intrusive_work_task!(ReadFile, task); bun_io::intrusive_io_request!(ReadFile, io_request); +/// What [`ReadFile::prepare_read`] decided the read continues with; performed +/// by `run_async_with_fd` once that `&mut self` stage has returned. +#[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 +332,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 +613,33 @@ impl ReadFile { Ok(()) } - pub(crate) fn run(&mut self, task: ReadFileTask) { - self.run_async(task); - } - - fn run_async(&mut self, task: ReadFileTask) { + /// The job's first pool step: keeps the token and starts the read. From + /// `get_fd` on, `*this` belongs to whichever thread the read continues on. + /// + /// # Safety + /// [`bun_jsc::JobContext::run`]'s contract; the caller does not touch + /// `*this` afterwards. + 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 +721,38 @@ impl ReadFile { } } + /// The read's [`OpenCallback`](crate::webcore::blob::OpenCallback): decides + /// the next step under a reborrow that ends before the step runs, because + /// the step hands `*this` on (to the io thread, or to the JS thread, which + /// frees it). + /// + /// # Safety + /// `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; whichever step runs is the last access to + // `*this` on this thread. + 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 +763,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 +777,7 @@ impl ReadFile { .to_system_error() .into(), ); - self.on_finish(); - return; + return Next::Finish; } self.buffer = v; } @@ -753,14 +794,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 +962,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 +988,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 +1100,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 +1164,44 @@ 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 [`OpenCallback`]: queues the fstat, or finishes (which frees + /// the task) if the open failed or the fstat cannot be queued. + /// + /// # 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. Field accesses are statement-scoped, the + // `on_finish` calls are the last access on their paths, and the FFI + // call gets the live VM uv loop, the task's own freshly deinit'd `fs_t` + // (whose `data` lets `on_file_initial_stat` recover the task), and the + // just-opened fd. + 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..7608920dbd1a 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,15 @@ 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); + // Starts the write, which hands `*this` on (to the io thread, or to the + // JS thread by finishing the token); nothing here touches it afterwards. + // SAFETY: fn contract, passed through. + unsafe { WriteFile::run_async(this, done) }; None } fn then(this: Self, _: (), cx: &bun_jsc::JsThread<'_>) -> jsc::JsResult<()> { @@ -98,6 +102,16 @@ pub struct WriteFile { bun_threading::intrusive_work_task!(WriteFile, task); bun_io::intrusive_io_request!(WriteFile, io_request); +/// What [`WriteFile::prepare_write`] decided the write continues with; +/// performed by `run_with_fd` once that `&mut self` stage has returned. +#[cfg(not(windows))] +#[derive(Clone, Copy)] +enum Next { + WriteLoop, + WaitForWritable, + Finish, +} + // ────────────────────────────────────────────────────────────────────────── // FileOpener / FileCloser // ────────────────────────────────────────────────────────────────────────── @@ -146,11 +160,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 +367,29 @@ impl WriteFile { Ok(()) } - pub(crate) fn run(&mut self, task: WriteFileTask) { + /// The job's first pool step: keeps the token and starts the write. From + /// `get_fd` on, `*this` belongs to whichever thread the write continues on. + /// + /// # Safety + /// [`bun_jsc::JobContext::run`]'s contract; the caller does not touch + /// `*this` afterwards. + 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 +418,33 @@ impl WriteFile { } } + /// The write's [`OpenCallback`](crate::webcore::blob::OpenCallback): + /// decides the next step under a reborrow that ends before the step runs, + /// because the step hands `*this` on (to the io thread, or to the JS + /// thread, which frees it). + /// + /// # Safety + /// `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; whichever step runs is the last access to + // `*this` on this thread. + 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 +488,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 +508,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..afd50de5fb2a --- /dev/null +++ b/test/internal/source-lints/self-receiver-job-start.test.ts @@ -0,0 +1,252 @@ +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: the first parameter of `fn run` inside a `JobContext` trait or impl +// block, the first parameter of `get_fd` / `get_fd_by_opening` inside the +// `FileOpener` trait block, and the spelling `fn(&mut Self, Fd)` of an open +// continuation anywhere. The steps below these entry points (`wait_for_*`, +// `on_finish`, `do_close`, the libuv completions) are guarded by their own +// conversions, not by this lint. +// +// 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)); +})(); + +// 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 (` followed by its first parameter, for the given names. */ +function fnWithFirstParam(names: string): RegExp { + return new RegExp(String.raw`\bfn\s+(?:${names})\s*\(\s*([^,)]*)`, "g"); +} + +/** Offsets of every entry point in a trait/impl block (found by `header`) whose first parameter is not a pointer. */ +function entryOffenders(stripped: string, header: RegExp, fns: string): number[] { + const out: number[] = []; + 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))) { + if (!POINTER_PARAM.test(f[1].trim())) out.push(item.start + f.index); + } + } + return out; +} + +// `JobContext::run`: the declaration and every implementation. +const JOB_CONTEXT = /\btrait\s+JobContext\b|\bJobContext\s+for\b/g; +function jobRunOffenders(stripped: string): number[] { + return entryOffenders(stripped, JOB_CONTEXT, "run"); +} + +// `FileOpener::get_fd` / `get_fd_by_opening`, the frames that invoke the continuation. +const FILE_OPENER = /\btrait\s+FileOpener\b/g; +function fileOpenerOffenders(stripped: string): number[] { + return entryOffenders(stripped, FILE_OPENER, "get_fd|get_fd_by_opening"); +} + +// An open continuation typed as taking the task by reference, wherever it is +// spelled (the `get_fd` parameter, the Windows stash accessors, a field). +const OPEN_CONTINUATION_BY_REF = /\bfn\s*\(\s*&\s*(?:'\w+\s+)?mut\s+Self\s*,\s*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 offenders = { jobRun: [] as string[], fileOpener: [] 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, ""); + for (const offset of jobRunOffenders(stripped)) offenders.jobRun.push(`${source}:${lineOf(stripped, offset)}`); + for (const offset of fileOpenerOffenders(stripped)) { + offenders.fileOpener.push(`${source}:${lineOf(stripped, offset)}`); + } + for (const offset of openContinuationOffenders(stripped)) { + offenders.openContinuation.push(`${source}:${lineOf(stripped, 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", + // An unrelated trait with a `run` taking a reference. + "impl TaskContext for Foo {\n fn run(&mut self) {}\n}\n", + ]; + expect(banned.map(s => jobRunOffenders(s).length)).toEqual(banned.map(() => 1)); + expect(allowed.map(s => jobRunOffenders(s).length)).toEqual(allowed.map(() => 0)); +}); + +test("the FileOpener patterns match the banned shapes and nothing else", () => { + const opener = (body: string) => `pub trait FileOpener: Sized {\n fn opened_fd(&self) -> Fd;\n${body}}\n`; + const bannedEntries = [ + // `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 allowedEntries = [ + 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", + ), + // The accessors the entry points use may take `self`: they return before the hand-over. + opener( + " fn set_opened_fd(&mut self, fd: Fd);\n fn open_pathlike(&mut self) -> Fd {\n Fd::INVALID\n }\n", + ), + // A `get_fd` outside the trait block (the sinks have one) is something else. + "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(bannedEntries.map(s => fileOpenerOffenders(s).length)).toEqual(bannedEntries.map(() => 1)); + expect(allowedEntries.map(s => fileOpenerOffenders(s).length)).toEqual(allowedEntries.map(() => 0)); + + const bannedContinuations = [ + "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 ),", + ]; + const allowedContinuations = [ + "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)", + ]; + expect(bannedContinuations.map(s => openContinuationOffenders(s).length)).toEqual(bannedContinuations.map(() => 1)); + expect(allowedContinuations.map(s => openContinuationOffenders(s).length)).toEqual(allowedContinuations.map(() => 0)); +}); + +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("no open continuation takes the task by reference", () => { + expect(offenders.openContinuation).toEqual([]); +}); From 3ace166059b61a8158fa23fd6676061e9e3e02ef Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:25:16 +0000 Subject: [PATCH 2/3] Pin the OpenCallback alias in the lint and assert the anchors still match The continuation type now only exists as the OpenCallback alias, so the lint checks the alias definition itself (unsafe fn over *mut) and widens the spelled-out net to any fn(&mut X, Fd). The anchored checks record what they examined and assert the JobContext declaration, the two FileOpener entry points and the one alias were found, so a rename fails the lint rather than emptying it. The readdir scan's run comment says that its hand-over still happens inside perform_work's &mut self. --- src/runtime/node/node_fs.rs | 4 +- .../self-receiver-job-start.test.ts | 196 ++++++++++++++---- 2 files changed, 154 insertions(+), 46 deletions(-) diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index d31fce9ad4c1..3487db3d3423 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -2204,7 +2204,9 @@ mod _async_tasks { ZStr::from_buf(bytes, bytes.len() - 1) }; // May finish synchronously (no subdirectories) or fan out; the last - // subtask finishes the token, so this is the hand-over. + // 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/test/internal/source-lints/self-receiver-job-start.test.ts b/test/internal/source-lints/self-receiver-job-start.test.ts index afd50de5fb2a..5fd2a8cce876 100644 --- a/test/internal/source-lints/self-receiver-job-start.test.ts +++ b/test/internal/source-lints/self-receiver-job-start.test.ts @@ -37,12 +37,26 @@ import { globAllSources } from "../../../scripts/glob-sources.ts"; // 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: the first parameter of `fn run` inside a `JobContext` trait or impl -// block, the first parameter of `get_fd` / `get_fd_by_opening` inside the -// `FileOpener` trait block, and the spelling `fn(&mut Self, Fd)` of an open -// continuation anywhere. The steps below these entry points (`wait_for_*`, -// `on_finish`, `do_close`, the libuv completions) are guarded by their own -// conversions, not by this lint. +// 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. @@ -63,6 +77,12 @@ const tracked: Set | 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. @@ -86,39 +106,55 @@ function itemBlock(stripped: string, headerIndex: number): { start: number; bloc // receiver, `this: &mut Self`, `ctx: &mut C`) is the banned shape. const POINTER_PARAM = /^(?:mut\s+)?\w+\s*:\s*\*mut\b/; -/** `fn (` followed by its first parameter, for the given names. */ +/** `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"); + return new RegExp(String.raw`\bfn\s+(${names})\s*\(\s*([^,)]*)`, "g"); } -/** Offsets of every entry point in a trait/impl block (found by `header`) whose first parameter is not a pointer. */ -function entryOffenders(stripped: string, header: RegExp, fns: string): number[] { - const out: number[] = []; +/** 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))) { - if (!POINTER_PARAM.test(f[1].trim())) out.push(item.start + f.index); + 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 out; + return scan; } -// `JobContext::run`: the declaration and every implementation. +// 1. `JobContext::run`: the declaration and every implementation. const JOB_CONTEXT = /\btrait\s+JobContext\b|\bJobContext\s+for\b/g; -function jobRunOffenders(stripped: string): number[] { - return entryOffenders(stripped, JOB_CONTEXT, "run"); +function scanJobRun(stripped: string): Scan { + return scanEntries(stripped, JOB_CONTEXT, "run"); } -// `FileOpener::get_fd` / `get_fd_by_opening`, the frames that invoke the continuation. +// 2. `FileOpener::get_fd` / `get_fd_by_opening`, the frames that invoke the continuation. const FILE_OPENER = /\btrait\s+FileOpener\b/g; -function fileOpenerOffenders(stripped: string): number[] { - return entryOffenders(stripped, FILE_OPENER, "get_fd|get_fd_by_opening"); +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; } -// An open continuation typed as taking the task by reference, wherever it is -// spelled (the `get_fd` parameter, the Windows stash accessors, a field). -const OPEN_CONTINUATION_BY_REF = /\bfn\s*\(\s*&\s*(?:'\w+\s+)?mut\s+Self\s*,\s*Fd\s*\)/g; +// 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); } @@ -127,7 +163,13 @@ function lineOf(text: string, offset: number): number { return text.slice(0, offset).split("\n").length; } -const offenders = { jobRun: [] as string[], fileOpener: [] as string[], openContinuation: [] as string[] }; +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, "/"); @@ -141,13 +183,16 @@ for (const abs of rustSources) { // 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, ""); - for (const offset of jobRunOffenders(stripped)) offenders.jobRun.push(`${source}:${lineOf(stripped, offset)}`); - for (const offset of fileOpenerOffenders(stripped)) { - offenders.fileOpener.push(`${source}:${lineOf(stripped, offset)}`); - } - for (const offset of openContinuationOffenders(stripped)) { - offenders.openContinuation.push(`${source}:${lineOf(stripped, offset)}`); + 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", () => { @@ -182,16 +227,21 @@ test("the JobContext::run pattern matches the banned shapes and nothing else", ( // 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", - // An unrelated trait with a `run` taking a reference. - "impl TaskContext for Foo {\n fn run(&mut self) {}\n}\n", ]; - expect(banned.map(s => jobRunOffenders(s).length)).toEqual(banned.map(() => 1)); - expect(allowed.map(s => jobRunOffenders(s).length)).toEqual(allowed.map(() => 0)); + 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 patterns match the banned shapes and nothing else", () => { +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 bannedEntries = [ + 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", @@ -203,40 +253,92 @@ test("the FileOpener patterns match the banned shapes and nothing else", () => { ), opener(" unsafe fn get_fd(this: &mut Self, callback: OpenCallback) {}\n"), ]; - const allowedEntries = [ + 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", ), - // The accessors the entry points use may take `self`: they return before the hand-over. + ]; + 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", ), - // A `get_fd` outside the trait block (the sinks have one) is something else. "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(bannedEntries.map(s => fileOpenerOffenders(s).length)).toEqual(bannedEntries.map(() => 1)); - expect(allowedEntries.map(s => fileOpenerOffenders(s).length)).toEqual(allowedEntries.map(() => 0)); + expect(notExamined.map(s => scanFileOpener(s))).toEqual(notExamined.map(() => ({ checked: [], offenders: [] }))); +}); - const bannedContinuations = [ +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 allowedContinuations = [ + 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(bannedContinuations.map(s => openContinuationOffenders(s).length)).toEqual(bannedContinuations.map(() => 1)); - expect(allowedContinuations.map(s => openContinuationOffenders(s).length)).toEqual(allowedContinuations.map(() => 0)); + 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", () => { @@ -247,6 +349,10 @@ test("FileOpener's entry points take the task by pointer", () => { expect(offenders.fileOpener).toEqual([]); }); -test("no open continuation takes the task by reference", () => { +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([]); }); From dd27fce990520ebe2e508211e1a3bf26b7775df3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 18:11:52 +0000 Subject: [PATCH 3/3] Shorten the new doc and SAFETY comments --- src/jsc/job.rs | 24 ++++-------- src/runtime/webcore/Blob.rs | 54 ++++++++++---------------- src/runtime/webcore/blob/read_file.rs | 37 +++++++----------- src/runtime/webcore/blob/write_file.rs | 24 ++++-------- 4 files changed, 50 insertions(+), 89 deletions(-) diff --git a/src/jsc/job.rs b/src/jsc/job.rs index 94a0e6b0d0dc..40a286e50443 100644 --- a/src/jsc/job.rs +++ b/src/jsc/job.rs @@ -240,21 +240,14 @@ pub trait JobContext: Sized + 'static { /// complete then. Work that outlives this call runs under no borrow and /// must touch only the off-thread part. /// - /// `off` is a pointer rather than `&mut` because a body that keeps `done` - /// hands the job on before it returns (to an io loop, to sub-tasks, or, by - /// finishing `done`, to the JS thread), and whoever ends up finishing it - /// makes the JS thread read and free the allocation through the job's own - /// pointer (`Job::complete`), possibly before this thread has returned - /// from here. A reference argument is protected for the whole call, and an - /// access through another pointer to memory a protected reference covers - /// is UB under the aliasing model whether or not the reference is used - /// again; a raw pointer makes no such claim. A body that completes - /// synchronously just reborrows for the work and returns `done`; one that - /// hands the job on does its own work through reborrows that end before the - /// hand-over, and makes the hand-over its last access. + /// `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 + /// `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, @@ -405,9 +398,8 @@ impl Job { return done.finish(); }; // SAFETY: as above; the borrow keeps the VM (and any JsPtr target) alive. - // On `None` the body handed the job on (it may already be freed), and - // nothing below touches `*this`: releasing `vm` goes through our own - // `handle` clone, not the job's. + // 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(); diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index c1e7020539eb..3a8d20642c30 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -6846,17 +6846,14 @@ bun_jsc::jsc_host_abi! { // FileOpener / FileCloser // ────────────────────────────────────────────────────────────────────────── -/// What [`FileOpener::get_fd`] continues with once the task's fd is known: the -/// fd, or `Fd::INVALID` with `errno` / `system_error` set when the open failed. -/// It takes the task over: the implementations end by handing it on -/// (`ReadFile`, `WriteFile`; the JS thread then reads and frees the job through -/// the job's own pointer) or by freeing it right there (`ReadFileUV`). Neither -/// is allowed while a `&mut Self` argument is still protected, i.e. until the -/// continuation has returned (see [`bun_jsc::JobContext::run`]); hence the -/// pointer, here and in the `get_fd` frames that invoke it. +/// [`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, and the caller does not touch it +/// `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); @@ -6898,9 +6895,8 @@ pub trait FileOpener: Sized { #[cfg(windows)] fn open_callback(&self) -> OpenCallback; - /// Opens the path in `pathlike()` and records the outcome: the fd in - /// `opened_fd`, or `Fd::INVALID` plus `errno` / `system_error`. Returns - /// what it recorded. + /// 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(); @@ -6955,11 +6951,9 @@ pub trait FileOpener: Sized { // 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` is the live request queued below, whose `data` - // is the task's pointer; the task was left alone until this - // completion. The request is done with before the task is - // touched, each reborrow of the task ends with its accessor - // call, and `cb` (which takes the task over) is the last access. + // 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; @@ -6995,11 +6989,9 @@ pub trait FileOpener: Sized { 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, so it is live for as long as - // the open is in flight, and nothing touches it from here until - // `wrapped_callback` runs. `req.data` is set before the open is - // queued because a synchronous failure runs `callback` (which may - // free the task) right below. + // `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_(); @@ -7015,9 +7007,8 @@ pub trait FileOpener: Sized { ) }; if let Some(errno) = rc.err_enum_e() { - // SAFETY: fn contract; libuv did not keep the request. The - // reborrows end with their accessor calls, and `callback` is the - // last access to `*this`. + // 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( @@ -7036,23 +7027,20 @@ pub trait FileOpener: Sized { { // SAFETY: fn contract; the reborrow ends with the call. let fd = unsafe { (*this).open_pathlike() }; - // SAFETY: fn contract, passed through; nothing here touches `*this` - // afterwards. + // SAFETY: fn contract, passed through; this is the last access. unsafe { callback(this, fd) } } } - /// Finds the task's fd (`opened_fd` if it is already set, the descriptor - /// of an fd-backed `pathlike()`, or else by opening the path) and continues - /// with `callback`, which takes the task over. + /// 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, since `callback` hands it on or frees it (see - /// [`OpenCallback`]). + /// 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 to `*this` on the paths that run it. + // `callback` is the last access on the paths that run it. unsafe { let fd = (*this).opened_fd(); if fd != Fd::INVALID { diff --git a/src/runtime/webcore/blob/read_file.rs b/src/runtime/webcore/blob/read_file.rs index 8f4ec8e5640f..9055c4344ed1 100644 --- a/src/runtime/webcore/blob/read_file.rs +++ b/src/runtime/webcore/blob/read_file.rs @@ -230,9 +230,7 @@ impl bun_jsc::JobContext for ReadFile { _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - // Starts the read, which hands `*this` on (to the io thread, or to the - // JS thread by finishing the token); nothing here touches it afterwards. - // SAFETY: fn contract, passed through. + // SAFETY: fn contract, passed through; `run_async` hands `*this` on. unsafe { ReadFile::run_async(this, done) }; None } @@ -296,8 +294,7 @@ pub struct ReadFile { bun_threading::intrusive_work_task!(ReadFile, task); bun_io::intrusive_io_request!(ReadFile, io_request); -/// What [`ReadFile::prepare_read`] decided the read continues with; performed -/// by `run_async_with_fd` once that `&mut self` stage has returned. +/// The step `run_async_with_fd` performs once `prepare_read`'s `&mut self` has ended. #[cfg(not(windows))] #[derive(Clone, Copy)] enum Next { @@ -613,12 +610,10 @@ impl ReadFile { Ok(()) } - /// The job's first pool step: keeps the token and starts the read. From - /// `get_fd` on, `*this` belongs to whichever thread the read continues on. + /// First pool step: keeps the token and starts the read; `get_fd` hands `*this` on. /// /// # Safety - /// [`bun_jsc::JobContext::run`]'s contract; the caller does not touch - /// `*this` afterwards. + /// [`bun_jsc::JobContext::run`]'s contract. unsafe fn run_async(this: *mut Self, task: ReadFileTask) { #[cfg(windows)] { @@ -721,19 +716,16 @@ impl ReadFile { } } - /// The read's [`OpenCallback`](crate::webcore::blob::OpenCallback): decides - /// the next step under a reborrow that ends before the step runs, because - /// the step hands `*this` on (to the io thread, or to the JS thread, which - /// frees it). + /// The read's continuation: `prepare_read`'s reborrow has ended by the + /// time the step it chose hands `*this` on. /// /// # Safety - /// `OpenCallback`'s contract. + /// [`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; whichever step runs is the last access to - // `*this` on this thread. + // SAFETY: fn contract; the step is this thread's last access. unsafe { match next { Next::ReadLoop => (*this).do_read_loop(), @@ -1164,18 +1156,15 @@ impl<'a> ReadFileUV<'a> { Self::finalize(core::ptr::from_mut(self)); } - /// The read's [`OpenCallback`]: queues the fstat, or finishes (which frees - /// the task) if the open failed or the fstat cannot be queued. + /// The read's continuation: queues the fstat, or finishes (freeing the task). /// /// # Safety - /// `OpenCallback`'s contract. + /// [`OpenCallback`]'s contract. unsafe fn on_file_open(this: *mut Self, opened_fd: Fd) { log!("ReadFileUV.onFileOpen"); - // SAFETY: fn contract. Field accesses are statement-scoped, the - // `on_finish` calls are the last access on their paths, and the FFI - // call gets the live VM uv loop, the task's own freshly deinit'd `fs_t` - // (whose `data` lets `on_file_initial_stat` recover the task), and the - // just-opened fd. + // 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(); diff --git a/src/runtime/webcore/blob/write_file.rs b/src/runtime/webcore/blob/write_file.rs index 7608920dbd1a..1921af8dbf62 100644 --- a/src/runtime/webcore/blob/write_file.rs +++ b/src/runtime/webcore/blob/write_file.rs @@ -55,9 +55,7 @@ impl bun_jsc::JobContext for WriteFile { _vm: &bun_jsc::vm_handle::Borrow, done: bun_jsc::Completion, ) -> Option> { - // Starts the write, which hands `*this` on (to the io thread, or to the - // JS thread by finishing the token); nothing here touches it afterwards. - // SAFETY: fn contract, passed through. + // SAFETY: fn contract, passed through; `run_async` hands `*this` on. unsafe { WriteFile::run_async(this, done) }; None } @@ -102,8 +100,7 @@ pub struct WriteFile { bun_threading::intrusive_work_task!(WriteFile, task); bun_io::intrusive_io_request!(WriteFile, io_request); -/// What [`WriteFile::prepare_write`] decided the write continues with; -/// performed by `run_with_fd` once that `&mut self` stage has returned. +/// The step `run_with_fd` performs once `prepare_write`'s `&mut self` has ended. #[cfg(not(windows))] #[derive(Clone, Copy)] enum Next { @@ -367,12 +364,10 @@ impl WriteFile { Ok(()) } - /// The job's first pool step: keeps the token and starts the write. From - /// `get_fd` on, `*this` belongs to whichever thread the write continues on. + /// First pool step: keeps the token and starts the write; `get_fd` hands `*this` on. /// /// # Safety - /// [`bun_jsc::JobContext::run`]'s contract; the caller does not touch - /// `*this` afterwards. + /// [`bun_jsc::JobContext::run`]'s contract. unsafe fn run_async(this: *mut Self, task: WriteFileTask) { #[cfg(windows)] { @@ -418,19 +413,16 @@ impl WriteFile { } } - /// The write's [`OpenCallback`](crate::webcore::blob::OpenCallback): - /// decides the next step under a reborrow that ends before the step runs, - /// because the step hands `*this` on (to the io thread, or to the JS - /// thread, which frees it). + /// The write's continuation: `prepare_write`'s reborrow has ended by the + /// time the step it chose hands `*this` on. /// /// # Safety - /// `OpenCallback`'s contract. + /// [`OpenCallback`](crate::webcore::blob::OpenCallback)'s contract. #[cfg(not(windows))] 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; whichever step runs is the last access to - // `*this` on this thread. + // SAFETY: fn contract; the step is this thread's last access. unsafe { match next { Next::WriteLoop => (*this).do_write_loop(),