Re: [PATCH] ntdll: Make RtlDeregisterWaitEx(handle, INVALID_HANDLE_VALUE) thread safe.

[Stefan Dösinger]

Hi,

Is the test I was referring to in the description of the previous patch.
It causes seemingly unrelated test failures like
https://testbot.winehq.org/JobDetails.pl?Key=32956 (previous iteration
of the test that used a different implementation)

Stefan

Am 2017-09-05 um 17:34 schrieb Stefan Dösinger:
> Chromium signals the wait semaphore and calls DeregisterWaitEx with
> CompletionHandle = INVALID_HANDLE_VALUE in close succession. Sometimes
> the worker thread decides to run the callback, but before it sets
> CallbackInProgress RtlDeregisterWaitEx decides that the callback is not
> running and returns STATUS_SUCCESS. Chromium then releases resources
> that the callback needs to run, resulting in random crashes.
>
> Signed-off-by: Stefan Dösinger
>
> The tests show that we're only supposed to return STATUS_PENDING if the
> callback is running. Note that the choice between waiting and returning
> STATUS_SUCCESS and not waiting and returning STATUS_PENDING is still
> racy, but we'll no longer return STATUS_SUCCESS and run the callback
> afterwards.
>
> I tried to extend the tests to show what happens when both events are
> available simultanously. The answer is that it is fairly random.
> INVALID_HANDLE_VALUE blocks and runs the callback one last time, the
> other ways of calling RtlDeregisterWait[Ex] randomly return STATUS_SUCCESS
> and do not run the callback, or they return STATUS_PENDING and may or
> may not run the callback. However, in no circumstance does the callback
> run after STATUS_SUCCESS is returned.
>
> I am not including the tests because of the random outcome and because
> it plays with SuspendThread and makes the random failures in follow up
> tests worse. I'll send it to wine-devel for reference.
>
> dlls/ntdll/threadpool.c | 59 +++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c
> index 6063d51d9f..1b0546a781 100644
> --- a/dlls/ntdll/threadpool.c
> +++ b/dlls/ntdll/threadpool.c
> @@ -541,11 +541,13 @@ static DWORD CALLBACK wait_thread_proc(LPVOID Arg)
>              break;
>      }
> -
>
> completion_event = wait_work_item->CompletionEvent;
> -
>
> if (completion_event) NtSetEvent( completion_event, NULL );
>
> if (interlocked_inc( &wait_work_item->DeleteCount ) == 2 )
> -
>
> {
> -
>    completion_event = wait_work_item->CompletionEvent;
>    delete_wait_work_item( wait_work_item );
> -
>    if (completion_event) NtSetEvent( completion_event, NULL );
> -
>
> }
>
> return 0;
>
> }
> @@ -633,44 +635,47 @@ NTSTATUS WINAPI RtlRegisterWait(PHANDLE NewWaitObject, HANDLE Object,
>  NTSTATUS WINAPI RtlDeregisterWaitEx(HANDLE WaitHandle, HANDLE CompletionEvent)
>  {
>      struct wait_work_item *wait_work_item = WaitHandle;
> - NTSTATUS status = STATUS_SUCCESS;
> - NTSTATUS status;
> - HANDLE LocalEvent = NULL;
> - BOOLEAN CallbackInProgress;
> - TRACE( "(%p)\n", WaitHandle );
> -
>
> TRACE( "(%p %p)\n", WaitHandle, CompletionEvent );
>
> if (WaitHandle == NULL)
>     return STATUS_INVALID_HANDLE;
> - NtSetEvent( wait_work_item->CancelEvent, NULL );
> - if (wait_work_item->CallbackInProgress)
> - CallbackInProgress = wait_work_item->CallbackInProgress;
> - if (CompletionEvent == INVALID_HANDLE_VALUE || !CallbackInProgress)
> {
> -
>    if (CompletionEvent != NULL)
> -
>    {
> -
>        if (CompletionEvent == INVALID_HANDLE_VALUE)
> -
>        {
> -
>            status = NtCreateEvent( &CompletionEvent, EVENT_ALL_ACCESS, NULL, NotificationEvent, FALSE );
> -
>            if (status != STATUS_SUCCESS)
> -
>                return status;
> -
>            interlocked_xchg_ptr( &wait_work_item->CompletionEvent, CompletionEvent );
> -
>            if (wait_work_item->CallbackInProgress)
> -
>                NtWaitForSingleObject( CompletionEvent, FALSE, NULL );
> -
>            NtClose( CompletionEvent );
> -
>        }
> -
>        else
> -
>        {
> -
>            interlocked_xchg_ptr( &wait_work_item->CompletionEvent, CompletionEvent );
> -
>            if (wait_work_item->CallbackInProgress)
> -
>                status = STATUS_PENDING;
> -
>        }
> -
>    }
> -
>    else
> -
>        status = STATUS_PENDING;
> -
>    status = NtCreateEvent( &LocalEvent, EVENT_ALL_ACCESS, NULL, NotificationEvent, FALSE );
> -
>    if (status != STATUS_SUCCESS)
> -
>        return status;
> -
>    interlocked_xchg_ptr( &wait_work_item->CompletionEvent, LocalEvent );
> -
>
> }
> -
>
> else if (CompletionEvent != NULL)
> -
>
> {
> -
>    interlocked_xchg_ptr( &wait_work_item->CompletionEvent, CompletionEvent );
>
> }
> -
>
> NtSetEvent( wait_work_item->CancelEvent, NULL );
> -
>
> if (interlocked_inc( &wait_work_item->DeleteCount ) == 2 )
>    {
>        status = STATUS_SUCCESS;
>        delete_wait_work_item( wait_work_item );
>    }
> -
>
> else if (LocalEvent)
> -
>
> {
> -
>    NtWaitForSingleObject( LocalEvent, FALSE, NULL );
> -
>    status = STATUS_SUCCESS;
> -
>
> }
> -
>
> else
> -
>
> {
> -
>    status = STATUS_PENDING;
> -
>
> }
> -
> -
>
> if (LocalEvent)
> -
>    NtClose( LocalEvent );
>
> return status;
>
> }