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@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)