[PATCH v2 0/2] MR127: ws2_32: Make wait in select alertable

[Paul Gofman (＠gofman)]

Fixes V Rising hang on exit.

During exit the main thread wants to terminate the other thread by queuing APC which APC is supposed to raise an exception and abort the thread. That is basically how Unity / Mono aborts threads. The thread being terminated is calling select() in a loop and the wait in select() is currently not alertable (while they should be, as the test suggests).

But with the first patch the things may get more interesting. ws2_32.select() passes on stack io status block to NtDeviceIoControlFile(), then it is killed from APC while waiting for event to be signaled by ioctl async processing. Then, wineserver wants to complete the async. The async owner thread is already terminated and it delivers SIGUSR1 to process the system APC to the main thread. When the APC is processed from USR1 handler it segfaults trying to put the IO status to iosb pointing to already deallocated thread stack. This is segfault on signal stack and as such is unrecoverable. The process is left hanging.

Thus the second patch. In the ideal case we'd probably want to avoid crashing or handle segfault in system APC processing somehow, but I don't yet see a feasible way to do it. In theory the same may happen without ws2_32 if the app does the same, but I don't know that anything hits that case while ws2_32 queues those on stack iosbs, so avoiding that may probably avoid the practical issues.

--
  v2: ws2_32: Make wait in WS2_recv_base() alertable.
      ws2_32: Make wait in select() alertable.

https://gitlab.winehq.org/wine/wine/-/merge_requests/127

[PATCH v2 1/2] ws2_32: Make wait in select() alertable.

[Paul Gofman]

From: Paul Gofman

Signed-off-by: Paul Gofman 
---
 dlls/ws2_32/socket.c     | 10 +++++++++-
 dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index 964724b36e7..9be7190062f 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -551,6 +551,14 @@ static HANDLE get_sync_event(void)
     return data->sync_event;
 }

+static DWORD wait_event_alertable( HANDLE event )
+{
+    DWORD ret;
+
+    while ((ret = WaitForSingleObjectEx( event, INFINITE, TRUE )) == WAIT_IO_COMPLETION)
+        ;
+    return ret;
+}

BOOL WINAPI DllMain( HINSTANCE instance, DWORD reason, void *reserved )
 {
@@ -2769,7 +2777,7 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr,
                                     IOCTL_AFD_POLL, params, params_size, params, params_size );
     if (status == STATUS_PENDING)
     {
-        if (WaitForSingleObject( sync_event, INFINITE ) == WAIT_FAILED)
+        if (wait_event_alertable( sync_event ) == WAIT_FAILED)
         {
             free( read_input );
             free( params );
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
index 22e414e02ec..80d458b650d 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -202,6 +202,11 @@ static void tcp_socketpair(SOCKET *src, SOCKET *dst)
     tcp_socketpair_flags(src, dst, WSA_FLAG_OVERLAPPED);
 }

+static void WINAPI apc_func(ULONG_PTR apc_count)
+{
+    ++*(unsigned int *)apc_count;
+}
+
 /* Set the linger timeout to zero and close the socket. This will trigger an
  * RST on the connection on Windows as well as on Unix systems. */
 static void close_with_rst(SOCKET s)
@@ -3613,6 +3618,7 @@ static void test_select(void)
     select_thread_params thread_params;
     HANDLE thread_handle;
     DWORD ticks, id, old_protect;
+    unsigned int apc_count;
     unsigned int maxfd, i;
     char *page_pair;

@@ -3716,14 +3722,24 @@ static void test_select(void)

FD_ZERO(&readfds);
     FD_SET(fdRead, &readfds);
+    apc_count = 0;
+    ret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_count);
+    ok(ret, "QueueUserAPC returned %d\n", ret);
     ret = select(fdRead+1, &readfds, NULL, NULL, &select_timeout);
     ok(!ret, "select returned %d\n", ret);
+    ok(apc_count == 1, "got apc_count %d.\n", apc_count);

FD_ZERO(&writefds);
     FD_SET(fdWrite, &writefds);
+    apc_count = 0;
+    ret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_count);
+    ok(ret, "QueueUserAPC returned %d\n", ret);
     ret = select(fdWrite+1, NULL, &writefds, NULL, &select_timeout);
     ok(ret == 1, "select returned %d\n", ret);
     ok(FD_ISSET(fdWrite, &writefds), "fdWrite socket is not in the set\n");
+    ok(!apc_count, "APC was called\n");
+    SleepEx(0, TRUE);
+    ok(apc_count == 1, "got apc_count %d.\n", apc_count);

/* select the same socket twice */
     writefds.fd_count = 2;

[PATCH v2 1/2] ws2_32: Make wait in select() alertable.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=116718

Your paranoid android.

=== w1064v1507 (64 bit report) ===

ws2_32:
sock.c:7966: Test failed: failed to accept, error 10035
sock.c:7978: Test failed: got 0xc0000023
sock.c:8013: Test failed: got 00000000
sock.c:8027: Test failed: AcceptEx on too small remote address size returned 0 + errno 10022
sock.c:8047: Test failed: AcceptEx returned 0 + errno 10022
sock.c:8067: Test failed: got error 10056
sock: Timeout

[PATCH v2 2/2] ws2_32: Make wait in WS2_recv_base() alertable.

[Paul Gofman]

From: Paul Gofman

Signed-off-by: Paul Gofman 
---
 dlls/ws2_32/socket.c     |  2 +-
 dlls/ws2_32/tests/sock.c | 75 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index 9be7190062f..9cc75aacf20 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -959,7 +959,7 @@ static int WS2_recv_base( SOCKET s, WSABUF *buffers, DWORD buffer_count, DWORD *
                                     IOCTL_AFD_WINE_RECVMSG, &params, sizeof(params), NULL, 0 );
     if (status == STATUS_PENDING && !overlapped)
     {
-        if (WaitForSingleObject( event, INFINITE ) == WAIT_FAILED)
+        if (wait_event_alertable( event ) == WAIT_FAILED)
             return -1;
         status = piosb->u.Status;
     }
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
index 80d458b650d..af4226e6258 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -6747,21 +6747,58 @@ static void test_WSASendTo(void)
     ok(ret_addr.sin_port, "expected nonzero port\n");
 }

+struct recv_thread_apc_param
+{
+    SOCKET sock;
+    unsigned int apc_count;
+};
+
+static void WINAPI recv_thread_apc_func(ULONG_PTR param)
+{
+    struct recv_thread_apc_param *p = (struct recv_thread_apc_param *)param;
+    int ret;
+
+    ++p->apc_count;
+
+    ret = send(p->sock, "test", 4, 0);
+    ok(ret == 4, "got %d.\n", ret);
+}
+
+struct recv_thread_param
+{
+    SOCKET sock;
+    BOOL overlapped;
+};
+
 static DWORD WINAPI recv_thread(LPVOID arg)
 {
-    SOCKET sock = *(SOCKET *)arg;
+    struct recv_thread_param *p = arg;
+    SOCKET sock = p->sock;
     char buffer[32];
     WSABUF wsa;
     WSAOVERLAPPED ov;
     DWORD flags = 0;
+    DWORD len;
+    int ret;

wsa.buf = buffer;
     wsa.len = sizeof(buffer);
-    ov.hEvent = WSACreateEvent();
-    WSARecv(sock, &wsa, 1, NULL, &flags, &ov, NULL);
+    if (p->overlapped)
+    {
+        ov.hEvent = WSACreateEvent();
+        WSARecv(sock, &wsa, 1, NULL, &flags, &ov, NULL);

-    WaitForSingleObject(ov.hEvent, 1000);
-    WSACloseEvent(ov.hEvent);
+        WaitForSingleObject(ov.hEvent, 1000);
+        WSACloseEvent(ov.hEvent);
+    }
+    else
+    {
+        SetLastError(0xdeadbeef);
+        ret = WSARecv(sock, &wsa, 1, &len, &flags, NULL, NULL);
+        ok(!ret, "got ret %d.\n", ret);
+        ok(WSAGetLastError() == 0, "got error %d.\n", WSAGetLastError());
+        ok(len == 4, "got len %lu.\n", len);
+    }
     return 0;
 }

@@ -6775,11 +6812,14 @@ static void WINAPI io_completion(DWORD error, DWORD transferred, WSAOVERLAPPED *
 static void test_WSARecv(void)
 {
     SOCKET src, dest, server = INVALID_SOCKET;
+    struct recv_thread_apc_param apc_param;
+    struct recv_thread_param recv_param;
     char buf[20];
     WSABUF bufs[2];
     WSAOVERLAPPED ov;
     DWORD bytesReturned, flags, id;
     struct sockaddr_in addr;
+    unsigned int apc_count;
     int iret, len;
     DWORD dwret;
     BOOL bret;
@@ -6799,10 +6839,20 @@ static void test_WSARecv(void)
     ok(GetLastError() == ERROR_SUCCESS, "Expected 0, got %ld\n", GetLastError());
     SetLastError(0xdeadbeef);
     bytesReturned = 0xdeadbeef;
+
+    apc_count = 0;
+    dwret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_count);
+    ok(dwret, "QueueUserAPC returned %lu\n", dwret);
+
     iret = WSARecv(dest, bufs, 1, &bytesReturned, &flags, NULL, NULL);
     ok(!iret, "Expected 0, got %d\n", iret);
     ok(bytesReturned == 2, "Expected 2, got %ld\n", bytesReturned);
     ok(GetLastError() == ERROR_SUCCESS, "Expected 0, got %ld\n", GetLastError());
+
+    ok(!apc_count, "got apc_count %u.\n", apc_count);
+    SleepEx(0, TRUE);
+    ok(apc_count == 1, "got apc_count %u.\n", apc_count);
+
     SetLastError(0xdeadbeef);
     bytesReturned = 0xdeadbeef;
     iret = WSARecv(dest, bufs, 1, &bytesReturned, &flags, NULL, NULL);
@@ -6895,10 +6945,23 @@ static void test_WSARecv(void)
     if (dest == INVALID_SOCKET) goto end;

send(src, "test message", sizeof("test message"), 0);
-    thread = CreateThread(NULL, 0, recv_thread, &dest, 0, &id);
+    recv_param.sock = dest;
+    recv_param.overlapped = TRUE;
+    thread = CreateThread(NULL, 0, recv_thread, &recv_param, 0, &id);
     WaitForSingleObject(thread, 3000);
     CloseHandle(thread);

+    recv_param.overlapped = FALSE;
+    thread = CreateThread(NULL, 0, recv_thread, &recv_param, 0, &id);
+    apc_param.apc_count = 0;
+    apc_param.sock = src;
+    dwret = QueueUserAPC(recv_thread_apc_func, thread, (ULONG_PTR)&apc_param);
+    ok(dwret, "QueueUserAPC returned %lu\n", dwret);
+    WaitForSingleObject(thread, 3000);
+    ok(apc_param.apc_count == 1, "got apc_count %u.\n", apc_param.apc_count);
+
+    CloseHandle(thread);
+
     memset(&ov, 0, sizeof(ov));
     ov.hEvent = event;
     ResetEvent(event);

[PATCH v2 0/2] MR127: ws2_32: Make wait in select and recv alertable - approved

[Zebediah Figura (＠zfigura)]

This merge request was approved by Zebediah Figura.