From: Sebastian Lackner sebastian@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@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
-
Gabriel Ivăncescu