Bug 303387 – REGRESSION (299880@main): WASM memory size cache makes bulk memory operations across threads fail

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[Martin Haug]

In commit https://commits.webkit.org/299880@main, a memory size cache (m_cachedMemorySize) was introduced to speed up the bulk memory operations memory.fill and memory.copy.

However, when used together with shared memory, the cached size observed in bulk memory operations can lag behind the actual allocated memory amount, causing valid operations to fail.

I have first observed this behavior in Typst (e.g. on https://typst.app/play, where an out of bounds memory access is reported on the console). 299880@main is the first revision exhibiting this problem, it works fine in 299879@main.

This patch resolves the problem. However, it walks the pointers to the underlying memory object to determine its type, an operation that the cache was likely designed to avoid:

```
 Source/JavaScriptCore/wasm/WasmOperationsInlines.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Source/JavaScriptCore/wasm/WasmOperationsInlines.h b/Source/JavaScriptCore/wasm/WasmOperationsInlines.h
index bc3d2676a7d8..a2fa04b68ce3 100644
--- a/Source/JavaScriptCore/wasm/WasmOperationsInlines.h
+++ b/Source/JavaScriptCore/wasm/WasmOperationsInlines.h
@@ -661,6 +661,9 @@ inline bool memoryInit(JSWebAssemblyInstance* instance, unsigned dataSegmentInde
 
 inline bool memoryFill(JSWebAssemblyInstance* instance, uint32_t dstAddress, uint32_t targetValue, uint32_t count)
 {
+    if (instance->memory()->memory().sharingMode() == MemorySharingMode::Shared)
+        return instance->memory()->memory().fill(dstAddress, static_cast<uint8_t>(targetValue), count);
+
     auto* base = std::bit_cast<uint8_t*>(instance->cachedMemory());
     uint64_t size = instance->cachedMemorySize();
 
@@ -674,6 +677,9 @@ inline bool memoryFill(JSWebAssemblyInstance* instance, uint32_t dstAddress, uin
 
 inline bool memoryCopy(JSWebAssemblyInstance* instance, uint32_t dstAddress, uint32_t srcAddress, uint32_t count)
 {
+    if (instance->memory()->memory().sharingMode() == MemorySharingMode::Shared)
+        return instance->memory()->memory().copy(dstAddress, srcAddress, count);
+
     auto* base = std::bit_cast<uint8_t*>(instance->cachedMemory());
     uint64_t size = instance->cachedMemorySize();
```

[Laurenz Mädje]

Created attachment 477624 [details]
Minimal reproducible example

Minimal reproducible example, also available at https://github.com/laurmaedje/safari-bulk-bug.

[Laurenz Mädje]

I created a minimal reproducible example for the bug at https://github.com/laurmaedje/safari-bulk-bug (also attached it to this bug).

From my current understanding the bug affects every application that uses multi-threaded WebAssembly with runtime-growable memory and bulk memory operations. If no action is taken, the bug will probably hit stable Safari very soon (alongside macOS 26.2). I'm not sure what the policies are for WebKit, but I'd like to suggest that, if still possible, the commit is reverted before it hits stable, since it was only an optimization anyway.

In the meantime, we'll try to work on a workaround, but since LLD requires bulk memory operations to be enabled when atomics are[^1], disabling bulk memory ops is typically not an option either. Our current idea is to attempt allocating enough memory upfront (via `initial`, with exponential increase upon crash), and patching Rust's memory allocator to take these initial memory pages into account rather than always calling `memory.grow`.

[^1]: https://github.com/llvm/llvm-project/blob/e7f3226e4f70f502cbd60ca5e999a6680850a50e/lld/wasm/Writer.cpp#L636-L640

[Keith Miller]

Thanks for the report, I'll take a look.

[Radar WebKit Bug Importer]

<rdar://problem/165913114>

[Yusuke Suzuki]

Pull request: https://github.com/WebKit/WebKit/pull/55012

[EWS]

Committed 304204@main (597e00851c71): <https://commits.webkit.org/304204@main>

Reviewed commits have been landed. Closing PR #55012 and removing active labels.