[PATCH] ntdll and wineserver: Make QueueUserAPC more conformant.

[David Koolhoven]

Windows developers expect calls made on a terminating thread
to result in ERROR_GEN_FAILURE like a disconnected device,
not ERROR_ACCESS_DENIED.
Change to code added here:
<https://source.winehq.org/git/wine.git/commitdiff/442f5f56d1c3367c9d166ef340a682a390bd57cb>

QueueUserAPC is expected to create a thread specific FIFO
queue. Adding to the end of an unspecified queue on a
different thread removes multithreading optimization
attempts.
Change to code added here:
Thread swap on queueAPC call:
<https://source.winehq.org/git/wine.git/commitdiff/6ca1d1b0812496e670957543294b04abfefe8d78>

Removed a set_error call on test to queue_apc to allow for
other error reasons besides THREAD_IS_TERMINATING to make it
out, and added more informative errror codes inside thread
handlers.

This solves problems seen in earlier WebRTC:
<https://webrtc.googlesource.com/src/+/dbfb58b850c14eeadac5ac66689fb855c79abf36>

This may also improve queue and thread based optimizations,
and prevent race conditions. I wasn't able to find a
Wine-Bug open for this.

Signed-off-by: David Koolhoven <david(a)koolhoven-home.net>
---
 dlls/ntdll/error.c     |  2 +-
 dlls/ntdll/make_errors |  2 +-
 server/thread.c        | 61 +++++++++++++++++-------------------------
 3 files changed, 27 insertions(+), 38 deletions(-)

diff --git a/dlls/ntdll/error.c b/dlls/ntdll/error.c
index fb630fa9864..3b6b2ecdebe 100644
--- a/dlls/ntdll/error.c
+++ b/dlls/ntdll/error.c
@@ -475,7 +475,7 @@ static const DWORD error_map[1579] =
     ERROR_INVALID_PARAMETER,                           /* c0000048 (STATUS_PORT_ALREADY_SET) */
     ERROR_INVALID_PARAMETER,                           /* c0000049 (STATUS_SECTION_NOT_IMAGE) */
     ERROR_SIGNAL_REFUSED,                              /* c000004a (STATUS_SUSPEND_COUNT_EXCEEDED) */
-    ERROR_ACCESS_DENIED,                               /* c000004b (STATUS_THREAD_IS_TERMINATING) */
+    ERROR_GEN_FAILURE,                                 /* c000004b (STATUS_THREAD_IS_TERMINATING) */
     ERROR_INVALID_PARAMETER,                           /* c000004c (STATUS_BAD_WORKING_SET_LIMIT) */
     ERROR_INVALID_PARAMETER,                           /* c000004d (STATUS_INCOMPATIBLE_FILE_MAP) */
     ERROR_INVALID_PARAMETER,                           /* c000004e (STATUS_SECTION_PROTECTION) */
diff --git a/dlls/ntdll/make_errors b/dlls/ntdll/make_errors
index bedce57fe7a..b7a2a346e5c 100755
--- a/dlls/ntdll/make_errors
+++ b/dlls/ntdll/make_errors
@@ -307,7 +307,7 @@ my %error_map = qw(
     STATUS_PORT_ALREADY_SET                             ERROR_INVALID_PARAMETER
     STATUS_SECTION_NOT_IMAGE                            ERROR_INVALID_PARAMETER
     STATUS_SUSPEND_COUNT_EXCEEDED                       ERROR_SIGNAL_REFUSED
-    STATUS_THREAD_IS_TERMINATING                        ERROR_ACCESS_DENIED
+    STATUS_THREAD_IS_TERMINATING                        ERROR_GEN_FAILURE
     STATUS_BAD_WORKING_SET_LIMIT                        ERROR_INVALID_PARAMETER
     STATUS_INCOMPATIBLE_FILE_MAP                        ERROR_INVALID_PARAMETER
     STATUS_SECTION_PROTECTION                           ERROR_INVALID_PARAMETER
diff --git a/server/thread.c b/server/thread.c
index 942a8ff8389..9e747271e61 100644
--- a/server/thread.c
+++ b/server/thread.c
@@ -388,7 +388,8 @@ static void cleanup_thread( struct thread *thread )
 
     if (thread->context)
     {
-        thread->context->status = STATUS_ACCESS_DENIED;
+        thread->context->status = STATUS_THREAD_IS_TERMINATING;
+        set_error( STATUS_THREAD_IS_TERMINATING );
         wake_up( &thread->context->obj, 0 );
         release_object( thread->context );
         thread->context = NULL;
@@ -427,6 +428,7 @@ static void cleanup_thread( struct thread *thread )
 static void destroy_thread( struct object *obj )
 {
     struct thread *thread = (struct thread *)obj;
+    set_error( STATUS_THREAD_IS_TERMINATING ); /* Maybe remove */
     assert( obj->ops == &thread_ops );
 
     assert( !thread->debug_ctx );  /* cannot still be debugging something */
@@ -518,7 +520,7 @@ struct thread *get_thread_from_id( thread_id_t id )
     struct object *obj = get_ptid_entry( id );
 
     if (obj && obj->ops == &thread_ops) return (struct thread *)grab_object( obj );
-    set_error( STATUS_INVALID_CID );
+    set_error( STATUS_NO_LDT );
     return NULL;
 }
 
@@ -538,6 +540,7 @@ struct thread *get_thread_from_tid( int tid )
     {
         if (thread->unix_tid == tid) return thread;
     }
+    set_error( STATUS_NO_LDT );
     return NULL;
 }
 
@@ -1077,41 +1080,23 @@ static int queue_apc( struct process *process, struct thread *thread, struct thr
 {
     struct list *queue;
 
-    if (thread && thread->state == TERMINATED && process)
-        thread = NULL;
-
-    if (!thread)  /* find a suitable thread inside the process */
+    if (thread && thread->state == TERMINATED)
     {
-        struct thread *candidate;
-
-        /* first try to find a waiting thread */
-        LIST_FOR_EACH_ENTRY( candidate, &process->thread_list, struct thread, proc_entry )
-        {
-            if (candidate->state == TERMINATED) continue;
-            if (is_in_apc_wait( candidate ))
-            {
-                thread = candidate;
-                break;
-            }
-        }
-        if (!thread)
-        {
-            /* then use the first one that accepts a signal */
-            LIST_FOR_EACH_ENTRY( candidate, &process->thread_list, struct thread, proc_entry )
-            {
-                if (send_thread_signal( candidate, SIGUSR1 ))
-                {
-                    thread = candidate;
-                    break;
-                }
-            }
-        }
-        if (!thread) return 0;  /* nothing found */
-        queue = get_apc_queue( thread, apc->call.type );
+        set_error( STATUS_THREAD_IS_TERMINATING );
+        return 0;
+    }
+    if (!thread)
+    {
+        set_error( STATUS_NO_LDT );
+        return 0;
     }
     else
     {
-        if (thread->state == TERMINATED) return 0;
+        if (thread->state == TERMINATED)
+        {
+            set_error( STATUS_THREAD_IS_TERMINATING );
+            return 0;
+        }
         queue = get_apc_queue( thread, apc->call.type );
         /* send signal for system APCs if needed */
         if (queue == &thread->system_apc && list_empty( queue ) && !is_in_apc_wait( thread ))
@@ -1251,7 +1236,11 @@ int thread_get_inflight_fd( struct thread *thread, int client )
 /* kill a thread on the spot */
 void kill_thread( struct thread *thread, int violent_death )
 {
-    if (thread->state == TERMINATED) return;  /* already killed */
+    if (thread->state == TERMINATED)
+    {
+        set_error( STATUS_THREAD_IS_TERMINATING );
+        return;  /* already killed */
+    }
     thread->state = TERMINATED;
     thread->exit_time = current_time;
     if (current == thread) current = NULL;
@@ -1563,7 +1552,7 @@ DECL_HANDLER(suspend_thread)
 
     if ((thread = get_thread_from_handle( req->handle, THREAD_SUSPEND_RESUME )))
     {
-        if (thread->state == TERMINATED) set_error( STATUS_ACCESS_DENIED );
+        if (thread->state == TERMINATED) set_error( STATUS_UNSUCCESSFUL );
         else reply->count = suspend_thread( thread );
         release_object( thread );
     }
@@ -1750,7 +1739,7 @@ DECL_HANDLER(queue_apc)
 
     if (thread)
     {
-        if (!queue_apc( NULL, thread, apc )) set_error( STATUS_THREAD_IS_TERMINATING );
+        queue_apc( NULL, thread, apc );
         release_object( thread );
     }
     else if (process)
-- 
2.19.1

[Alexandre Julliard]

David Koolhoven <david(a)koolhoven-home.net> writes:
> Windows developers expect calls made on a terminating thread
> to result in ERROR_GEN_FAILURE like a disconnected device,
> not ERROR_ACCESS_DENIED.
> Change to code added here:
> <https://source.winehq.org/git/wine.git/commitdiff/442f5f56d1c3367c9d166ef340a682a390bd57cb>
>
> QueueUserAPC is expected to create a thread specific FIFO
> queue. Adding to the end of an unspecified queue on a
> different thread removes multithreading optimization
> attempts.
> Change to code added here:
> Thread swap on queueAPC call:
> <https://source.winehq.org/git/wine.git/commitdiff/6ca1d1b0812496e670957543294b04abfefe8d78>
>
> Removed a set_error call on test to queue_apc to allow for
> other error reasons besides THREAD_IS_TERMINATING to make it
> out, and added more informative errror codes inside thread
> handlers.

Please send separate fixes as separate patches, and try to add test
cases.

-- 
Alexandre Julliard
julliard(a)winehq.org