windows with_exclusive_access implementation is not threadsafe

[froydnj]

Summary

The Windows implementation of with_exclusive_access is not safe with respect to threads.

Thought experiment to reproduce

Consider the code:

https://github.com/rustwasm/wee_alloc/blob/82b60dcfd01d29b3c3535d59cb7931f3b3c945f9/wee_alloc/src/imp_windows.rs#L49-L54

If we have two threads requesting exclusive access, then something akin to the following can happen:

T1: Checks for the lock at line 50, finds it to be NULL. T1: Executes any code up to the point of actually storing the CreateMutexW return value. T1: Gets suspended. T2: Checks for the lock at line 50, finds it to be NULL.

We're now in trouble, because we have two threads that are trying to create the mutex for a single object, and will therefore be confused about who may access the data.

https://github.com/rustwasm/wee_alloc/blob/82b60dcfd01d29b3c3535d59cb7931f3b3c945f9/wee_alloc/src/imp_windows.rs#L56-L66

T2: Creates the lock, locks the mutex at line 56. T2: Executes any code up to line 65, where the mutex is released. T2: Gets suspended. T1: Resumes, stores (and overwrites!) the lock with its mutex. T1: Continues executing, possibly running f concurrently with T2, depending on where T2 was stopped.

This is a pretty unlikely scenario, as it can only happen on the very first exclusive access, and then only with some particular scheduling decisions on the part of the OS.

I don't believe the pthreads implementation is vulnerable to this, as the Exclusive<T> value's lock is initialized during creation, rather than at runtime during with_exclusive_access. It's possible that the semantics of UnsafeCell make the scenario above not work; I'm not 100% sure either way since I'm not that familiar with UnsafeCell. But looking at UnsafeCell's implementation didn't turn up anything that would make this safe.

A straightforward way to fix this on the Windows size would be to use slim reader/writer locks, which can be initialized with 0 at Exclusive<T> creation time, rather than requiring a runtime call like critical sections or mutexes.

[fitzgen]

Thanks for the detailed report @froydnj :)

+cc @DrGoldfire

[DrGoldfire]

I like your suggestion for fixing this; the pthreads version isn't vulnerable because it can statically initialize its mutex, and an SRW can be initialized the same way, so I agree that should resolve the issue, and that's what I'll try to do.