[PATCH] server: Do not signal violently terminated threads until they are really gone

[Gabriel Ivăncescu]

From: Sebastian Lackner <sebastian at fds-team.de>

When a thread is terminated violently (such as by using TerminateThread)
that is not the current thread, the server sends a signal to the thread to
terminate it, but it immediately wakes up anything waiting on it. The caller
can expect WaitForSingleObject (or similar) to return when the thread is
really gone and doesn't execute anything anymore, and this is exactly what
happens on Windows.

If that thread was altering global state, and the thread that was waiting
on it will read (or alter) the global state *after* waiting for it and
expecting it to not change (because it assumes the thread is terminated by
that point, as on Windows), the result will be a race condition, since there's
no guarantee currently that the terminated thread really stopped executing.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
---

Trying to upstream a wine-staging patch properly.

This affects MSYS2 apps and Cygwin under Wine. The known side-effects of
this bug have been fixed with this iteration on it, which only waits if
the thread was terminated violently (otherwise, it doesn't matter) so it
removes the impact it has on most apps.

In other words, I can confirm that
https://bugs.winehq.org/show_bug.cgi?id=41107 is no longer a side-effect
issue with this version of the patch. So that bug can be also marked fixed
(though it affects only wine-staging).

Note that I tried different methods to get rid of the polling because
it's ugly, but none worked. POSIX or Linux really don't seem to offer any
functionality to wait on an arbitrary (non-child) thread or process, which
really sucks.

As for test-cases: I'm really not sure how to test race conditions
reliably. I did a bunch of simple tests but all passed even without the
patch on my machine, so it's quite difficult to have reliable failures
(maybe sending SIGQUIT was much faster than the round trip back from the
wineserver, by happenstance). However, it's clear that no wait is being
done so just relying on luck like currently is not ideal, either.

 server/thread.c | 32 +++++++++++++++++++++++++++++---
 server/thread.h |  1 +
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/server/thread.c b/server/thread.c
index 7162fc3..372882f 100644
--- a/server/thread.c
+++ b/server/thread.c
@@ -199,6 +199,7 @@ static inline void init_thread_structure( struct thread *thread )
     thread->suspend         = 0;
     thread->desktop_users   = 0;
     thread->token           = NULL;
+    thread->exit_poll       = NULL;
 
     thread->creation_time = current_time;
     thread->exit_time     = 0;
@@ -347,6 +348,7 @@ static void destroy_thread( struct object *obj )
     list_remove( &thread->entry );
     cleanup_thread( thread );
     release_object( thread->process );
+    if (thread->exit_poll) remove_timeout_user( thread->exit_poll );
     if (thread->id) free_ptid( thread->id );
     if (thread->token) release_object( thread->token );
 }
@@ -364,7 +366,7 @@ static void dump_thread( struct object *obj, int verbose )
 static int thread_signaled( struct object *obj, struct wait_queue_entry *entry )
 {
     struct thread *mythread = (struct thread *)obj;
-    return (mythread->state == TERMINATED);
+    return mythread->state == TERMINATED && !mythread->exit_poll;
 }
 
 static unsigned int thread_map_access( struct object *obj, unsigned int access )
@@ -1125,6 +1127,26 @@ int thread_get_inflight_fd( struct thread *thread, int client )
     return -1;
 }
 
+static void check_terminated( void *arg )
+{
+    struct thread *thread = arg;
+    assert( thread->obj.ops == &thread_ops );
+    assert( thread->state == TERMINATED );
+
+    /* don't wake up until the thread is really dead, to avoid race conditions */
+    if (thread->unix_tid != -1 && !kill( thread->unix_tid, 0 ))
+    {
+        thread->exit_poll = add_timeout_user( -TICKS_PER_SEC / 1000, check_terminated, thread );
+        return;
+    }
+
+    /* grab reference since object can be destroyed while trying to wake up */
+    grab_object( &thread->obj );
+    thread->exit_poll = NULL;
+    wake_up( &thread->obj, 0 );
+    release_object( &thread->obj );
+}
+
 /* kill a thread on the spot */
 void kill_thread( struct thread *thread, int violent_death )
 {
@@ -1145,8 +1167,12 @@ void kill_thread( struct thread *thread, int violent_death )
     kill_console_processes( thread, 0 );
     debug_exit_thread( thread );
     abandon_mutexes( thread );
-    wake_up( &thread->obj, 0 );
-    if (violent_death) send_thread_signal( thread, SIGQUIT );
+    if (violent_death)
+    {
+        send_thread_signal( thread, SIGQUIT );
+        check_terminated( thread );
+    }
+    else wake_up( &thread->obj, 0 );
     cleanup_thread( thread );
     remove_process_thread( thread->process, thread );
     release_object( thread );
diff --git a/server/thread.h b/server/thread.h
index e4332df..1b01c68 100644
--- a/server/thread.h
+++ b/server/thread.h
@@ -89,6 +89,7 @@ struct thread
     timeout_t              creation_time; /* Thread creation time */
     timeout_t              exit_time;     /* Thread exit time */
     struct token          *token;         /* security token associated with this thread */
+    struct timeout_user   *exit_poll;     /* poll if the thread/process has exited already */
 };
 
 struct thread_snapshot
-- 
2.19.1