From ed9fbc71ad357fe633abb5da5aba1ffddcd09a33 Mon Sep 17 00:00:00 2001
From: Sebastian Lackner <sebastian@fds-team.de>
Date: Wed, 4 Nov 2015 07:30:36 +0100
Subject: ntdll: Block signals while executing system APCs. (v2)

Changes in v2:
  * Do not delete the still valid comment "Return TRUE if a user APC has been run.".
    I still had this deleted from an earlier version of the patchset, where the
    return type was changed.

If a USR1 suspend signal arrives between dequeing a system APC and sending
back the result to the wineserver, a deadlock occurs. To avoid this issue
this patch blocks all signals in server_select(), except while waiting or
processing user APCs.

In server_queue_process_apc() we do not need to block signals because no
other process will wait on the result.

Signed-off-by: Sebastian Lackner <sebastian@fds-team.de>
---
 dlls/kernel32/tests/sync.c | 11 ++---------
 dlls/ntdll/ntdll_misc.h    |  2 +-
 dlls/ntdll/server.c        | 21 +++++++++++++++++----
 dlls/ntdll/sync.c          |  2 +-
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c
index f7be51e972..982a6ae105 100644
--- a/dlls/kernel32/tests/sync.c
+++ b/dlls/kernel32/tests/sync.c
@@ -2604,21 +2604,14 @@ static void test_apc_deadlock(void)
     result = WaitForSingleObject(event, 1000);
     ok(result == WAIT_OBJECT_0, "expected WAIT_OBJECT_0, got %u\n", result);
 
-    for (i = 0; i < 1000 && info.running; i++)
+    for (i = 0; i < 1000; i++)
     {
         result = SuspendThread(pi.hThread);
         ok(result == 0, "expected 0, got %u\n", result);
 
         WaitForSingleObject(event, 0); /* reset event */
         result = WaitForSingleObject(event, 1000);
-        if (result == WAIT_TIMEOUT)
-        {
-            todo_wine
-            ok(result == WAIT_OBJECT_0, "expected WAIT_OBJECT_0, got %u\n", result);
-            info.running = FALSE;
-        }
-        else
-            ok(result == WAIT_OBJECT_0, "expected WAIT_OBJECT_0, got %u\n", result);
+        ok(result == WAIT_OBJECT_0, "expected WAIT_OBJECT_0, got %u\n", result);
 
         result = ResumeThread(pi.hThread);
         ok(result == 1, "expected 1, got %u\n", result);
diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h
index 1630978f0a..3c7411e6d6 100644
--- a/dlls/ntdll/ntdll_misc.h
+++ b/dlls/ntdll/ntdll_misc.h
@@ -105,7 +105,7 @@ extern NTSTATUS alloc_object_attributes( const OBJECT_ATTRIBUTES *attr, struct o
                                          data_size_t *ret_len ) DECLSPEC_HIDDEN;
 extern NTSTATUS validate_open_object_attributes( const OBJECT_ATTRIBUTES *attr ) DECLSPEC_HIDDEN;
 extern int wait_select_reply( void *cookie ) DECLSPEC_HIDDEN;
-extern BOOL invoke_apc( const apc_call_t *call, apc_result_t *result ) DECLSPEC_HIDDEN;
+extern BOOL invoke_apc( const apc_call_t *call, apc_result_t *result, sigset_t *user_sigset ) DECLSPEC_HIDDEN;
 
 /* module handling */
 extern LIST_ENTRY tls_links DECLSPEC_HIDDEN;
diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c
index 38356f4674..8944508f85 100644
--- a/dlls/ntdll/server.c
+++ b/dlls/ntdll/server.c
@@ -385,8 +385,9 @@ int wait_select_reply( void *cookie )
  *              invoke_apc
  *
  * Invoke a single APC. Return TRUE if a user APC has been run.
+ * Optionally unblock signals while executing user APCs.
  */
-BOOL invoke_apc( const apc_call_t *call, apc_result_t *result )
+BOOL invoke_apc( const apc_call_t *call, apc_result_t *result, sigset_t *user_sigset )
 {
     BOOL user_apc = FALSE;
     SIZE_T size;
@@ -401,15 +402,19 @@ BOOL invoke_apc( const apc_call_t *call, apc_result_t *result )
     case APC_USER:
     {
         void (WINAPI *func)(ULONG_PTR,ULONG_PTR,ULONG_PTR) = wine_server_get_ptr( call->user.func );
+        if (user_sigset) pthread_sigmask( SIG_SETMASK, user_sigset, NULL );
         func( call->user.args[0], call->user.args[1], call->user.args[2] );
+        if (user_sigset) pthread_sigmask( SIG_BLOCK, &server_block_set, user_sigset );
         user_apc = TRUE;
         break;
     }
     case APC_TIMER:
     {
         void (WINAPI *func)(void*, unsigned int, unsigned int) = wine_server_get_ptr( call->timer.func );
+        if (user_sigset) pthread_sigmask( SIG_SETMASK, user_sigset, NULL );
         func( wine_server_get_ptr( call->timer.arg ),
               (DWORD)call->timer.time, (DWORD)(call->timer.time >> 32) );
+        if (user_sigset) pthread_sigmask( SIG_BLOCK, &server_block_set, user_sigset );
         user_apc = TRUE;
         break;
     }
@@ -595,10 +600,12 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT
     obj_handle_t apc_handle = 0;
     apc_call_t call;
     apc_result_t result;
+    sigset_t old_set;
     timeout_t abs_timeout = timeout ? timeout->QuadPart : TIMEOUT_INFINITE;
 
     memset( &result, 0, sizeof(result) );
 
+    pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set );
     for (;;)
     {
         SERVER_START_REQ( select )
@@ -615,9 +622,14 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT
             call        = reply->call;
         }
         SERVER_END_REQ;
-        if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie );
+        if (ret == STATUS_PENDING)
+        {
+            pthread_sigmask( SIG_SETMASK, &old_set, NULL );
+            ret = wait_select_reply( &cookie );
+            pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set );
+        }
         if (ret != STATUS_USER_APC) break;
-        if (invoke_apc( &call, &result ))
+        if (invoke_apc( &call, &result, &old_set ))
         {
             /* if we ran a user apc we have to check once more if additional apcs are queued,
              * but we don't want to wait */
@@ -630,6 +642,7 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT
         if (size >= sizeof(select_op->signal_and_wait) && select_op->op == SELECT_SIGNAL_AND_WAIT)
             size = offsetof( select_op_t, signal_and_wait.signal );
     }
+    pthread_sigmask( SIG_SETMASK, &old_set, NULL );
 
     if (ret == STATUS_TIMEOUT && user_apc) ret = STATUS_USER_APC;
 
@@ -668,7 +681,7 @@ unsigned int server_queue_process_apc( HANDLE process, const apc_call_t *call, a
 
         if (self)
         {
-            invoke_apc( call, result );
+            invoke_apc( call, result, NULL );
         }
         else
         {
diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c
index 62cb3cd5ec..a304c9dee4 100644
--- a/dlls/ntdll/sync.c
+++ b/dlls/ntdll/sync.c
@@ -2039,7 +2039,7 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size
 
         if (ret == STATUS_PENDING) ret = wait_select_reply( &cookie );
         if (ret != STATUS_USER_APC) break;
-        if (invoke_apc( &call, &result ))
+        if (invoke_apc( &call, &result, NULL ))
         {
             /* if we ran a user apc we have to check once more if additional apcs are queued,
              * but we don't want to wait */
-- 
2.14.1

