Windows: Mutex not properly initialized

[RalfJung]

On Windows, a mutex is initialized with

        Mutex {
            lock: AtomicUsize::new(0),
            held: UnsafeCell::new(false),
        }

and then, if kind() determines we are using SRWLock, the next thing that could happen is AcquireSRWLockExclusive. However, the documentation for InitializeSRWLock says that the lock must be initialized before it is used.

I am not at all an expert in the Windows API, but -- aren't we relying on undocumented behavior here, namely, that setting the lock to 0 is a proper way to initialize it?

[RalfJung]

Seems like there is a bigger problem: We actually invoke UB on Windows by attempting to recursively lock an SRWLock. See #35836.

[retep998]

To initialize the structure statically, assign the constant SRWLOCK_INIT to the structure variable.

#define SRWLOCK_INIT RTL_SRWLOCK_INIT
#define RTL_SRWLOCK_INIT {0}

Therefore zero initializing the lock is in fact a valid way of initializing the lock.

[RalfJung]

Well, that is exploiting an implementation detail.

Admittedly, they can't really ever change this since it gets embedded in the compiled programs, but still -- we could at least have an assert! testing that SRWLOCK_INIT is 0.

[retep998]

What would the point of an assert! be given that Rust code is defining that constant itself? We're not dynamically fetching that value from the headers.

The value of a constant is not an implementation detail, given that Microsoft provides strong stability guarantees about the values of constants in the headers. Therefore I fail to see any issue here. Besides, if we want to be able to initialize a lock in a constant expression, we have to use the constant and not the runtime function

[sanmai-NL]

@retep998: agree, but what about using a constant rather than hardcoded zero and/or a comment with your explanation?

[retep998]

Feel free to submit a PR that adds comments to that effect.

[RalfJung]

What would the point of an assert! be given that Rust code is defining that constant itself? We're not dynamically fetching that value from the headers.

It's different Rust code, so this is at least a basic sanity check.