Re: [PATCH v13 0/2] MR3504: ntdll: An implementation of SRWLOCK that closer matches Windows'.

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/ntdll/sync.c:
> - struct srwlock_waiter *prev = NULL, waiter = {0};
> - for (;;)
> - tag  = srwlock_get_tag(expected);
> - TRACE("%p %p %p\n", lock, &waiter, expected.Ptr);
> -
> - waiter.state = waiter_state;
> - if (tag & SRWLOCK_TAG_HAS_WAITERS)
> - {
> -
>    prev = srwlock_get_waiter(expected);
> -
>    if (!(tag & SRWLOCK_TAG_LIST_LOCKED))
> -
>        waiter.head = prev->head;
> -
>    else
> -
>        /* some other thread is modifying the list, they
> -
>         * are responsible for setting our head pointer. */
> -
>        waiter.head = NULL;

This is pretty clearly broken, because the flag can get set immediately after we read it. But looking at the implementation, do we actually need to worry about the "locked" case? As far as I can tell srwlock_transfer_ownership() already correctly handles any case where the lock is concurrently updated.

(And if so, we can just throw out *all* of the "locking" code, right?)