Never hand one StringImpl to two threads' atom tables (object URLs, BroadcastChannel, --compile workers)

src/CLAUDE.md

@@ -384,13 +384,21 @@ non-transferring path UAFs at GC.
 
 ### Cross-thread string hazards
 
-`AtomString`s live in a per-thread table. Never deref one from another thread —
-it trips `wasRemoved` in `AtomStringImpl::remove()`. If a `bun_core::String`
-may be dropped from a non-JS thread (HTTP worker, threadpool, dying VM), build
-it via `String::clone_utf8` (a plain `WTFStringImpl` with an atomic refcount),
-not from an interned/atomized JS string. See the comment in
-`src/runtime/webcore/fetch/FetchTasklet.rs` near `Response::init` for the
-canonical example of this bug class and its fix.
+`StringImpl` refcounts are atomic, but atom tables are per thread: using a
+string as a property key (`Identifier::fromString`) atomizes its impl _in
+place_ into the current thread's table, and the last `deref()` of an atom
+removes it from the current thread's table (`RELEASE_ASSERT(wasRemoved)` if it
+is not there). So an atomizable impl must never be reachable from two VMs —
+not via a process-global registry handing out `String::clone()`s, not via one
+`SerializedScriptValue` deserialized by several receivers. Hand another thread
+its own bytes (`Box<[u8]>` / `clone_utf8` on arrival), or share a
+`String::share()`d / `Bun::isolatedCopyForSharing` / `toCrossThreadShareable`
+string (pre-hashed and marked never-atomize, so a receiver's atom table copies
+it instead; see `src/jsc/bindings/BunString.cpp`) or a static string — make it
+shared once, then hand it out with plain `clone()`. `String::to_thread_safe()`
+is a plain isolated copy for handing a value to _one_ other owner.
+`ObjectURLRegistry`, `StandaloneModuleGraph::File` and the structured-clone
+object fast paths are the worked examples.
 
 ## Common Patterns
 

src/bun_core/string/mod.rs

@@ -104,6 +104,7 @@ unsafe extern "C" {
     // fields in place; the type encodes the sole pointer-validity precondition,
     // so `safe fn` discharges the link-time proof here.
     safe fn BunString__toThreadSafe(this: &mut String);
+    safe fn BunString__share(this: &mut String);
     fn BunString__createAtom(bytes: *const u8, len: usize) -> String;
     fn BunString__tryCreateAtom(bytes: *const u8, len: usize) -> String;
     fn BunString__createStaticExternal(bytes: *const u8, len: usize, isLatin1: bool) -> String;
@@ -507,16 +508,27 @@ impl String {
             core::ptr::null_mut()
         }
     }
+    /// Replace a WTF-backed impl with an isolated copy, for handing the value
+    /// to one other thread; not for sharing one impl between VMs.
     pub fn to_thread_safe(&mut self) {
         if self.tag == Tag::WTFStringImpl {
             BunString__toThreadSafe(self)
         }
         debug_assert!(self.is_thread_safe());
     }
-    /// True iff this `String` may be sent to / shared with another thread
-    /// without racing the WTF `StringImpl`'s non-atomic refcount: every tag
-    /// except `WTFStringImpl` is inert (raw slice / static / dead), and a
-    /// WTF-backed string is safe iff its impl reports `isThreadSafe()`.
+    /// Make a WTF-backed impl safe to hand to any number of threads/VMs:
+    /// isolated if it was an atom/symbol/substring, pre-hashed, and never
+    /// atomized in place; hand it out with `clone()`.
+    pub fn share(&mut self) {
+        if self.tag == Tag::WTFStringImpl {
+            BunString__share(self)
+        }
+        debug_assert!(self.is_thread_safe());
+    }
+    /// True iff this `String` may be handed to another thread as-is: every
+    /// tag except `WTFStringImpl` is inert (raw slice / static / dead), and a
+    /// WTF-backed string qualifies iff its impl is not an atom, symbol or
+    /// substring (`isThreadSafe()`).
     ///
     /// Call sites that move a `String` across a thread boundary must ensure
     /// this holds (typically by calling [`to_thread_safe`] first); see the
@@ -916,13 +928,11 @@ impl String {
         let mut out = self.into_utf8_with_string();
         if out.string.tag == Tag::WTFStringImpl {
             if out.utf8.is_none() {
-                // 8-bit all-ASCII: a thread-safe `string` keeps backing the bytes.
-                if out.string.is_thread_safe() {
-                    return out;
-                }
-                out.utf8 = Some(out.slice().to_vec());
+                // 8-bit all-ASCII: a shared `string` keeps backing the bytes.
+                out.string.share();
+                return out;
             }
-            // Transcoded or copied; drop the WTF backing to release memory.
+            // Transcoded; drop the WTF backing to release memory.
             out.string = String::EMPTY;
         }
         out
@@ -973,20 +983,16 @@ impl Default for String {
 }
 // SAFETY: `String` is a tag + raw ptr to a `WTF::StringImpl` (or a borrowed
 // `EncodedSlice` slice / static / dead sentinel). All non-WTF tags are trivially
-// `Send + Sync` (no interior mutability, no refcount). The WTF tag is the
-// hazard: `WTF::StringImpl`'s refcount is non-atomic unless the impl was
-// created thread-safe, so sending/sharing a non-thread-safe impl across
-// threads and then `ref_()`/`deref()`ing it is a data race.
+// `Send + Sync` (no interior mutability, no refcount). A `WTF::StringImpl`'s
+// refcount is atomic; the hazards are per-thread atom tables and the lazily
+// computed hash/flags (see "Cross-thread string hazards" in `src/CLAUDE.md`).
 //
 // We keep the blanket impls to match the C++ `BunString`
 // FFI contract (the type must round-trip by value through `extern "C"` and sit
 // in `Send + Sync` containers), and instead enforce the invariant at the
-// boundary: any code that moves a `String` to another thread MUST first call
-// [`String::to_thread_safe`] (or otherwise guarantee [`String::is_thread_safe`]
-// returns `true`). [`String::debug_assert_thread_safe`] is the debug-build
-// checkpoint for that hand-off; `to_thread_safe()` itself asserts its own
-// postcondition. A `ThreadSafeString` newtype split would make this static,
-// but is deferred until the FFI surface can be reshaped.
+// boundary: code that moves a `String` to one other thread calls
+// [`String::to_thread_safe`], code that shares one impl between threads/VMs
+// calls [`String::share`] (both assert [`String::is_thread_safe`]).
 unsafe impl Send for String {}
 // SAFETY: same contract as the `Send` impl above — sharing requires
 // `is_thread_safe()`; non-WTF tags are inert and trivially `Sync`.

src/bundler/lib.rs

@@ -364,7 +364,7 @@ bun_dispatch::link_interface! {
         fn is_blob_url(specifier: &[u8]) -> bool;
         fn resolve_blob(specifier: &[u8]) -> Option<options::OpaqueBlob>;
         fn blob_loader(blob: options::OpaqueBlob) -> Option<bun_ast::Loader>;
-        fn blob_file_name(blob: options::OpaqueBlob) -> Option<&'static [u8]>;
+        fn blob_store_path(blob: options::OpaqueBlob) -> Option<&'static [u8]>;
         fn blob_needs_read_file(blob: options::OpaqueBlob) -> bool;
         fn blob_shared_view(blob: options::OpaqueBlob) -> &'static [u8];
         fn blob_deinit(blob: options::OpaqueBlob);

src/bundler/options.rs

@@ -520,7 +520,7 @@ pub fn get_loader_and_virtual_source<'a>(
 
             // "file:" loader makes no sense for blobs
             // so let's default to tsx.
-            if let Some(filename) = jsc_vm.blob_file_name(blob) {
+            if let Some(filename) = jsc_vm.blob_store_path(blob) {
                 let current_path = Fs::Path::init(filename);
 
                 // Only treat it as a file if is a Bun.file()

src/http/AsyncHTTP.rs

@@ -3,7 +3,7 @@ use core::sync::atomic::{AtomicUsize, Ordering};
 
 use bun_ast::{Loc, Log};
 use bun_core::FeatureFlags;
-use bun_core::{MutableString, Utf8Bytes};
+use bun_core::MutableString;
 use bun_threading::IntrusiveWorkTask as _;
 use bun_threading::thread_pool::{self, Batch, Task};
 use bun_url::{PercentEncoding, URL};
@@ -24,9 +24,9 @@ bun_core::declare_scope!(AsyncHTTP, visible);
 
 // Lifetime `'a` covers every borrowed input the caller hands in: `url`,
 // `http_proxy`, `request_header_buf`, the borrowed `HTTPRequestBody::Bytes`
-// payload, and `client.{header_buf,hostname,if_modified_since}`. Intrusive
-// fields (`real`, `next`) are raw pointers and thus lifetime-erased; the
-// HTTP-thread copy uses the same `'a` as the JS-thread original it mirrors.
+// payload, and `client.{header_buf,hostname,unix_socket_path,if_modified_since}`.
+// Intrusive fields (`real`, `next`) are raw pointers and thus lifetime-erased;
+// the HTTP-thread copy uses the same `'a` as the JS-thread original it mirrors.
 pub struct AsyncHTTP<'a> {
     pub response: Option<picohttp::Response<'static>>,
     pub request_headers: headers::EntryList,
@@ -191,7 +191,7 @@ fn make_client<'a>(
         signals,
         async_http_id,
         hostname,
-        unix_socket_path: Utf8Bytes::EMPTY,
+        unix_socket_path: b"",
         compress: None,
         compressed_request_body: Vec::new(),
         compressed_body_len: 0,
@@ -245,7 +245,7 @@ pub struct Options<'a> {
     pub proxy_headers: Option<Headers>,
     pub hostname: Option<&'a [u8]>,
     pub signals: Option<Signals>,
-    pub unix_socket_path: Option<Utf8Bytes<'static>>,
+    pub unix_socket_path: Option<&'a [u8]>,
     pub disable_timeout: Option<bool>,
     /// Per-request idle timeout override in seconds; see
     /// `HTTPClient::idle_timeout_seconds`.
@@ -315,7 +315,6 @@ impl<'a> AsyncHTTP<'a> {
 
     pub fn clear_data(&mut self) {
         self.response = None;
-        self.client.unix_socket_path = Utf8Bytes::EMPTY;
     }
 }
 
@@ -461,7 +460,6 @@ impl<'a> AsyncHTTP<'a> {
             signals,
         };
         if let Some(val) = options.unix_socket_path {
-            debug_assert!(this.client.unix_socket_path.slice().is_empty());
             this.client.unix_socket_path = val;
         }
         if let Some(val) = options.disable_timeout {
@@ -711,11 +709,11 @@ impl<'a> AsyncHTTP<'a> {
                 // (`start_queued_task`), so any owned field that was already
                 // populated at that point — `request_headers`,
                 // `client.header_entries`, `client.proxy_headers`,
-                // `client.proxy_settings`, `client.tls_props`,
-                // `client.unix_socket_path` — is *shared* with the original
-                // and must NOT be dropped here; the original drops them when
-                // its `Box<AsyncHTTP>` is reclaimed. Only the state the clone
-                // built up itself during request processing is torn down.
+                // `client.proxy_settings`, `client.tls_props` — is *shared*
+                // with the original and must NOT be dropped here; the original
+                // drops them when its `Box<AsyncHTTP>` is reclaimed. Only the
+                // state the clone built up itself during request processing is
+                // torn down.
                 {
                     // `handle_response_metadata` rewrites per-hop request state in
                     // place: `client.url`/`connected_url` become self-borrows into

src/http/HTTPThread.rs

@@ -409,16 +409,9 @@ impl HttpThread {
             // funnels through here before touching `https_context.{group,secure}`.
             self.ensure_https_context_init();
         }
-        // Note: borrowck — `slice()` borrows `client`; capture into a
-        // `bun_ptr::RawSlice` (encapsulated outlives-holder invariant) so the
-        // borrow of `client` ends before we hand `&mut client` to
-        // `connect_socket`. Backing storage is `client.unix_socket_path`, which
-        // `connect_socket` does not touch.
-        let unix_path = bun_ptr::RawSlice::new(client.unix_socket_path.slice());
+        let unix_path = client.unix_socket_path;
         if !unix_path.is_empty() {
-            return self
-                .context::<IS_SSL>()
-                .connect_socket(client, unix_path.slice());
+            return self.context::<IS_SSL>().connect_socket(client, unix_path);
         }
 
         if IS_SSL {

src/http/lib.rs

@@ -648,7 +648,6 @@ pub(crate) fn hash_header_name(name: &[u8]) -> u64 {
 // `bun_uws::NewSocketHandler` methods (`ext`/`timeout`/`raw_write`/`flush`/
 // `shutdown`/`connect_group`/…) land.
 
-use bun_core::Utf8Bytes;
 use bun_url::URL;
 use core::ptr::NonNull;
 
@@ -791,8 +790,8 @@ fn no_proxy_matches(no_proxy_text: &[u8], hostname: &[u8], host: &[u8]) -> bool
 // Many of these fields can be moved to a packed struct and use less space
 //
 // Lifetime `'a` ties every borrowed input — `url`, `http_proxy`, `header_buf`,
-// `if_modified_since`, `hostname`, and the borrowed `HTTPRequestBody::Bytes`
-// payload — to the caller's storage. The original port erased these to `'static`
+// `if_modified_since`, `hostname`, `unix_socket_path`, and the borrowed
+// `HTTPRequestBody::Bytes` payload — to the caller's storage. The original port erased these to `'static`
 // and lifetime-erased at every call site; threading the lifetime removes that hazard.
 // Intrusive raw-pointer backrefs (socket ext, h2/h3 streams) store the
 // lifetime-erased `HTTPClient<'static>` form via [`HTTPClient::as_erased_ptr`].
@@ -870,7 +869,7 @@ pub struct HTTPClient<'a> {
     pub(crate) signals: Signals,
     pub(crate) async_http_id: u32,
     pub(crate) hostname: Option<&'a [u8]>,
-    pub(crate) unix_socket_path: Utf8Bytes<'static>,
+    pub(crate) unix_socket_path: &'a [u8],
     /// `fetch({ compress })` — when set, the body is compressed lazily at
     /// write time (h1: `send_initial_request_payload`; h2/h3: at attach) so
     /// the output can borrow `LibdeflateState::shared_buffer`. Persists across
@@ -1943,7 +1942,7 @@ impl<'a> HTTPClient<'a> {
         // through a non-keepalive Agent or `agent: false` skip it, matching Node.
         //
         // TCP options do not apply to a unix socket.
-        if !self.flags.disable_keepalive && self.unix_socket_path.slice().is_empty() {
+        if !self.flags.disable_keepalive && self.unix_socket_path.is_empty() {
             let _ = socket.set_keep_alive(true, 60);
         }
 
@@ -1971,7 +1970,7 @@ impl<'a> HTTPClient<'a> {
         if self.flags.is_preconnect_only {
             return false;
         }
-        if self.unix_socket_path.slice().len() > 0 {
+        if !self.unix_socket_path.is_empty() {
             return false;
         }
         if matches!(
@@ -2020,7 +2019,7 @@ impl<'a> HTTPClient<'a> {
         if self.flags.is_preconnect_only {
             return false;
         }
-        if self.unix_socket_path.slice().len() > 0 {
+        if !self.unix_socket_path.is_empty() {
             return false;
         }
         if matches!(
@@ -2259,7 +2258,7 @@ impl<'a> HTTPClient<'a> {
     pub(crate) fn is_keep_alive_possible(&self) -> bool {
         if FeatureFlags::ENABLE_KEEPALIVE {
             // TODO keepalive for unix sockets
-            if self.unix_socket_path.slice().len() > 0 {
+            if !self.unix_socket_path.is_empty() {
                 return false;
             }
             // check state
@@ -2633,15 +2632,9 @@ impl<'a> HTTPClient<'a> {
             self.flags.is_streaming_request_body = false;
         }
 
-        // Decided before unix_socket_path is forgotten below: a unix-socket connection must not be pooled.
+        // Decided before unix_socket_path is cleared below: a unix-socket connection must not be pooled.
         let keep_alive_possible = self.is_keep_alive_possible();
-        // There is no struct copy-back
-        // (`sync_progress_from` skips owned fields) and the original retains
-        // its own `Owned(Vec)` aliasing the same allocation (the HTTP-thread
-        // clone was created via `ptr::read`). Dropping it here would
-        // double-free when the original later runs `clear_data()`. Forget the
-        // clone's view; the original is the sole owner.
-        let _ = core::mem::ManuallyDrop::new(core::mem::take(&mut self.unix_socket_path));
+        self.unix_socket_path = b"";
         // TODO: what we do with stream body?
         let request_body: &[u8] = if self.state.flags.resend_request_body_on_redirect
             && matches!(self.state.original_request_body, HTTPRequestBody::Bytes(_))
@@ -2863,7 +2856,7 @@ impl<'a> HTTPClient<'a> {
                 self.complete_connecting_process();
                 return;
             }
-            if self.http_proxy.is_some() || self.unix_socket_path.slice().len() > 0 {
+            if self.http_proxy.is_some() || !self.unix_socket_path.is_empty() {
                 self.fail(crate::Error::HTTP3Unsupported);
                 self.complete_connecting_process();
                 return;
@@ -4357,10 +4350,7 @@ impl<'a> HTTPClient<'a> {
         if matches!(self.state.original_request_body, HTTPRequestBody::Stream(_)) {
             self.flags.is_streaming_request_body = false;
         }
-        // See `do_redirect`: the HTTP-thread clone shares this allocation
-        // with the JS-thread original (created via `ptr::read`); dropping it
-        // here double-frees once the original runs `clear_data()`.
-        let _ = core::mem::ManuallyDrop::new(core::mem::take(&mut self.unix_socket_path));
+        self.unix_socket_path = b"";
         let request_body: &[u8] = if self.state.flags.resend_request_body_on_redirect
             && matches!(self.state.original_request_body, HTTPRequestBody::Bytes(_))
         {
@@ -4953,7 +4943,7 @@ impl<'a> HTTPClient<'a> {
                     // request to the same origin may be h3-eligible even if this
                     // one was pinned/proxied/sendfile.
                     if self.is_https()
-                        && self.unix_socket_path.slice().len() == 0
+                        && self.unix_socket_path.is_empty()
                         && !(self.flags.proxy_tunneling && self.proxy_tunnel.is_none())
                         && h3_alt_svc_enabled()
                     {

src/http/session_cache.rs

@@ -182,7 +182,7 @@ unsafe extern "C" {
 pub(crate) fn eligible(client: &crate::HTTPClient<'_>) -> bool {
     client.flags.reject_unauthorized
         && !client.signals.get(signals::Field::CertErrors)
-        && client.unix_socket_path.slice().is_empty()
+        && client.unix_socket_path.is_empty()
         && !bun_core::env_var::feature_flag::BUN_FEATURE_FLAG_DISABLE_FETCH_TLS_SESSION_CACHE
             .get()
             .unwrap_or(false)