From e1966ac26ea98127005fc8c70c962a7a8232b9d8 Mon Sep 17 00:00:00 2001 From: Zebediah Figura Date: Fri, 22 Mar 2024 00:12:25 -0500 Subject: [PATCH] ntdll-Threading: Remove patch. This has sat here for a long time pending careful review, because the logic is not easy to follow. Fortunately I think I understand it now. The described race is pretty much accurate. When a thread called RtlExitUserThread() in 1.7.38, it first decremented nb_threads. If it was the last thread, it would call exit() with the thread exit status; if not, it would mask off signals [the order here is important] and then call pthread_exit() with the status. When a thread called RtlExitUserProcess(), this happened: * The caller does a terminate_process() request to the server, which sends SIGQUIT to every thread *but* the caller. * The SIGQUIT handler calls terminate_thread() with a zero status. terminate_thread() masks off signals, then decrements nb_threads. If the aborting thread is the last thread, it would call _exit(), otherwise, it'd again just pthread_exit(). * Finally, the original thread would call exit(), with the intended status code. [All of the intermediate function calls and helpers are skipped for brevity and clarity]. The problem happens if both of these happen at the same time in different threads. In this case the RtlExitUserThread() thread could decrement nb_threads, then get interrupted by SIGQUIT and decrement nb_threads again. The end result is that, instead of the RtlExitUserProcess() thread exiting with the intended status, instead one of the SIGQUIT threads will be the "last" thread, and exit with the status that SIGQUIT uses, which is zero as described. A more serious race than this can be constructed if a thread is terminated by another thread while already exiting. In this case nb_threads would be executed twice, but the consequence would be that the *penultimate* thread to exit, later, would end up killing the process, since it thinks it's the ultimate thread. 2334f4e64582a518e4d5a7627472a0d817b147ef changed this. Now a thread calling RtlExitUserThread() does not decrement nb_threads, but instead asks the server if it's the last thread, and if so exit the whole process [at the time via exit(); later via RtlExitUserProcess().] If not the last thread, the threads mask off signals and then call pthread_exit() as before. This avoided the race, but added a different one, essentially the opposite problem: if two threads exit cleanly at the same time, neither one of them will think they're the last thread, then both will exit without calling exit(). Apparently (from IRC logs) this would leave the thread in a weird state where it'd still be running somehow, although it's not really clear how. In any case, this problem was fixed by fac1aabbef3753afc53a4ea4f933b3d0516fd302 upstream. Now if two threads call NtTerminateThread() on themselves at the same time, they really will exit cleanly and one will terminate the process. Critically, this is now safe from the original race, because decrementing nb_threads is done after masking off signals. --- ...ondition-when-threads-are-killed-dur.patch | 62 ------------------- patches/ntdll-Threading/definition | 3 - patches/server-Shared_Memory/definition | 1 - 3 files changed, 66 deletions(-) delete mode 100644 patches/ntdll-Threading/0001-ntdll-Fix-race-condition-when-threads-are-killed-dur.patch delete mode 100644 patches/ntdll-Threading/definition diff --git a/patches/ntdll-Threading/0001-ntdll-Fix-race-condition-when-threads-are-killed-dur.patch b/patches/ntdll-Threading/0001-ntdll-Fix-race-condition-when-threads-are-killed-dur.patch deleted file mode 100644 index e331c3dc..00000000 --- a/patches/ntdll-Threading/0001-ntdll-Fix-race-condition-when-threads-are-killed-dur.patch +++ /dev/null @@ -1,62 +0,0 @@ -From 9da818bd948256572640e17766a14a72e58ce100 Mon Sep 17 00:00:00 2001 -From: Sebastian Lackner -Date: Wed, 25 Feb 2015 22:45:42 +0100 -Subject: [PATCH] ntdll: Fix race-condition when threads are killed during - shutdown. - -When exit_thread is executed, nb_threads is decremented before the thread is -fully shutdown. When another thread runs ExitProcess() this will cause a SIGQUIT -signal to all threads, effectively decrementing nb_threads twice. The process -will terminate with a wrong exitcode then because the refcount reaches zero too -early. - -Currently Wine has no locking protection of LdrShutdownProcess(), so it can -only be executed safely when all other threads have terminated before. Most -likely there are more Wine bugs in this area, but the attached patch should -fix the most critical one (messed up refcounting of threads) for now. ---- - dlls/ntdll/thread.c | 2 +- - dlls/ntdll/unix/thread.c | 7 +++++++ - 2 files changed, 8 insertions(+), 1 deletion(-) - -diff --git a/dlls/ntdll/thread.c b/dlls/ntdll/thread.c -index d5e34cae3b1..83237b3569a 100644 ---- a/dlls/ntdll/thread.c -+++ b/dlls/ntdll/thread.c -@@ -295,7 +295,7 @@ void WINAPI RtlExitUserThread( ULONG status ) - SERVER_END_REQ; - } - -- if (InterlockedDecrement( &nb_threads ) <= 0) -+ if (InterlockedCompareExchange( &nb_threads, 0, 0 ) <= 0) - { - LdrShutdownProcess(); - unix_funcs->exit_process( status ); -diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c -index 205a1312e92..563712bd59e 100644 ---- a/dlls/ntdll/unix/thread.c -+++ b/dlls/ntdll/unix/thread.c -@@ -219,6 +219,7 @@ void CDECL abort_thread( int status ) - void CDECL exit_thread( int status ) - { - static void *prev_teb; -+ sigset_t sigset; - TEB *teb; - - pthread_sigmask( SIG_BLOCK, &server_block_set, NULL ); -@@ -233,6 +234,12 @@ void CDECL exit_thread( int status ) - virtual_free_teb( teb ); - } - } -+ -+ sigemptyset( &sigset ); -+ sigaddset( &sigset, SIGQUIT ); -+ pthread_sigmask( SIG_BLOCK, &sigset, NULL ); -+ if (!InterlockedDecrement( nb_threads )) _exit( status ); -+ - signal_exit_thread( status, pthread_exit_wrapper ); - } - --- -2.26.2 - diff --git a/patches/ntdll-Threading/definition b/patches/ntdll-Threading/definition deleted file mode 100644 index 7e597913..00000000 --- a/patches/ntdll-Threading/definition +++ /dev/null @@ -1,3 +0,0 @@ -Fixes: Fix race-condition when threads are killed during shutdown -# Needs careful review to determine if this is still needed. Deferring. -Disabled: true diff --git a/patches/server-Shared_Memory/definition b/patches/server-Shared_Memory/definition index 593dea94..54f7a6e4 100644 --- a/patches/server-Shared_Memory/definition +++ b/patches/server-Shared_Memory/definition @@ -1,7 +1,6 @@ # Bugs 37419 and 29582 track applications which want a faster # GetForegroundWindow(). Unfortunately, that patch is disabled, and I can't # find record of applications which want the other functions here to be faster. -Depends: ntdll-Threading Depends: server-PeekMessage Depends: server-Signal_Thread Depends: ntdll-ext4-case-folder