3400ff6aa7b63e840b7cecd229e38e2c19e538ae  Don't share StringImpl for worker names

The bug at a glance

The change removes cross-thread sharing of a WTF StringImpl between the thread that creates a WebAssembly.Module and the JSC Wasm compiler threads that read its import-name comparison strings, where StringImpl’s refcount and interior fields are not all atomic — a data race that can in principle corrupt the refcount and lead to a premature free / use-after-free. The author explicitly characterizes the current site as benign, and no in-the-wild trigger or memory-safety failure is demonstrated, so this is a hardening/hygiene fix; the Medium/6.5 rating reflects the theoretical cross-thread refcount UAF potential of StringImpl sharing rather than a proven exploitable primitive. Because the shared object was an isolatedCopy() intended precisely to avoid sharing, real-world reachability of the race is narrow.

WebAssembly compilation is multi-threaded: a module’s import-name comparison data is read on JSC compiler/worker threads while the module is built on the requesting thread. The old code stored the module’s importedStringConstants and builtin-set names as WTF Strings created with isolatedCopy(), on the theory that an isolated copy is safe to hand across threads — but StringImpl is not fully atomic (its refcount and some fields), so any StringImpl that ends up shared across threads can race its refcount into a premature free. The fix stops sharing StringImpl entirely: it eagerly transcodes those names into Name (a plain UTF-8 byte vector) at module-creation time and compares raw bytes on the compiler threads, so no StringImpl ever crosses a thread boundary. As a bonus it makes import-name matching honor exact UTF-8 semantics, which the added test pins down.

Root cause

ModuleInformation carried the imported string-constant module name and the qualified builtin-set names as std::optional<String> m_importedStringConstants and Vector<String> m_qualifiedBuiltinSetNames. applyCompileOptions() populated them with constants->isolatedCopy() and name.isolatedCopy(), with an ASSERT trying to guarantee the copies were neither atom nor symbol (except the canonical empty string). These fields are then read during Wasm compilation — populateImportShouldBeHidden() and, later, WebAssemblyModuleRecord::initializeImports()/findEnabledBuiltinSet() — to test whether each import.module matches the string-constants name or a builtin set.

The hazard is that these String/StringImpl objects are read from JSC Wasm compiler threads while owned by the module-creating thread. WTF::StringImpl does not yet make all of its fields (including refcount manipulation in every path) atomic, so a StringImpl that is genuinely shared between two threads can have its reference count updated non-atomically from both sides. A lost update to the refcount can drop it to zero early and free the StringImpl while another thread still references it — a cross-thread refcount use-after-free. isolatedCopy() was meant to prevent sharing, but it does not change the fundamental non-atomicity, and the safety of the site rested on fragile invariants (the very ASSERTs in the code).

The fix removes StringImpl from the cross-thread path altogether. A new helper encodeAsImportName(const String&) calls string.tryGetUTF8(StrictConversion) and, on success, returns a Name (a Vector of char8_t holding the raw UTF-8 bytes); StrictConversion means a String that cannot be encoded as well-formed UTF-8 (e.g. a lone surrogate) yields std::nullopt and simply cannot match any import name. applyCompileOptions() now stores std::optional<Name> m_importedStringConstants and Vector<Name> m_qualifiedBuiltinSetNames, built from encodeAsImportName. The comparison helpers importedStringConstantsEquals() and builtinSetsInclude() now take a const Name& (the import.module, itself raw UTF-8 bytes) and compare byte vectors, so the compiler threads only ever touch Name byte data — never a shared StringImpl.

Callers are updated to pass import.module (a Name) directly instead of materializing a String via makeString(import.module): populateImportShouldBeHidden(), findEnabledBuiltinSet(), and initializeImports(). A String is only materialized (makeString(import.module)) where genuinely required, e.g. WebAssemblyBuiltinRegistry::findByQualifiedName(). Because import names in the binary are already raw UTF-8, comparing in the UTF-8 domain is also semantically more correct than the prior String comparison, which the new JSTests/wasm/stress/wasm-imported-string-constants-utf8.js exercises (café, replacement char, lone-surrogate no-match, and builtin-set name cases).

Key code

Store import names as UTF-8 Name byte vectors instead of shared StringImpl-backed Strings

+static std::optional<Name> encodeAsImportName(const String& string)
+{
+    auto utf8 = string.tryGetUTF8(StrictConversion);
+    if (!utf8)
+        return std::nullopt;
+    return Name(byteCast<char8_t>(utf8->span()));
+}

     const auto& constants = options.importedStringConstants();
-    if (constants.has_value()) {
-        m_importedStringConstants = constants->isolatedCopy();
-        ASSERT(!(...isAtom() || ...isSymbol()) || ... == StringImpl::empty());
-    }
+    if (constants.has_value())
+        m_importedStringConstants = encodeAsImportName(*constants);

-    std::optional<String> m_importedStringConstants;
-    Vector<String> m_qualifiedBuiltinSetNames;
+    std::optional<Name> m_importedStringConstants;
+    Vector<Name> m_qualifiedBuiltinSetNames;

Patch walkthrough

• Source/JavaScriptCore/wasm/WasmModuleInformation.cpp — Adds static encodeAsImportName(const String&) which transcodes a String to UTF-8 via tryGetUTF8(StrictConversion) and returns std::optional<Name> (nullopt on non-UTF-8-encodable input). applyCompileOptions() now stores m_importedStringConstants and m_qualifiedBuiltinSetNames as encoded Name byte vectors rather than isolatedCopy() Strings, deleting the fragile atom/symbol ASSERTs. populateImportShouldBeHidden() compares import.module (a Name) directly against those Names and only calls makeString(import.module) when it must hand a String to WebAssemblyBuiltinRegistry::findByQualifiedName().
• Source/JavaScriptCore/wasm/WasmModuleInformation.h — Changes the member types from std::optional<String>/Vector<String> to std::optional<Name>/Vector<Name>, and retypes importedStringConstantsEquals()/builtinSetsInclude() to take const Name& and compare against the stored Names. This is the core of the fix: the objects crossing into compiler-thread reads are now plain UTF-8 byte vectors, not refcounted StringImpls.
• Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp — findEnabledBuiltinSet() and initializeImports() are updated to pass the import (import.module Name) instead of a pre-built String; the String is only reconstituted via makeString(import.module) at the registry lookup that requires it. This removes the last spots that would have created/compared Strings derived from the shared module names on the linking path.
• JSTests/wasm/stress/wasm-imported-string-constants-utf8.js — New stress test that builds modules with raw-UTF-8 import module names and checks importedStringConstants/builtins matching: café and U+FFFD match their UTF-8 encodings, empty matches empty, lone/embedded surrogates never match (no valid UTF-8), and non-equal ASCII does not match. It locks in the UTF-8-domain comparison semantics introduced by encodeAsImportName.

Background

WTF::StringImpl atomicity — StringImpl is refcounted but not fully thread-safe: not all of its fields/refcount paths are atomic. Sharing one StringImpl across threads risks non-atomic refcount updates, where a lost increment/decrement frees the buffer while another thread holds it — a classic cross-thread refcount UAF.

isolatedCopy() — Produces a String whose StringImpl is meant to be safe to pass to another thread by not sharing the original’s buffer. It does not make StringImpl atomic, so relying on it to move refcounted objects across threads is fragile — which is why the fix removes StringImpl from the cross-thread path entirely.

Wasm Name — In JSC’s Wasm code, Name is a vector of char8_t holding raw UTF-8 bytes as they appear in the module binary. Import module/field names are already Names, so comparing against Names avoids allocating/refcounting Strings and matches the on-the-wire encoding exactly.

importedStringConstants / builtin sets — WebAssembly JS String Builtins let a module import string constants and builtin function sets identified by a module name. The engine supplies these itself and hides them from WebAssembly.Module.imports(), so the name-matching must be both correct and thread-safe during multi-threaded compilation.

Vulnerability window

1. Module creation — Script calls new WebAssembly.Module(bytes, { importedStringConstants, builtins }); applyCompileOptions() records the constant module name and builtin-set names into ModuleInformation.
2. Pre-fix storage — Those names were stored as isolatedCopy() Strings (std::optional<String>, Vector<String>), backed by StringImpl objects, with ASSERTs guarding against atom/symbol impls.
3. Cross-thread read — JSC Wasm compiler/worker threads read these fields via importedStringConstantsEquals()/builtinSetsInclude() while comparing import names, sharing the StringImpl across the creating thread and compiler threads.
4. Latent race — Because StringImpl fields/refcount are not all atomic, a shared StringImpl could in principle have its refcount raced into an early free — a cross-thread UAF; the author notes the site is currently benign.
5. Fix — encodeAsImportName() transcodes names to Name (UTF-8 byte vectors) at creation time; members become std::optional<Name>/Vector<Name>; comparisons run over raw bytes so no StringImpl ever crosses threads.
6. Semantics pinned — The added JSTests case verifies UTF-8-domain matching (café, U+FFFD, empty match; lone/embedded surrogates and non-equal ASCII do not), guarding the behavioral change.

Triggering

The bug is a latent cross-thread StringImpl refcount race that the author themselves calls benign at this site; it is not deterministically reproducible from script and no memory-safety failure is demonstrated. The patch’s added test (wasm-imported-string-constants-utf8.js) is a correctness test for UTF-8 import-name matching, not a race reproducer, and reconstructing a reliable data-race PoC would require racing WebAssembly.Module compilation against StringImpl refcount churn with no guaranteed observable — fabricating such a primitive would misrepresent the fix, so no PoC is claimed.

Exploitation

1. Provoke sharing — An attacker would compile WebAssembly.Module instances with importedStringConstants/builtins whose names alias a StringImpl still referenced on another thread, hoping the isolatedCopy path leaves a StringImpl shared between the creating thread and Wasm compiler threads.
2. Race the refcount — Concurrent non-atomic refcount updates would need to be driven from both threads to lose an increment/decrement and free the StringImpl early. This is highly timing-dependent and, per the author, does not manifest at this site in practice, so no controlled UAF is established.

Detection & hunting

For defenders and SOC / detection engineers:

• ThreadSanitizer on Wasm compile —
• Shared non-atomic StringImpl audit —
• Import-name matching regressions —

Audit directions

• Other cross-thread String on Wasm paths —
• isolatedCopy()-as-thread-transfer —
• UTF-8 vs String comparison of on-wire names —