[PATCH 1/3] ntdll: Use a separate wait handle for pending completion ports

[Andrew Eikum]

The problematic call sequence is:

Thread A:
  GetQueuedCompletionPortStatus(port); -> blocks

Thread B:
  PostQueuedCompletionPortStatus(port);
  CloseHandle(port);

Thread A:
  unblocks -> INVALID_HANDLE_VALUE

Tests on Windows show that the queued status is always returned to
thread A in this scenario. In Wine, it would sometimes fail due to the
CloseHandle being called before the queued status unblocked the thread,
causing sporadic failures in Mount & Blade: Bannerlord.

This patch works around this by returning a new handle in the
would-block case, which allows the blocked thread to read the last
queued status before destroying the object.

Signed-off-by: Andrew Eikum 
---
 dlls/ntdll/unix/sync.c | 32 ++++++++++++++++++++++++++------
 server/completion.c    |  7 +++++++
 server/protocol.def    |  2 ++
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c
index d5d92145503..7fad8250e43 100644
--- a/dlls/ntdll/unix/sync.c
+++ b/dlls/ntdll/unix/sync.c
@@ -1815,6 +1815,7 @@ NTSTATUS WINAPI NtRemoveIoCompletion( HANDLE handle, ULONG_PTR *key, ULONG_PTR *
                                       IO_STATUS_BLOCK *io, LARGE_INTEGER *timeout )
 {
     NTSTATUS status;
+    HANDLE wait_handle = handle;

TRACE( "(%p, %p, %p, %p, %p)\n", handle, key, value, io, timeout );

@@ -1822,7 +1823,8 @@ NTSTATUS WINAPI NtRemoveIoCompletion( HANDLE handle, ULONG_PTR *key, ULONG_PTR *
     {
         SERVER_START_REQ( remove_completion )
         {
-            req->handle = wine_server_obj_handle( handle );
+            req->handle = wine_server_obj_handle( wait_handle );
+            req->alloc_wait_handle = wait_handle == handle;
             if (!(status = wine_server_call( req )))
             {
                 *key            = reply->ckey;
@@ -1830,12 +1832,22 @@ NTSTATUS WINAPI NtRemoveIoCompletion( HANDLE handle, ULONG_PTR *key, ULONG_PTR *
                 io->Information = reply->information;
                 io->u.Status    = reply->status;
             }
+            else if (status == STATUS_PENDING)
+            {
+                wait_handle = wine_server_ptr_handle( reply->wait_handle );
+            }
         }
         SERVER_END_REQ;
-        if (status != STATUS_PENDING) return status;
-        status = NtWaitForSingleObject( handle, FALSE, timeout );
-        if (status != WAIT_OBJECT_0) return status;
+        if (status != STATUS_PENDING) goto exit;
+        status = NtWaitForSingleObject( wait_handle, FALSE, timeout );
+        if (status != WAIT_OBJECT_0) goto exit;
     }
+
+exit:
+    if (wait_handle != handle)
+        NtClose( wait_handle );
+
+    return status;
 }

@@ -1847,6 +1859,7 @@ NTSTATUS WINAPI NtRemoveIoCompletionEx( HANDLE handle, FILE_IO_COMPLETION_INFORM
 {
     NTSTATUS status;
     ULONG i = 0;
+    HANDLE wait_handle = handle;

TRACE( "%p %p %u %p %p %u\n", handle, info, count, written, timeout, alertable );

@@ -1856,7 +1869,8 @@ NTSTATUS WINAPI NtRemoveIoCompletionEx( HANDLE handle, FILE_IO_COMPLETION_INFORM
         {
             SERVER_START_REQ( remove_completion )
             {
-                req->handle = wine_server_obj_handle( handle );
+                req->handle = wine_server_obj_handle( wait_handle );
+                req->alloc_wait_handle = wait_handle == handle;
                 if (!(status = wine_server_call( req )))
                 {
                     info[i].CompletionKey             = reply->ckey;
@@ -1864,6 +1878,10 @@ NTSTATUS WINAPI NtRemoveIoCompletionEx( HANDLE handle, FILE_IO_COMPLETION_INFORM
                     info[i].IoStatusBlock.Information = reply->information;
                     info[i].IoStatusBlock.u.Status    = reply->status;
                 }
+                else if (status == STATUS_PENDING)
+                {
+                    wait_handle = wine_server_ptr_handle( reply->wait_handle );
+                }
             }
             SERVER_END_REQ;
             if (status != STATUS_SUCCESS) break;
@@ -1874,10 +1892,12 @@ NTSTATUS WINAPI NtRemoveIoCompletionEx( HANDLE handle, FILE_IO_COMPLETION_INFORM
             if (status == STATUS_PENDING) status = STATUS_SUCCESS;
             break;
         }
-        status = NtWaitForSingleObject( handle, alertable, timeout );
+        status = NtWaitForSingleObject( wait_handle, alertable, timeout );
         if (status != WAIT_OBJECT_0) break;
     }
     *written = i ? i : 1;
+    if (wait_handle != handle)
+        NtClose( wait_handle );
     return status;
 }

diff --git a/server/completion.c b/server/completion.c
index 6933195e72d..6e8a224e3fa 100644
--- a/server/completion.c
+++ b/server/completion.c
@@ -220,7 +220,14 @@ DECL_HANDLER(remove_completion)

entry = list_head( &completion->queue );
     if (!entry)
+    {
+        if (req->alloc_wait_handle)
+            reply->wait_handle = duplicate_handle( current->process,
+                    req->handle, current->process, 0, 0, DUPLICATE_SAME_ACCESS );
+        else
+            reply->wait_handle = req->handle;
         set_error( STATUS_PENDING );
+    }
     else
     {
         list_remove( entry );
diff --git a/server/protocol.def b/server/protocol.def
index ad6d2bb58d0..051a5dbe848 100644
--- a/server/protocol.def
+++ b/server/protocol.def
@@ -3494,11 +3494,13 @@ struct handle_info
 /* get completion from completion port queue */
 @REQ(remove_completion)
     obj_handle_t handle;          /* port handle */
+    unsigned int alloc_wait_handle; /* whether to duplicate handle on PENDING result */
 @REPLY
     apc_param_t   ckey;           /* completion key */
     apc_param_t   cvalue;         /* completion value */
     apc_param_t   information;    /* IO_STATUS_BLOCK Information */
     unsigned int  status;         /* completion result */
+    obj_handle_t  wait_handle;    /* if alloc_wait_handle was non-zero, duplicate handle on PENDING result */
 @END