Race Condition in WASM Code

[indolering]

I'm not a C programmer, but it appears as if this race condition (which was pointed out in a code review a few days after the WASM PR was merged) is still present. A comment states that this chunk of code is no longer used, but the buggy code should be removed or at least documented. I am unable to tell whether the problem was ported over to sbrk().

My apologies if this is incorrect or unhelpful, I performed a few searches and a manual review of commits but didn't turn up anything suggesting this had been fixed.

[daanx]

Hi @indolering, thanks for pointing this out.

The current code using the low-level wasm __builtin_wasm_memory_grow calls is unused and we currently use sbrk when compiled to wasm -- this relies on the (emscripten) libc implementation which seems more robust. This works great. Also, if using large allocations it is best to call mi_reserve_os_memory with a large size at the start of the program so mimalloc can manage the wasm memory in the best way (since wasm memory cannot be returned to the "OS").

However, the older code that calls directly __builtin_wasm_memory_grow and __builtin_wasm_memory_size is still correct I think and has no race condition; I think this is the case because WASM "threads" are web workers and should be completely isolated with only communication possible via SharedArrayBuffers (I could not find a definitive reference for this though, I'll ask the wasm experts soon). As such, I rather leave it in, just in case someone needs it (like using mimalloc with a minimal libc or something).

[indolering]

I'm not entirely sure what that code is doing, but @sunfishcode is one of WASM/WASI's architects. I'm inclined to believe them when they say that there is a race condition. But they are very busy, perhaps @jedisct1 could chime in?

If you don't have the resources to fix it, I agree that it would be better to leave this functionality in but it should be documented in the code.

[sunfishcode]

sbrk is supported in both Emscripten and wasi-sdk, and is a reasonable API to use.

The WebAssembly.Memory object that __builtin_wasm_memory_grow operates on can be shared between threads, such that one thread calling __builtin_wasm_memory_size followed by a separate __builtin_wasm_memory_grow call risks having another thread call __builtin_wasm_memory_grow in between, in which case the earlier __builtin_wasm_memory_size return value will point to memory that both threads will expect they allocated for themselves.

[daanx]

Thanks @sunfishcode for helping out. I understand the issue (and similar to issues with sbrk).

• I was under the impression that currently wasm does not have multiple threads, and that the only way to have concurrency is through webworkers which should be fully isolated. I thought that the wasm threads thus can only communicate a SharedArrayBuffer. It seems though that you imply that is not the case and that all workers can share the same memory ? (and thus can observe each others wasm_memory_grow calls?). I guess this is the actual proposal right? : https://github.com/WebAssembly/threads/blob/main/proposals/threads/Overview.md
• Anyway, if wasm_memory_grow can indeed be accessed by multiple threads I should surely fix it and use the return value. It is a bit troublesome though as mimalloc wants to allocate aligned memory (on say a 4MiB boundary). That means that without a way to return memory, mimalloc must always be conservative and overallocate by (4MiB - 64KiB) -- that is not great. Perhaps we can still amend the threads proposal to allow for an alignment parameter to wasm_memory_grow :-)

Thanks again

[sunfishcode]

Yes, that's the proposal; see the resizing section: after a memory.grow (formerly known as grow_memory), "All agents in the executing agent's cluster will have access to the additional linear memory". Web Workers are isolated from each other in general, but with the wasm threads feature, they share a linear memory.

Explicit synchronization between threads would be another option. At a quick scan of the code, it appears mimalloc doesn't use mutexes, and has its own atomics and spinning, so it's not immediately clear to me what to suggest here.

Perhaps we can still amend the threads proposal to allow for an alignment parameter to wasm_memory_grow :-)

The memory.grow instruction encoding does have a reserved immediate field, so there is a technical way to add an alignment parameter to it, if someone wanted to propose that. It's an interesting question whether it would be a good idea for wasm in general though; wasm instances often live in the same OS process as other wasm instances and other things, so the cost for each wasm instance allocating padding to achieve 4 MiB alignment for itself is more noticeable than in native code that has a whole OS process to itself.

[daanx]

Thanks @sunfishcode for the feedback. Yeah, the current API is quite bad for getting aligned allocation; I now protect with a lock and try opportunistic alignment but things can go bad. (see the code).

It would be great to extend the WASM instruction with the ability to allocate aligned memory; I am on the WASM stack committee (working on having first-class continuations/algebraic effects, proposal 1359) but I am not familiar with the design considerations for memory allocation. Especially for wasm64, an mmap like API with commit/decommit would be much better for modern allocators.
And as you remarked, even for wasm32 the alignment issue can be quite problematic given the limited area available (and having no way to release/ungrow memory)