[PATCH 0/2] MR4811: ntdll: Avoid hanging when a thread is terminated while holding a pthread mutex.

[PATCH 1/2] ntdll: Add SIGQUIT to server_block_set.

[Elizabeth Figura]

From: Elizabeth Figura

Keil UV4 C51 starts a thread and then immediately terminates it, usually while
it's in the middle of an NtClose(). This results in the thread dying while
holding the fd_cache_handle mutex, which results in a hang from other threads in
the process.

We don't need to process SIGQUIT immediately, so just mask it off until we're
out of the uninterrupted section.

This will require the next commit to fix the bug.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54807
---
 dlls/ntdll/unix/server.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c
index 53fcc61ccf3..057efb5e917 100644
--- a/dlls/ntdll/unix/server.c
+++ b/dlls/ntdll/unix/server.c
@@ -1579,6 +1579,7 @@ size_t server_init_process(void)
     sigaddset( &server_block_set, SIGIO );
     sigaddset( &server_block_set, SIGINT );
     sigaddset( &server_block_set, SIGHUP );
+    sigaddset( &server_block_set, SIGQUIT );
     sigaddset( &server_block_set, SIGUSR1 );
     sigaddset( &server_block_set, SIGUSR2 );
     sigaddset( &server_block_set, SIGCHLD );

[PATCH 2/2] ntdll: Do not abort the thread if the server pipe is broken.

[Elizabeth Figura]

From: Elizabeth Figura

Keil UV4 C51 starts a thread and then immediately terminates it, usually while
it's in the middle of an NtClose().

The previous commit prevents the SIGQUIT from killing the thread before it exits
the fd_cache_section mutex. However, the server will still have closed its side
of the socket, resulting in the close_handle server request receiving EPIPE from
write(), or zero from read(), at which point the client calls abort_thread()
anyway, while still holding the mutex.

As far as I can tell, the point of aborting here is to avoid letting a client
process remain forever if the server dies. This is a sensible goal; however, it
cannot always work in theory (a thread which never makes a server call will
never reach this point and will remain anyway) and, speaking from experience, it
almost never works in practice (every time the server dies I have to manually
kill most processes).

In summary, if the server intentionally closes a client thread's socket fd, we
can rely on the accompanying SIGQUIT to kill the client thread, and if the
server dies unexpectedly, the client was most likely going to hang anyway. Since
the abort is not really helping, and is now causing problems, just remove it.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54807
---
 dlls/ntdll/unix/server.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c
index 057efb5e917..4de2a794127 100644
--- a/dlls/ntdll/unix/server.c
+++ b/dlls/ntdll/unix/server.c
@@ -206,7 +206,7 @@ static unsigned int send_request( const struct __server_request_info *req )
     }

if (ret >= 0) server_protocol_error( "partial write %d\n", ret );
-    if (errno == EPIPE) abort_thread(0);
+    if (errno == EPIPE) return STATUS_THREAD_IS_TERMINATING;
     if (errno == EFAULT) return STATUS_ACCESS_VIOLATION;
     server_protocol_perror( "write" );
 }
@@ -217,7 +217,7 @@ static unsigned int send_request( const struct __server_request_info *req )
  *
  * Read data from the reply buffer; helper for wait_reply.
  */
-static void read_reply_data( void *buffer, size_t size )
+static unsigned int read_reply_data( void *buffer, size_t size )
 {
     int ret;

@@ -225,7 +225,7 @@ static void read_reply_data( void *buffer, size_t size )
     {
         if ((ret = read( ntdll_get_thread_data()->reply_fd, buffer, size )) > 0)
         {
-            if (!(size -= ret)) return;
+            if (!(size -= ret)) return STATUS_SUCCESS;
             buffer = (char *)buffer + ret;
             continue;
         }
@@ -234,8 +234,7 @@ static void read_reply_data( void *buffer, size_t size )
         if (errno == EPIPE) break;
         server_protocol_perror("read");
     }
-    /* the server closed the connection; time to die... */
-    abort_thread(0);
+    return STATUS_THREAD_IS_TERMINATING;
 }

[Zebediah Figura (＠zfigura)]

The attached test reliably reproduces the deadlock without these patches.

This may not be the preferred way to avoid this deadlock, and I may be missing some context as to why the code is structured like it is...

[deadlock-test.diff](/uploads/91898ab89d5eca8fb18709112a0f751c/deadlock-test.diff)