diff --git a/src/jsc/webcore_types.rs b/src/jsc/webcore_types.rs index fa74140c563b..b6445abf6f2b 100644 --- a/src/jsc/webcore_types.rs +++ b/src/jsc/webcore_types.rs @@ -778,7 +778,6 @@ pub mod store { // ──────────────────────────────────────────────────────────────────── /// A blob store referencing a file on disk. - #[derive(Clone)] pub struct File { pub pathlike: PathOrFileDescriptor, pub mime_type: MimeType, @@ -786,8 +785,10 @@ pub mod store { pub mode: bun_sys::Mode, pub seekable: Option, pub max_size: SizeType, - /// Milliseconds since ECMAScript epoch. - pub last_modified: crate::JSTimeType, + /// Milliseconds since ECMAScript epoch. Atomic: worker-thread + /// `ReadFile` tasks write it while the JS thread reads it + /// (overlapping `file.bytes()` calls share one `Store`). + pub last_modified: core::sync::atomic::AtomicU64, } impl Default for File { @@ -799,7 +800,26 @@ pub mod store { mode: 0, seekable: None, max_size: MAX_SIZE, - last_modified: crate::INIT_TIMESTAMP, + last_modified: core::sync::atomic::AtomicU64::new(crate::INIT_TIMESTAMP), + } + } + } + + impl Clone for File { + fn clone(&self) -> Self { + Self { + pathlike: self.pathlike.clone(), + mime_type: self.mime_type.clone(), + is_atty: self.is_atty, + mode: self.mode, + seekable: self.seekable, + max_size: self.max_size, + // Snapshot the atomic via `Relaxed`; `Clone` is a per-thread + // value copy, not a memory-ordering sync point. + last_modified: core::sync::atomic::AtomicU64::new( + self.last_modified + .load(core::sync::atomic::Ordering::Relaxed), + ), } } } @@ -1048,15 +1068,21 @@ pub mod store { core::mem::ManuallyDrop::new(self).ptr.as_ptr() } - /// Mutable access to `data` through the shared handle. The caller - /// must ensure no - /// other `&mut` to the same `Store` is live (single-threaded JS - /// event-loop discipline). + /// Mutable access to `data` through the shared handle. + /// + /// # Safety + /// No other reference (`&Store`, `&mut Store`, `&Data`, `&mut Data`) + /// to the same pointee may be live for the duration of the returned + /// borrow — on this thread or any other. The same contract governs + /// the sibling `unsafe fn`s that mint `&mut Store` access: + /// `blob_store_mut`/`set_blob_content_type` in `webcore::body`, and + /// `BlobExt::shared_view_raw`/`set_is_ascii_flag` and the free + /// `resolve_file_stat` in `webcore::blob`. #[inline] #[allow(clippy::mut_from_ref)] - pub fn data_mut(&self) -> &mut Data { - // SAFETY: caller guarantees no other `&mut` to this `Store` is - // live; see doc comment. + pub unsafe fn data_mut(&self) -> &mut Data { + // SAFETY: precondition — no aliasing `&`/`&mut` to the pointee is + // live for the returned borrow's duration (see fn doc). unsafe { &mut (*self.as_ptr()).data } } } @@ -1107,11 +1133,32 @@ pub mod store { } impl Eq for StoreRef {} - // SAFETY: `Store`'s refcount is atomic and its payload is either - // immutable-after-init or guarded by callers. + // SAFETY: `Store`'s refcount is atomic; the `Data` payload is mutated + // only under `data_mut`'s exclusivity precondition (move, don't share). + // CAVEAT — `Data::S3` holds `Rc` (non-atomic refcount, + // shared with JS-thread state via `Rc::clone(s3.get_credentials())` in + // `Blob.rs`), but worker-pool tasks only carry `Data::File`/`Data::Bytes` + // stores; S3 I/O stays on the JS thread. If an S3 store ever crosses + // threads, make that `Rc` an `Arc`. unsafe impl Send for StoreRef {} - // SAFETY: `Store::ref_count` is atomic and `&StoreRef` only derefs to - // `&Store`. - unsafe impl Sync for StoreRef {} + // Intentionally NOT `Sync`: two threads sharing `&StoreRef` could each + // mint `&mut Data` via `data_mut`. Dropping `Sync` closes that direct + // shape; cloned handles (`Send`) and `Blob: Sync` still route around + // it, so the load-bearing guard remains `data_mut`'s precondition, + // discharged in writing at every call site. + + // Compile-time trip-wire: if `StoreRef` ever gains `Sync`, both blanket + // impls of `_NotSyncCheck` apply and `_NOT_SYNC` fails to compile with + // "conflicting impls" (same pattern as + // `src/runtime/shell/subproc.rs` `__pipe_reader_thread_confined`). + mod __store_ref_not_sync { + use super::StoreRef; + trait _NotSyncCheck { + const OK: () = (); + } + impl _NotSyncCheck<()> for T {} + impl _NotSyncCheck for T {} + const _NOT_SYNC: () = >::OK; + } } pub use store::{Store, StoreRef}; diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index 8c2a9d0eef54..fa40f508d120 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -298,8 +298,17 @@ pub trait BlobExt { where Self: Sized; fn transfer(&self); - fn shared_view_raw(&self) -> *mut [u8]; - fn set_is_ascii_flag(&self, is_all_ascii: bool); + /// # Safety + /// Mints a mutable raw view into the backing `Store` via + /// `StoreRef::data_mut`; the caller asserts no other `&`/`&mut` to the + /// same `Store` is live while the returned pointer is in use (JS-thread + /// exclusivity). Same contract as [`blob::StoreRef::data_mut`]. + unsafe fn shared_view_raw(&self) -> *mut [u8]; + /// # Safety + /// Writes the backing `Store`'s `is_all_ascii` flag through + /// `StoreRef::as_ptr`; the caller asserts no other `&`/`&mut` to the same + /// `Store` is live for the write (JS-thread exclusivity). + unsafe fn set_is_ascii_flag(&self, is_all_ascii: bool); /// # Safety /// `raw_bytes` must be valid for reads for the duration of the call; when /// `LIFETIME == Temporary` it must be a leaked default-allocator `Box<[u8]>`. @@ -1039,7 +1048,8 @@ impl BlobExt for Blob { let content_type = self.content_type_slice(); let offset = self.offset.get(); let store = self.store().expect("infallible: store present"); - match store.data_mut() { + // Read-only formatter block; shared borrow suffices. + match &store.data { store::Data::S3(s3) => { S3File::write_format::( s3, @@ -2118,21 +2128,28 @@ impl BlobExt for Blob { fn get_last_modified(&self, _: &JSGlobalObject) -> JSValue { if let Some(store) = self.store.get() { if matches!(store.data, store::Data::File(_)) { - // do not hold a pattern-bound `&File` across - // `resolve_file_stat` — it materializes `&mut File` on the same - // memory (Stacked Borrows UB; the optimizer may legally cache the - // pre-call `last_modified` and return the stale `INIT_TIMESTAMP`). - // Re-read via `StoreRef::data_mut` (raw-ptr-backed accessor) after - // the mutating call. - let last_modified = store.data_mut().as_file().last_modified; - // last_modified can be already set during read. + // Borrow discipline: see `resolve_size`. Snapshot the + // `AtomicU64` through a shared borrow, drop it, then let + // `resolve_file_stat` materialize `&mut File`. + let last_modified = match &store.data { + store::Data::File(f) => { + f.last_modified.load(core::sync::atomic::Ordering::Relaxed) + } + _ => unreachable!("checked via matches! above"), + }; if last_modified == jsc::INIT_TIMESTAMP && !self.is_s3() { - resolve_file_stat(store); + // SAFETY: the snapshot above dropped its borrow; no + // `Data` borrow is live. + unsafe { resolve_file_stat(store) }; } - // Fresh borrow after possible mutation by `resolve_file_stat`. - return JSValue::js_number(JSValue::purify_nan( - store.data_mut().as_file().last_modified as f64, - )); + // Fresh shared borrow after the possible mutation. + let last_modified = match &store.data { + store::Data::File(f) => { + f.last_modified.load(core::sync::atomic::Ordering::Relaxed) + } + _ => unreachable!("checked via matches! above"), + }; + return JSValue::js_number(JSValue::purify_nan(last_modified as f64)); } } @@ -2252,16 +2269,11 @@ impl BlobExt for Blob { self.size.set(0); return; }; - // dispatch on the copied `DataTag` rather than - // `match &store.data { File(file) => … }`. The latter goes through - // `StoreRef::Deref → &Store → &Data` (no `UnsafeCell`), and that shared - // borrow is live across the arm body where `resolve_file_stat` - // materializes `&mut File` on the same memory via the raw - // `heap::alloc` pointer — Stacked Borrows UB, and under noalias the - // optimizer may legally cache the pre-call `seekable: None` and fall - // through to `self.size.get() = 0`. `StoreRef::data_mut` centralises - // the raw-ptr deref so each read here is a fresh, safe borrow. - match store.data_mut().tag() { + // Borrow discipline: never hold a `&Data`/`&File` across + // `resolve_file_stat` — it materializes `&mut File` on the same + // memory (aliasing UB; the optimizer may cache the pre-call field + // values). Snapshot via a shared borrow, drop it, call, re-borrow. + match store.data.tag() { store::DataTag::Bytes => { let offset = self.offset.get(); let store_size = store.size(); @@ -2272,11 +2284,20 @@ impl BlobExt for Blob { } } store::DataTag::File => { - if store.data_mut().as_file().seekable.is_none() { - resolve_file_stat(store); + let needs_stat = match &store.data { + store::Data::File(f) => f.seekable.is_none(), + _ => unreachable!("tag matched File"), + }; + if needs_stat { + // SAFETY: the `needs_stat` snapshot dropped its borrow; + // no `Data` borrow is live. + unsafe { resolve_file_stat(store) }; } - // Fresh borrow after possible mutation by `resolve_file_stat`. - let file = store.data_mut().as_file(); + // Fresh shared borrow after the possible mutation. + let file = match &store.data { + store::Data::File(f) => f, + _ => unreachable!("tag matched File"), + }; if file.seekable.is_some() && file.max_size != MAX_SIZE { let store_size = file.max_size; @@ -2307,10 +2328,8 @@ impl BlobExt for Blob { let Some(store) = self.store.get() else { return (self.offset.get(), 0); }; - // see `resolve_size` — dispatch on the copied tag and re-read - // via `StoreRef::data_mut` after `resolve_file_stat` so no - // `Deref`-produced `&Data`/`&File` is live across the mutating call. - match store.data_mut().tag() { + // Borrow discipline: see `resolve_size`. + match store.data.tag() { store::DataTag::Bytes => { let offset = self.offset.get(); let store_size = store.size(); @@ -2322,11 +2341,20 @@ impl BlobExt for Blob { (self.offset.get(), self.size.get()) } store::DataTag::File => { - if store.data_mut().as_file().seekable.is_none() { - resolve_file_stat(store); + let needs_stat = match &store.data { + store::Data::File(f) => f.seekable.is_none(), + _ => unreachable!("tag matched File"), + }; + if needs_stat { + // SAFETY: the `needs_stat` snapshot dropped its borrow; + // no `Data` borrow is live. + unsafe { resolve_file_stat(store) }; } - // Fresh borrow after possible mutation by `resolve_file_stat`. - let file = store.data_mut().as_file(); + // Fresh shared borrow after the possible mutation. + let file = match &store.data { + store::Data::File(f) => f, + _ => unreachable!("tag matched File"), + }; if file.seekable.is_some() && file.max_size != MAX_SIZE { let store_size = file.max_size; let offset = store_size.min(self.offset.get()); @@ -2505,7 +2533,7 @@ impl BlobExt for Blob { /// The returned pointer aliases the Store's `Vec` payload. Callers must /// not hold a live `&`/`&mut` into the same Store across uses of this /// pointer, and must keep a `StoreRef` alive for the pointer's lifetime. - fn shared_view_raw(&self) -> *mut [u8] { + unsafe fn shared_view_raw(&self) -> *mut [u8] { let empty = || { core::ptr::slice_from_raw_parts_mut(core::ptr::NonNull::::dangling().as_ptr(), 0) }; @@ -2521,7 +2549,10 @@ impl BlobExt for Blob { // not alias any outstanding borrow (other `StoreRef`s only hold raw // `NonNull`, never a long-lived `&Store`; JS execution is // single-threaded). - match store_ref.data_mut() { + // SAFETY: single-threaded JS path; no other borrow of the pointee is + // live — the function takes `&self` and releases the borrow at match + // exit (the returned raw pointer is not a reference). + match unsafe { store_ref.data_mut() } { store::Data::Bytes(bytes) => { let v: &mut [u8] = bytes.as_array_list_leak(); let len = v.len(); @@ -2540,7 +2571,7 @@ impl BlobExt for Blob { } } - fn set_is_ascii_flag(&self, is_all_ascii: bool) { + unsafe fn set_is_ascii_flag(&self, is_all_ascii: bool) { self.charset .set(strings::AsciiStatus::from_bool(Some(is_all_ascii))); // if this Blob represents the entire binary data @@ -2628,7 +2659,9 @@ impl BlobExt for Blob { }; if let Some(external) = converted { if LIFETIME != Lifetime::Temporary { - self.set_is_ascii_flag(false); + // SAFETY: single-threaded JS body-consumer path; no other + // borrow of the backing `Store` is live for this flag write. + unsafe { self.set_is_ascii_flag(false) }; } if LIFETIME == Lifetime::Transfer { self.detach(); @@ -2651,7 +2684,9 @@ impl BlobExt for Blob { } if LIFETIME != Lifetime::Temporary { - self.set_is_ascii_flag(true); + // SAFETY: single-threaded JS body-consumer path; no other + // borrow of the backing `Store` is live for this flag write. + unsafe { self.set_is_ascii_flag(true) }; } } @@ -2732,7 +2767,9 @@ impl BlobExt for Blob { // only ever reads through it (`&*raw_bytes`); the sole write path — // `heap::take` in the `Temporary` arm — is statically unreachable // below. - let view_ptr = self.shared_view_raw(); + // SAFETY: single-threaded JS body-consumer path; no other borrow of + // the backing `Store` is live while `view_ptr` is consumed below. + let view_ptr = unsafe { self.shared_view_raw() }; if view_ptr.len() == 0 { return Ok(ZigString::EMPTY.to_js(global)); } @@ -2771,7 +2808,9 @@ impl BlobExt for Blob { // `shared_view_raw` yields a `*mut [u8]` with mutable provenance (via // `StoreRef::as_ptr`). `to_json_with_bytes` only reads through it for the // non-`Temporary` lifetimes below. - let view_ptr = self.shared_view_raw(); + // SAFETY: single-threaded JS body-consumer path; no other borrow of + // the backing `Store` is live while `view_ptr` is consumed below. + let view_ptr = unsafe { self.shared_view_raw() }; match lifetime { // SAFETY: `view_ptr` is the store-backed view from `shared_view_raw`; // valid for reads while the store ref is held. @@ -2862,7 +2901,9 @@ impl BlobExt for Blob { .map_err(|_| global.throw_out_of_memory())? { if LIFETIME != Lifetime::Temporary { - self.set_is_ascii_flag(false); + // SAFETY: single-threaded JS body-consumer path; no other + // borrow of the backing `Store` is live for this flag write. + unsafe { self.set_is_ascii_flag(false) }; } let result = ZigString::init_utf16(&external).to_json_object(global); drop(external); @@ -2870,7 +2911,9 @@ impl BlobExt for Blob { } if LIFETIME != Lifetime::Temporary { - self.set_is_ascii_flag(true); + // SAFETY: single-threaded JS body-consumer path; no other + // borrow of the backing `Store` is live for this flag write. + unsafe { self.set_is_ascii_flag(true) }; } } @@ -3113,7 +3156,9 @@ impl BlobExt for Blob { // backing via FFI and materialize `&mut *buf` to record ptr+len, which // is sound now that the provenance is writable. The `Temporary` arm // (`heap::take`) is statically unreachable below. - let view_ptr = self.shared_view_raw(); + // SAFETY: single-threaded JS body-consumer path; no other borrow of + // the backing `Store` is live while `view_ptr` is consumed below. + let view_ptr = unsafe { self.shared_view_raw() }; if view_ptr.len() == 0 { return jsc::ArrayBuffer::create::(global, b""); } @@ -3161,7 +3206,9 @@ impl BlobExt for Blob { // `to_form_data_with_bytes` (`FormData::to_js` takes `&[u8]`). Note: the // Store is intrusively shared (`ref_count: AtomicU32`); `&mut self` does // NOT imply exclusive ownership of the underlying bytes. - let view_ptr = self.shared_view_raw(); + // SAFETY: single-threaded JS body-consumer path; no other borrow of + // the backing `Store` is live while `view_ptr` is consumed below. + let view_ptr = unsafe { self.shared_view_raw() }; if view_ptr.len() == 0 { return Ok(jsc::DOMFormData::create(global)); } @@ -4072,7 +4119,11 @@ fn on_structured_clone_deserialize>( // ScopeGuard derefs to its inner Blob. if let Some(store) = (*guard).store() { - if let store::Data::Bytes(bytes_store) = &mut store.data_mut() { + // SAFETY: deserialization runs on the JS thread; `store` + // was just freshly constructed inside the guarded `blob` + // and the match borrow is released before any other + // access to the pointee. + if let store::Data::Bytes(bytes_store) = &mut unsafe { store.data_mut() } { // Transfer ownership of the local `name: Vec` into // `stored_name` (a `Box<[u8]>`); freed by `Bytes::Drop`. bytes_store.stored_name = name.into_boxed_slice(); @@ -4246,7 +4297,13 @@ pub(crate) extern "C" fn Blob__setAsFile(this: &mut Blob, path_str: &mut BunStri // This is not 100% correct... if let Some(store) = this.store() { - if let store::Data::Bytes(bytes) = &mut store.data_mut() { + // SAFETY: synchronous JS-thread C-ABI entry; no JS re-entry occurs + // inside the match, so no other `&Data`/`&mut Data` borrow of this + // `Store` is live for its duration. (The `Store` itself may be + // aliased by sibling `Blob`s via `dupe()`/`slice()`, but each + // `data_mut` there is likewise a synchronous JS-thread borrow that + // cannot overlap this one.) + if let store::Data::Bytes(bytes) = &mut unsafe { store.data_mut() } { if bytes.stored_name.is_empty() { // Owned heap slice // owned by `stored_name` (`Box<[u8]>`) and freed by `Bytes::Drop`. @@ -4915,8 +4972,11 @@ pub(crate) fn write_file_internal( debug_assert!(!matches!(blob_store.data, store::Data::Bytes(_))); // TODO only reset last_modified on success paths instead of resetting // last_modified at the beginning for better performance. - if let store::Data::File(ref mut file) = *blob_store.data_mut() { - file.last_modified = jsc::INIT_TIMESTAMP; + // Shared borrow: `last_modified` is `AtomicU64`, `store(Relaxed)` + // takes `&self` — no `&mut Data` materialized. + if let store::Data::File(file) = &blob_store.data { + file.last_modified + .store(jsc::INIT_TIMESTAMP, core::sync::atomic::Ordering::Relaxed); } } @@ -5549,7 +5609,14 @@ pub(crate) fn jsdom_file_construct( blob = Blob::get::(global_this, args[0])?; if let Some(store_) = blob.store.get() { - match store_.data_mut() { + // SAFETY: synchronous JS-thread `File` constructor; no JS + // re-entry occurs inside the match, so no other `&Data`/`&mut + // Data` to this `Store` is live for its duration. (For the + // `new File([existingBlob], ...)` path `Blob::get` returns + // `existingBlob.dupe()`, so `store_` may share the `Store` + // with the originating JS `Blob`; their respective `data_mut` + // borrows are also JS-thread-synchronous and cannot overlap.) + match unsafe { store_.data_mut() } { store::Data::Bytes(bytes) => { // `get::<_, true>` on a single-Blob sequence returns // `dupe()` (a shared StoreRef), so this `Bytes` may already @@ -5748,7 +5815,10 @@ impl S3BlobDownloadTask { // Move the downloaded body into a Blob store so its lifetime is // tied to the Blob/JS view and freed via the store's finalizer. let store = Store::init(response.body.list); - let bytes: *mut [u8] = match store.data_mut() { + // SAFETY: `store` is a freshly-constructed local and is the + // sole holder of the underlying allocation; the match borrow + // is released before the next statement. + let bytes: *mut [u8] = match unsafe { store.data_mut() } { store::Data::Bytes(b) => std::ptr::from_mut(b.as_array_list()), _ => unreachable!(), }; @@ -6121,11 +6191,18 @@ fn window_size(current: SizeType, available: SizeType) -> SizeType { } /// resolve file stat like size, last_modified -fn resolve_file_stat(store: &StoreRef) { - // `StoreRef::data_mut` encapsulates the raw-pointer deref under the - // `StoreRef` liveness invariant; the caller holds the only ref across - // this call, so an exclusive borrow is sound. - let file = store.data_mut().as_file_mut(); +/// +/// # Safety +/// Materializes `&mut File` via [`StoreRef::data_mut`]; the caller asserts no +/// `&`/`&mut Data` to the same `Store` is live across the call. Worker-pool +/// tasks (`read_file.rs`, `write_file.rs`) can still hold `&File` to this +/// `Store`: the `AtomicU64` `last_modified` covers the observable race, and +/// the remaining `seekable`/`mode` overlap is idempotent (every writer stores +/// the same fstat-derived values); closing it fully means interior-mutable +/// `File` fields, a follow-up beyond #30800. +unsafe fn resolve_file_stat(store: &StoreRef) { + // SAFETY: precondition — no aliasing `Data` borrow is live (see fn doc). + let file = unsafe { store.data_mut() }.as_file_mut(); match &file.pathlike { PathOrFileDescriptor::Path(path) => { let mut buffer = bun_paths::PathBuffer::uninit(); @@ -6138,7 +6215,10 @@ fn resolve_file_stat(store: &StoreRef) { }; file.mode = stat.st_mode as bun_sys::Mode; file.seekable = Some(bun_sys::S::ISREG(stat.st_mode as _)); - file.last_modified = stat_to_js_mtime(&stat); + file.last_modified.store( + stat_to_js_mtime(&stat), + core::sync::atomic::Ordering::Relaxed, + ); } // the file may not exist yet. That's okay. _ => {} @@ -6153,7 +6233,10 @@ fn resolve_file_stat(store: &StoreRef) { }; file.mode = stat.st_mode as bun_sys::Mode; file.seekable = Some(bun_sys::S::ISREG(stat.st_mode as _)); - file.last_modified = stat_to_js_mtime(&stat); + file.last_modified.store( + stat_to_js_mtime(&stat), + core::sync::atomic::Ordering::Relaxed, + ); } _ => {} }, @@ -6394,7 +6477,9 @@ impl Any { if let Some(s) = blob.store.get() { if matches!(s.data, store::Data::Bytes(_)) && s.has_one_ref() { // `StoreRef` exposes interior-mutable `data_mut()` (no DerefMut). - let internal = s.data_mut().as_bytes_mut().to_internal_blob(); + // SAFETY: `has_one_ref()` confirms this is the sole holder; + // `Any` is JS-thread-only, so no concurrent access exists. + let internal = unsafe { s.data_mut() }.as_bytes_mut().to_internal_blob(); // StoreRef::drop on the replace below releases the store ref. *self = Any::InternalBlob(internal); return; diff --git a/src/runtime/webcore/Body.rs b/src/runtime/webcore/Body.rs index 199858791101..f78b71651382 100644 --- a/src/runtime/webcore/Body.rs +++ b/src/runtime/webcore/Body.rs @@ -41,25 +41,38 @@ pub(super) fn wtf_impl(s: &WTFStringImpl) -> &WTFStringImplStruct { /// Mutable view of a [`Blob`]'s backing `Store` through its /// `JsCell>` field. Centralises the per-site raw -/// `(*blob.store.get()…as_ptr()).mime_type = …` deref under the same -/// invariant `StoreRef::data_mut` already documents: -/// shared-mutable interior, single-threaded JS event-loop, no concurrent -/// `&Store` outstanding for the borrow's duration. +/// `(*blob.store.get()…as_ptr()).mime_type = …` deref used by the body-mixin +/// `consume_` helpers. +/// +/// # Safety +/// For the lifetime of the returned `&mut Store`, no other reference +/// (`&Store`, `&mut Store`, `&Data`, `&mut Data`) to the same pointee may be +/// live — on this thread or any other. Same contract as +/// [`blob::StoreRef::data_mut`]. #[inline] #[allow(clippy::mut_from_ref)] -fn blob_store_mut(blob: &Blob) -> Option<&mut blob::Store> { +unsafe fn blob_store_mut(blob: &Blob) -> Option<&mut blob::Store> { blob.store .get() .as_ref() - // SAFETY: `StoreRef` invariant — pointee is a live heap `Store` while - // any `StoreRef` exists; single-threaded JS event-loop discipline - // guarantees no other `&`/`&mut Store` is live for this borrow. + // SAFETY: precondition — no aliasing `&`/`&mut` to the pointee is + // live for the returned borrow's duration (see fn doc). .map(|s| unsafe { &mut *s.as_ptr() }) } -fn set_blob_content_type(blob: &Blob, mime_type: MimeType) { +/// Stamp `mime_type` onto `blob.content_type` (and the backing `Store`'s +/// `mime_type`, when present). Wraps the `blob_store_mut` back-door, so it +/// inherits the same exclusivity precondition. +/// +/// # Safety +/// Same contract as [`blob_store_mut`]: for the duration of this call, no +/// other reference (`&Store`, `&mut Store`, `&Data`, `&mut Data`) to the +/// `Blob`'s backing `Store` may be live — on this thread or any other. +unsafe fn set_blob_content_type(blob: &Blob, mime_type: MimeType) { blob.content_type_was_set.set(true); - if let Some(store) = blob_store_mut(blob) { + // SAFETY: precondition — no aliasing `Store` reference is live for this + // borrow (see fn doc). + if let Some(store) = unsafe { blob_store_mut(blob) } { store.mime_type = mime_type.clone(); } blob.content_type @@ -1182,12 +1195,23 @@ impl Value { { let content_slice = content_type.to_slice(); let mime_type = MimeType::init(content_slice.slice(), true, None); - set_blob_content_type(blob, mime_type); + // SAFETY: synchronous JS-thread body-consumer + // continuation; no JS re-entry before the + // borrow ends, and other `StoreRef` clones + // (e.g. the originating JS `Blob`) only touch + // this `Store` on the same thread, so no + // aliasing `&`/`&mut Store` is live. + unsafe { set_blob_content_type(blob, mime_type) }; // content_slice dropped (replaces defer content_slice.deinit()) } } if !blob.content_type_was_set.get() && blob.store.get().is_some() { - set_blob_content_type(blob, bun_http_types::MimeType::TEXT); + // SAFETY: synchronous JS-thread body-consumer + // continuation; no JS re-entry before the borrow + // ends, and other `StoreRef` clones only touch + // this `Store` on the same thread, so no aliasing + // `&`/`&mut Store` is live. + unsafe { set_blob_content_type(blob, bun_http_types::MimeType::TEXT) }; } promise.resolve(global, blob.to_js(global))?; } @@ -2190,12 +2214,20 @@ pub(crate) trait BodyMixin: BodyOwnerJs + Sized { if let Some(content_type) = fetch_headers.fast_get(HTTPHeaderName::ContentType) { let content_slice = content_type.to_slice(); let mime_type = MimeType::init(content_slice.slice(), true, None); - set_blob_content_type(blob, mime_type); + // SAFETY: synchronous JS-thread body-consumer + // continuation; no JS re-entry before the borrow ends, and + // other `StoreRef` clones only touch this `Store` on the + // same thread, so no aliasing `&`/`&mut Store` is live. + unsafe { set_blob_content_type(blob, mime_type) }; // content_slice dropped (replaces defer content_slice.deinit()) } } if !blob.content_type_was_set.get() && blob.store.get().is_some() { - set_blob_content_type(blob, bun_http_types::MimeType::TEXT); + // SAFETY: synchronous JS-thread body-consumer continuation; + // no JS re-entry before the borrow ends, and other `StoreRef` + // clones only touch this `Store` on the same thread, so no + // aliasing `&`/`&mut Store` is live. + unsafe { set_blob_content_type(blob, bun_http_types::MimeType::TEXT) }; } } Ok(JSPromise::resolved_promise_value( diff --git a/src/runtime/webcore/FileReader.rs b/src/runtime/webcore/FileReader.rs index 6af15b077aa0..ee2fd70629cb 100644 --- a/src/runtime/webcore/FileReader.rs +++ b/src/runtime/webcore/FileReader.rs @@ -97,9 +97,11 @@ pub type IOReader = BufferedReader; pub enum Lazy { None, - /// Intrusively-refcounted `*Blob.Store`. Uses `StoreRef` (not `Arc`) so the - /// raw pointer carries mutable provenance from `heap::alloc` for the - /// direct field writes in `open_file_blob`. + /// Intrusively-refcounted `*Blob.Store`. Uses `StoreRef` (not `Arc`) + /// because `Store` carries its own intrusive refcount and frees itself + /// when it hits zero; `Arc` would add a second, conflicting + /// refcount/deallocation path (see the `StoreRef` doc in + /// `webcore_types.rs`). Blob(blob::StoreRef), } @@ -317,16 +319,23 @@ impl FileReader { // on every path through the original `if let` body) so the `StoreRef` // is owned locally and the cell borrow is released immediately. if let Lazy::Blob(store) = self.lazy.replace(Lazy::None) { - // `StoreRef::data_mut` encapsulates the raw-pointer deref under the - // `StoreRef` liveness invariant (single-threaded JS event loop; we - // hold the only mutating handle). - match store.data_mut() { + // Clone the `File` out so `open_file_blob` takes `&mut File` on + // its own copy instead of `data_mut()` on the shared `Store` + // (other `StoreRef` clones to the same allocation exist). The + // clone is cheap; the `is_atty = Some(true)` cache write + // `open_file_blob` makes is intentionally discarded with the + // clone — writing it back would need `data_mut()` on an aliased + // handle, re-opening #30800. Cost: a repeat `isatty` probe on a + // second `.stream()` of `Bun.file(0|1|2)` (the canonical stdio + // Stores are built with `is_atty` pre-populated). + match &store.data { blob::store::Data::S3(_) | blob::store::Data::Bytes(_) => { panic!("Invalid state in FileReader: expected file ") } blob::store::Data::File(file) => { - let open_result = Lazy::open_file_blob(file); - // drop the StoreRef; `lazy` was already cleared above + let mut file_local = file.clone(); + let open_result = Lazy::open_file_blob(&mut file_local); + // drop the StoreRef (Zig: this.lazy.blob.deref()); `lazy` was already cleared above drop(store); match open_result { Err(err) => { diff --git a/src/runtime/webcore/blob/copy_file.rs b/src/runtime/webcore/blob/copy_file.rs index 4c6ba47e57d7..45a837fffd92 100644 --- a/src/runtime/webcore/blob/copy_file.rs +++ b/src/runtime/webcore/blob/copy_file.rs @@ -1392,7 +1392,7 @@ impl<'a> CopyFileWindows<'a> { } fn prepare_pathlike( - pathlike: &mut PathOrFileDescriptor, + pathlike: &PathOrFileDescriptor, must_close: &mut bool, is_reading: bool, ) -> bun_sys::Result { @@ -1435,12 +1435,10 @@ impl<'a> CopyFileWindows<'a> { // Open the destination first, so that if we need to call // mkdirp(), we don't spend extra time opening the file handle for // the source. + // `prepare_pathlike` only reads `pathlike` — shared borrow through + // `StoreRef: Deref` is sufficient; no `data_mut()` needed. self.read_write_loop.destination_fd = match Self::prepare_pathlike( - &mut self - .destination_file_store - .data_mut() - .as_file_mut() - .pathlike, + &self.destination_file_store.data.as_file().pathlike, &mut self.read_write_loop.must_close_destination_fd, false, ) { @@ -1457,7 +1455,7 @@ impl<'a> CopyFileWindows<'a> { }; self.read_write_loop.source_fd = match Self::prepare_pathlike( - &mut self.source_file_store.data_mut().as_file_mut().pathlike, + &self.source_file_store.data.as_file().pathlike, &mut self.read_write_loop.must_close_source_fd, true, ) { diff --git a/src/runtime/webcore/blob/read_file.rs b/src/runtime/webcore/blob/read_file.rs index e009e7fca39f..b764c0a18ee3 100644 --- a/src/runtime/webcore/blob/read_file.rs +++ b/src/runtime/webcore/blob/read_file.rs @@ -692,9 +692,15 @@ impl ReadFile { }; if let Some(store) = &self.store { - if let Data::File(file) = store.data_mut() { + // Shared borrow: the only write is the `AtomicU64` + // `last_modified`, so sibling worker tasks holding `&Data` to + // the same allocation stay sound. + if let Data::File(file) = &store.data { let mtime = bun_sys::PosixStat::init(&stat).mtime(); - file.last_modified = jsc::to_js_time(mtime.sec as isize, mtime.nsec as isize); + file.last_modified.store( + jsc::to_js_time(mtime.sec as isize, mtime.nsec as isize), + core::sync::atomic::Ordering::Relaxed, + ); } } @@ -1218,11 +1224,14 @@ impl<'a> ReadFileUV<'a> { let stat = this.req.statbuf; // keep in sync with resolveSizeAndLastModified - if let Data::File(file) = this.store.data_mut() { + // Shared borrow: the only write is the `AtomicU64` `last_modified`. + if let Data::File(file) = &this.store.data { // `uv_timespec_t` fields are `c_long` (i32 on Windows); widen to the // platform-width `isize` `to_js_time` expects. - file.last_modified = - jsc::to_js_time(stat.mtime().sec as isize, stat.mtime().nsec as isize); + file.last_modified.store( + jsc::to_js_time(stat.mtime().sec as isize, stat.mtime().nsec as isize), + core::sync::atomic::Ordering::Relaxed, + ); } if bun_sys::S::ISDIR(u32::try_from(stat.mode()).expect("int cast")) { diff --git a/test/internal/source-lints/storeref-not-sync.test.ts b/test/internal/source-lints/storeref-not-sync.test.ts new file mode 100644 index 000000000000..72b07743576f --- /dev/null +++ b/test/internal/source-lints/storeref-not-sync.test.ts @@ -0,0 +1,54 @@ +// Source-level guard for the `StoreRef` soundness contract (oven-sh/bun#30800). +// +// `StoreRef::data_mut(&self) -> &mut Data` hands out a mutable borrow through +// a shared, clonable handle. Combined with `unsafe impl Sync for StoreRef`, +// two threads sharing `&StoreRef` could each mint `&mut Data` to the same +// heap allocation through a safe API — immediate UB. The fix keeps `Send` +// (move-based cross-thread use), drops `Sync`, makes `data_mut` an +// `unsafe fn` whose precondition is borrow exclusivity, and adds the +// `__store_ref_not_sync` compile-time trip-wire so a future +// `unsafe impl Sync for StoreRef` fails the build with conflicting impls. +// +// The trip-wire catches regressions at compile time; this test is the +// suite-level projection of the same invariants, with a readable failure +// message instead of a rustc diagnostic. Like its sibling +// `dead-code-escapes.test.ts`, it asserts on the source text. +// (Booleans are extracted first so a failure prints `true`/`false`, not the +// whole file.) + +import { expect, test } from "bun:test"; +import path from "path"; + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const source = await Bun.file(path.join(root, "src", "jsc", "webcore_types.rs")).text(); + +test("StoreRef does not implement Sync", () => { + // The original #30800 hole. Cross-thread mutation must go through cloned + // (moved) handles whose call sites discharge `data_mut`'s exclusivity + // precondition — never through a shared `&StoreRef`. + // + // Anchored to the start of a line (`^\s*unsafe`) so `// `-prefixed prose — + // e.g. the trip-wire comment in `webcore_types.rs`, which quotes the exact + // phrase — can never match. + const hasSyncImpl = /^\s*unsafe impl\s+Sync\s+for\s+StoreRef\b/m.test(source); + expect(hasSyncImpl).toBe(false); +}); + +test("StoreRef::data_mut is an unsafe fn", () => { + // The precondition-bearing signature: every call site must assert, in an + // `unsafe` block, that no aliasing `&`/`&mut` to the pointee is live. + const hasUnsafeDataMut = /pub unsafe fn data_mut\s*\(\s*&self\s*\)/.test(source); + expect(hasUnsafeDataMut).toBe(true); + // And the pre-#30800 safe spelling must not come back. + const hasSafeDataMut = /pub fn data_mut\s*\(\s*&self\s*\)/.test(source); + expect(hasSafeDataMut).toBe(false); +}); + +test("the __store_ref_not_sync compile-time trip-wire is present", () => { + // If `StoreRef` ever gains `Sync`, both blanket impls of `_NotSyncCheck` + // apply and the trip-wire const fails to compile with conflicting impls. + const hasTripWireModule = source.includes("mod __store_ref_not_sync"); + expect(hasTripWireModule).toBe(true); + const hasTripWireConst = />::OK/.test(source); + expect(hasTripWireConst).toBe(true); +}); diff --git a/test/js/web/fetch/blob-write.test.ts b/test/js/web/fetch/blob-write.test.ts index 665f42bb3b78..8a13ff8848d5 100644 --- a/test/js/web/fetch/blob-write.test.ts +++ b/test/js/web/fetch/blob-write.test.ts @@ -136,3 +136,45 @@ test("Bun.file(path).write() silently ignores an invalid options.type", async () // the .txt default is kept expect(file.type).toBe("text/plain;charset=utf-8"); }); + +// Stress the threadpool `ReadFile` path that reaches into the backing +// `Store` off the JS thread (`resolve_size_and_last_modified` on each +// worker). `do_read_file` `StoreRef::clone`s the backing handle into each +// spawned `ReadFile` task, so N concurrent `file.bytes()` calls on the +// *same* `Blob` schedule N workers that all observe the same `Store` +// allocation. The write-target (`file.last_modified`) is `AtomicU64` on +// Rust's memory model and the only worker-thread write, so the race is +// idempotent (every task stores the same `fstat`-derived mtime). Shared +// (not exclusive) borrow through `StoreRef::Deref` is what keeps the +// `&mut` aliasing hazard away under `bun bd` (ASAN + Rust's UB rules). +// Regression guard for oven-sh/bun#30800 — `StoreRef` soundness (dropped +// `Sync`, `data_mut` is now `unsafe fn`, `last_modified` converted to +// atomic to match the threading reality). +test("Bun.file().bytes() is safe under high concurrency", async () => { + const dir = tempDirWithFiles( + "bun-blob-concurrent", + Object.fromEntries( + Array.from({ length: 16 }, (_, i) => [ + `f${i}.txt`, + `content-${i}-${Buffer.alloc(1024, 65 + (i % 26)).toString()}`, + ]), + ), + ); + // Many overlapping reads per file; each goes through a distinct `ReadFile` + // task on the threadpool, and all 8 per file share ONE `Store` (the + // `Blob` is constructed once and cloned via `StoreRef::clone`). The test + // asserts every task returns the full, uncorrupted file bytes — i.e. + // the shared `Store`'s content-reading paths don't trample each other. + const results = await Promise.all( + Array.from({ length: 16 }, (_, i) => { + const file = Bun.file(path.join(dir, `f${i}.txt`)); + return Promise.all(Array.from({ length: 8 }, () => file.bytes())); + }), + ); + for (let i = 0; i < results.length; i++) { + const expected = `content-${i}-${Buffer.alloc(1024, 65 + (i % 26)).toString()}`; + for (const bytes of results[i]) { + expect(new TextDecoder().decode(bytes)).toBe(expected); + } + } +});