[PATCH 0/2] MR127: ws2_32: Make wait in select alertable
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/127
From: Paul Gofman <pgofman(a)codeweavers.com> --- 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 88089fa8d74..6507f2e3c5b 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 ) { @@ -2601,7 +2609,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 bdb683e6796..36672b22aef 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -3448,6 +3448,11 @@ static void test_listen(void) ok (ret == 0, "closesocket failed unexpectedly: %d\n", ret); } +static void WINAPI apc_func(ULONG_PTR apc_called) +{ + *(BOOL *)apc_called = TRUE; +} + #define FD_ZERO_ALL() { FD_ZERO(&readfds); FD_ZERO(&writefds); FD_ZERO(&exceptfds); } #define FD_SET_ALL(s) { FD_SET(s, &readfds); FD_SET(s, &writefds); FD_SET(s, &exceptfds); } static void test_select(void) @@ -3465,6 +3470,7 @@ static void test_select(void) DWORD ticks, id, old_protect; unsigned int maxfd, i; char *page_pair; + BOOL apc_called; fdRead = socket(AF_INET, SOCK_STREAM, 0); ok( (fdRead != INVALID_SOCKET), "socket failed unexpectedly: %d\n", WSAGetLastError() ); @@ -3566,14 +3572,24 @@ static void test_select(void) FD_ZERO(&readfds); FD_SET(fdRead, &readfds); + apc_called = FALSE; + ret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_called); + 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_called, "APC was not called\n"); FD_ZERO(&writefds); FD_SET(fdWrite, &writefds); + apc_called = FALSE; + ret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_called); + 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_called, "APC was called\n"); + SleepEx(0, TRUE); + ok(apc_called, "APC was not called\n"); /* select the same socket twice */ writefds.fd_count = 2; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/127
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 61 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 6507f2e3c5b..009b4882564 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -157,6 +157,57 @@ DECLARE_CRITICAL_SECTION(cs_socket_list); static SOCKET *socket_list; static unsigned int socket_list_size; +struct io_buf +{ + struct io_buf *next; + IO_STATUS_BLOCK io; +}; +static struct io_buf *io_freelist; + +static IO_STATUS_BLOCK *alloc_io(void) +{ + struct io_buf *io, *ret, *next; + + if (!(io = InterlockedExchangePointer( (void **)&io_freelist, NULL ))) + { + if (!(io = malloc(sizeof(*io)))) + { + ERR( "No memory.\n" ); + return NULL; + } + return &io->io; + } + + ret = io; + next = io->next; + if (next && InterlockedCompareExchangePointer( (void **)&io_freelist, next, NULL )) + { + while ((io = next)) + { + next = io->next; + free( io ); + } + } + return &ret->io; +} + +static void free_io(IO_STATUS_BLOCK *io_data) +{ + struct io_buf *io, *next; + + if (!io_data) return; + + io = CONTAINING_RECORD(io_data, struct io_buf, io); + + while (1) + { + next = io_freelist; + io->next = next; + if (InterlockedCompareExchangePointer( (void **)&io_freelist, io, next ) == next) + return; + } +} + const char *debugstr_sockaddr( const struct sockaddr *a ) { if (!a) return "(nil)"; @@ -2529,7 +2580,7 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr, unsigned int poll_count = 0; ULONG params_size, i, j; SOCKET poll_socket = 0; - IO_STATUS_BLOCK io; + IO_STATUS_BLOCK *io; HANDLE sync_event; int ret_count = 0; NTSTATUS status; @@ -2605,7 +2656,8 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr, assert( params->count == poll_count ); - status = NtDeviceIoControlFile( (HANDLE)poll_socket, sync_event, NULL, NULL, &io, + io = alloc_io(); + status = NtDeviceIoControlFile( (HANDLE)poll_socket, sync_event, NULL, NULL, io, IOCTL_AFD_POLL, params, params_size, params, params_size ); if (status == STATUS_PENDING) { @@ -2613,10 +2665,13 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr, { free( read_input ); free( params ); + free_io( io ); return -1; } - status = io.u.Status; + status = io->u.Status; } + free_io( io ); + if (status == STATUS_TIMEOUT) status = STATUS_SUCCESS; if (!status) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/127
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.
We can wrap the IOSB writes in `set_async_iosb` inside a `__TRY` block from `wine/unixlib.h`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/127#note_1205
"Jinoh Kang (@iamahuman)" <wine(a)gitlab.winehq.org> writes:
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.
We can wrap the IOSB writes in `set_async_iosb` inside a `__TRY` block from `wine/unixlib.h`.
That would only be hiding the bug. -- Alexandre Julliard julliard(a)winehq.org
On 5/25/22 22:30, Alexandre Julliard wrote:
We can wrap the IOSB writes in `set_async_iosb` inside a `__TRY` block from `wine/unixlib.h`.
That would only be hiding the bug.
Handling the exception does not necessarily mean silencing the bug; we can flag the error condition via ERR(). Also, letting the IOSB leak won't make debugging much more pleasurable either IMHO: 1. There's no way to detect pulling the rug out under a pending I/O operation. 2. It causes hard-to-trace memory leak. --- As a side note, Windows does ignore invalid memory accesses when writing IOSB. On Windows, running the test program below results in the following: Test 1. Valid Status, Valid Information iosb->Status = 0 iosb->Information = 0x1 Test 2. Valid Status, Faulting Information iosb->Status = 0xcccccccc iosb->Information = 0xcccccccccccccccc Test 3. Faulting Status, Valid Information iosb->Status = 0xcccccccc iosb->Information = 0x1 Test 4. Faulting Status, Faulting Information iosb->Status = 0xcccccccc iosb->Information = 0xcccccccccccccccc
From the output above, we can infer two things:
1. Windows gives up writing IOSB and let the program continue as usual when it encounters invalid memory accesses. 2. Even if the Status field is invalid, Windows still successfully writes the Information field. --- #define WIN32_LEAN_AND_MEAN #define _UNICODE #include <windows.h> #include <winternl.h> #include <stdio.h> #include <stdlib.h> NTSYSAPI NTSTATUS WINAPI NtReadFile(HANDLE,HANDLE,PIO_APC_ROUTINE,PVOID,PIO_STATUS_BLOCK,PVOID,ULONG,PLARGE_INTEGER,PULONG);; void die_(int line, unsigned int errcode) { fprintf(stderr, "error @ %d: %u (%#x)\n", line, errcode, errcode); exit(EXIT_FAILURE); } #define die() die_(__LINE__, GetLastError()) static void subtest(HANDLE read_pipe, HANDLE write_pipe, IO_STATUS_BLOCK *iosb, void *fault_page); int wmain(void) { HANDLE read_pipe, write_pipe; void *page_0, *page_1, *fault_page; const WCHAR pipe_name[] = L"\\\\.\\PIPE\\LOCAL\\faulty-iosb-test"; IO_STATUS_BLOCK *iosb; read_pipe = CreateNamedPipeW( pipe_name, PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED | FILE_FLAG_FIRST_PIPE_INSTANCE, PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT | PIPE_ACCEPT_REMOTE_CLIENTS, 1, 4096, 4096, 0, NULL ); if (read_pipe == INVALID_HANDLE_VALUE) die(); write_pipe = CreateFile( pipe_name, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, 0, NULL ); if (write_pipe == INVALID_HANDLE_VALUE) die(); page_0 = VirtualAlloc(NULL, 4096 * 2, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); if (!page_0) die(); page_1 = (char *)page_0 + 4096; puts("Test 1. Valid Status, Valid Information"); iosb = (IO_STATUS_BLOCK *)page_0; fault_page = page_1; subtest(read_pipe, write_pipe, iosb, fault_page); puts("Test 2. Valid Status, Faulting Information"); iosb = CONTAINING_RECORD((ULONG_PTR *)page_1, IO_STATUS_BLOCK, Information); fault_page = page_1; subtest(read_pipe, write_pipe, iosb, fault_page); puts("Test 3. Faulting Status, Valid Information"); iosb = CONTAINING_RECORD((ULONG_PTR *)page_1, IO_STATUS_BLOCK, Information); fault_page = page_0; subtest(read_pipe, write_pipe, iosb, fault_page); puts("Test 4. Faulting Status, Faulting Information"); iosb = (IO_STATUS_BLOCK *)page_0; fault_page = page_0; subtest(read_pipe, write_pipe, iosb, fault_page); return 0; } static void subtest(HANDLE read_pipe, HANDLE write_pipe, IO_STATUS_BLOCK *iosb, void *fault_page) { DWORD old_prot; DWORD result; NTSTATUS status; char buffer[1]; memset(iosb, 0xcc, sizeof(*iosb)); status = NtReadFile(read_pipe, NULL, NULL, NULL, iosb, buffer, sizeof(buffer), NULL, NULL); if (status != STATUS_PENDING) die_(__LINE__, status); if (!VirtualProtect(fault_page, 4096, PAGE_NOACCESS, &old_prot)) die(); if (!WriteFile(write_pipe, "", 1, &result, NULL)) die(); if (WaitForSingleObject(read_pipe, INFINITE) != WAIT_OBJECT_0) die(); if (!VirtualProtect(fault_page, 4096, old_prot, &old_prot)) die(); printf("iosb->Status = %#lx\n", (unsigned long)iosb->Status); printf("iosb->Information = %#Ix\n", iosb->Information); puts(""); } -- Sincerely, Jinoh Kang
Jinoh Kang <jinoh.kang.kr(a)gmail.com> writes:
On 5/25/22 22:30, Alexandre Julliard wrote:
We can wrap the IOSB writes in `set_async_iosb` inside a `__TRY` block from `wine/unixlib.h`.
That would only be hiding the bug.
Handling the exception does not necessarily mean silencing the bug; we can flag the error condition via ERR().
That's assuming that we'll always get an exception, but there's no guarantee that the thread stack space has not been allocated to something else, in which case it will silently corrupt data. -- Alexandre Julliard julliard(a)winehq.org
On 5/26/22 00:46, Jinoh Kang wrote:
On 5/25/22 22:30, Alexandre Julliard wrote:
We can wrap the IOSB writes in `set_async_iosb` inside a `__TRY` block from `wine/unixlib.h`.
That would only be hiding the bug.
Handling the exception does not necessarily mean silencing the bug; we can flag the error condition via ERR(). Also, letting the IOSB leak won't make debugging much more pleasurable either IMHO:
1. There's no way to detect pulling the rug out under a pending I/O operation. 2. It causes hard-to-trace memory leak.
On a second thought, we *can* use IO_APC_ROUTINE to detect leaked IOSB on completion (and maybe free it). Maybe it's not worth it, but I think it would still be nice to have some way (not necessarily mine) to detect when the app exhibits such abominable behavior, perhaps too frequently... -- Sincerely, Jinoh Kang
On 5/25/22 04:00, Paul Gofman (@gofman) wrote:
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.
On Windows, asyncs are terminated when owning thread is terminated. If we supported that and made sure that it happens before thread stack is deallocated, it should solve the problem. Jacek
On 5/25/22 10:28, Jacek Caban wrote:
On 5/25/22 04:00, Paul Gofman (@gofman) wrote:
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.
On Windows, asyncs are terminated when owning thread is terminated. If we supported that and made sure that it happens before thread stack is deallocated, it should solve the problem.
Jacek
Do you mean, e. g., if a thread has called an overlapped DeviceIoControl() (or queued other overlapped IO) and exited, the other thread waiting for it to complete, e. g., with GetOverlappedResult() or waiting for event it will never get the completion?
On 5/25/22 17:36, Paul Gofman wrote:
On 5/25/22 10:28, Jacek Caban wrote:
On 5/25/22 04:00, Paul Gofman (@gofman) wrote:
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.
On Windows, asyncs are terminated when owning thread is terminated. If we supported that and made sure that it happens before thread stack is deallocated, it should solve the problem.
Jacek
Do you mean, e. g., if a thread has called an overlapped DeviceIoControl() (or queued other overlapped IO) and exited, the other thread waiting for it to complete, e. g., with GetOverlappedResult() or waiting for event it will never get the completion?
I think that wait should complete. It's been a while since I looked at it, but AFAIR it looked like we should do something like cancel_process_asyncs() for each thread separately. Jacek
On 5/25/22 10:48, Jacek Caban wrote:
On 5/25/22 17:36, Paul Gofman wrote:
On 5/25/22 10:28, Jacek Caban wrote:
On 5/25/22 04:00, Paul Gofman (@gofman) wrote:
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.
On Windows, asyncs are terminated when owning thread is terminated. If we supported that and made sure that it happens before thread stack is deallocated, it should solve the problem.
Jacek
Do you mean, e. g., if a thread has called an overlapped DeviceIoControl() (or queued other overlapped IO) and exited, the other thread waiting for it to complete, e. g., with GetOverlappedResult() or waiting for event it will never get the completion?
I think that wait should complete. It's been a while since I looked at it, but AFAIR it looked like we should do something like cancel_process_asyncs() for each thread separately.
I can test that, but my impression (maybe wrong) was that I can exit the thread which queued the IO and still get the operation successfully completed and the IO status received on another thread after. But imagine that the thread which got an exception in an APC from select() did not exit but just got unwound through exception. In that case processing system APC won't crash but will corrupt the stack by writing the IO status. Should not we better avoid on-stack IO status regardless?
On 5/26/22 00:36, Paul Gofman wrote:
On 5/25/22 10:28, Jacek Caban wrote:
On 5/25/22 04:00, Paul Gofman (@gofman) wrote:
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.
On Windows, asyncs are terminated when owning thread is terminated. If we supported that and made sure that it happens before thread stack is deallocated, it should solve the problem.
Jacek
Do you mean, e. g., if a thread has called an overlapped DeviceIoControl() (or queued other overlapped IO) and exited, the other thread waiting for it to complete, e. g., with GetOverlappedResult() or waiting for event it will never get the completion?
Or, perhaps Windows cancels I/O operations (on thread termination) and doesn't let the thread stack go away until all I/O operations are completed?
-- Sincerely, Jinoh Kang
participants (7)
-
Alexandre Julliard -
Jacek Caban -
Jinoh Kang -
Jinoh Kang (＠iamahuman) -
Paul Gofman -
Paul Gofman
-
Paul Gofman (＠gofman)