[PATCH 0/4] MR3329: winsock: Better cancel asyncs when socket handle is closed.
It turns out that Windows have a special behaviour WRT cancelling asyncs for terminating thread (or already terminated thread when socket handle is closed) when there is no more open handles in the process for the socket (while there might be handles to it in another processes). We currently have a logic on cancelling async in server/async.c:cancel_terminating_thread_asyncs() called before thread termination. It has a special case ("async->completion && async->data.apc_context && !async->event") when async is not canceled (covered by the tests). It turns out that on Windows, if there are no more handles to the socket in the async's process, async is cancelled even in this case. Then, if thread is terminated while there are open handles in the same process, but those handles are closed after that, it turns out the asyncs should be canceled the same way, even if there are handles for the socket in another process. Why this is important (and, actually, makes some sense to me), is that it is very easy to duplicate all the open handles to the child process. Just e. g., Qt doesn't seem to have any way to create a process through QtProcess without inheriting handles. Programs might expect that once they close socket handle the async operations will be aborted and they receive completions on port. But that doesn't happen if they didn't use an event for overlapped operation and happened to create any child process with inherited handles after creating the socket. Windows behaviour looks to me as a workaround for this. Besides the present test, I also tested the same with AcceptEx and IOCTL_AFD_RECV which behaves the same. Just to note, closing the handle before thread termination when there are no other open handles is quite a different case. In its heart it behaves *somewhat* similar on Windows and Wine but not quite, hitting various issues. But that looks unrelated to the special case concerned in this patchset. Fixes Battle.net hanging on exit during "update and restart" (not to confuse with update itself not finishing which is an unrelated issue related to file DACLs). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329
From: Paul Gofman <pgofman(a)codeweavers.com> --- server/async.c | 4 +++- server/handle.c | 15 +++++++++++++++ server/handle.h | 1 + 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/server/async.c b/server/async.c index 26946b5f5ce..95e86c6f603 100644 --- a/server/async.c +++ b/server/async.c @@ -622,7 +622,9 @@ restart: LIST_FOR_EACH_ENTRY( async, &thread->process->asyncs, struct async, process_entry ) { if (async->thread != thread || async->terminated || async->canceled) continue; - if (async->completion && async->data.apc_context && !async->event) continue; + if (async->completion && async->data.apc_context && !async->event + && get_obj_handle_count( thread->process, get_fd_user( async->fd ))) + continue; async->canceled = 1; fd_cancel_async( async->fd, async ); diff --git a/server/handle.c b/server/handle.c index 38ad80da267..d6a8b40830c 100644 --- a/server/handle.c +++ b/server/handle.c @@ -501,6 +501,21 @@ unsigned int get_handle_access( struct process *process, obj_handle_t handle ) return entry->access & ~RESERVED_ALL; } +/* return number of open handles to the object in the process */ +unsigned int get_obj_handle_count( struct process *process, const struct object *obj ) +{ + struct handle_table *table = process->handles; + struct handle_entry *ptr; + unsigned int count = 0; + int i; + + if (!table) return 0; + + for (i = 0, ptr = table->entries; i <= table->last; i++, ptr++) + if (ptr->ptr == obj) ++count; + return count; +} + /* find the first inherited handle of the given type */ /* this is needed for window stations and desktops (don't ask...) */ obj_handle_t find_inherited_handle( struct process *process, const struct object_ops *ops ) diff --git a/server/handle.h b/server/handle.h index ac3104dc003..16967c1e0bd 100644 --- a/server/handle.h +++ b/server/handle.h @@ -47,6 +47,7 @@ extern obj_handle_t duplicate_handle( struct process *src, obj_handle_t src_hand extern obj_handle_t open_object( struct process *process, obj_handle_t parent, unsigned int access, const struct object_ops *ops, const struct unicode_str *name, unsigned int attr ); +extern unsigned int get_obj_handle_count( struct process *process, const struct object *obj ); extern obj_handle_t find_inherited_handle( struct process *process, const struct object_ops *ops ); extern void close_process_handles( struct process *process ); extern struct handle_table *alloc_handle_table( struct process *process, int count ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3329
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/afd.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/dlls/ws2_32/tests/afd.c b/dlls/ws2_32/tests/afd.c index 1644a9dc4ff..e613b216503 100644 --- a/dlls/ws2_32/tests/afd.c +++ b/dlls/ws2_32/tests/afd.c @@ -2506,9 +2506,9 @@ static void test_async_thread_termination(void) struct afd_poll_params *in_params = (struct afd_poll_params *)in_buffer; struct afd_poll_params *out_params = (struct afd_poll_params *)out_buffer; LARGE_INTEGER zero = {{0}}; + HANDLE event, port, port2; ULONG_PTR key, value; IO_STATUS_BLOCK io; - HANDLE event, port; ULONG params_size; SOCKET listener; unsigned int i; @@ -2516,15 +2516,8 @@ static void test_async_thread_termination(void) event = CreateEventW(NULL, FALSE, FALSE, NULL); - listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); - ret = bind(listener, (const struct sockaddr *)&bind_addr, sizeof(bind_addr)); - ok(!ret, "got error %u\n", WSAGetLastError()); - ret = listen(listener, 1); - ok(!ret, "got error %u\n", WSAGetLastError()); - in_params->count = 1; in_params->exclusive = FALSE; - in_params->sockets[0].socket = listener; in_params->sockets[0].flags = ~0; in_params->sockets[0].status = 0xdeadbeef; params_size = offsetof(struct afd_poll_params, sockets[1]); @@ -2534,6 +2527,14 @@ static void test_async_thread_termination(void) for (i = 0; i < ARRAY_SIZE(tests); ++i) { winetest_push_context("test %u", i); + + listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + ret = bind(listener, (const struct sockaddr *)&bind_addr, sizeof(bind_addr)); + ok(!ret, "got error %u\n", WSAGetLastError()); + ret = listen(listener, 1); + ok(!ret, "got error %u\n", WSAGetLastError()); + in_params->sockets[0].socket = listener; + memset(&io, 0xcc, sizeof(io)); ResetEvent(event); ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, (HANDLE)listener, tests[i].event ? event : NULL, @@ -2546,17 +2547,29 @@ static void test_async_thread_termination(void) ret = WaitForSingleObject(event, 1000); ok(!ret, "got %#x\n", ret); } + closesocket(listener); winetest_pop_context(); } SleepEx(0, TRUE); ok(!test_async_thread_termination_apc_count, "got APC.\n"); - port = CreateIoCompletionPort((HANDLE)listener, NULL, 0, 0); + port = CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 0); for (i = 0; i < ARRAY_SIZE(tests); ++i) { winetest_push_context("test %u", i); + + listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + ret = bind(listener, (const struct sockaddr *)&bind_addr, sizeof(bind_addr)); + ok(!ret, "got error %u\n", WSAGetLastError()); + ret = listen(listener, 1); + ok(!ret, "got error %u\n", WSAGetLastError()); + in_params->sockets[0].socket = listener; + + port2 = CreateIoCompletionPort((HANDLE)listener, port, 0, 0); + ok(port2 == port, "got %p, %p.\n", port, port2); + memset(&io, 0xcc, sizeof(io)); ResetEvent(event); ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, (HANDLE)listener, tests[i].event ? event : NULL, @@ -2604,12 +2617,12 @@ static void test_async_thread_termination(void) ok(!key, "got key %#Ix\n", key); ok(value == 0xdeadbeef, "got value %#Ix\n", value); ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + closesocket(listener); winetest_pop_context(); } CloseHandle(port); CloseHandle(event); - closesocket(listener); } static DWORD WINAPI sync_read_file_thread(void *arg) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3329
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/afd.c | 90 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 81 insertions(+), 9 deletions(-) diff --git a/dlls/ws2_32/tests/afd.c b/dlls/ws2_32/tests/afd.c index e613b216503..4e76d201e49 100644 --- a/dlls/ws2_32/tests/afd.c +++ b/dlls/ws2_32/tests/afd.c @@ -33,6 +33,24 @@ #define TIMEOUT_INFINITE _I64_MAX +static HANDLE create_process(const char *arg) +{ + STARTUPINFOA si = { 0 }; + PROCESS_INFORMATION pi; + char cmdline[MAX_PATH]; + char **argv; + BOOL ret; + + si.cb = sizeof(si); + winetest_get_mainargs(&argv); + sprintf(cmdline, "%s %s %s", argv[0], argv[1], arg); + ret = CreateProcessA(NULL, cmdline, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi); + ok(ret, "got %lu.\n", GetLastError()); + ret = CloseHandle(pi.hThread); + ok(ret, "got %lu.\n", GetLastError()); + return pi.hProcess; +} + static void tcp_socketpair_flags(SOCKET *src, SOCKET *dst, DWORD flags) { SOCKET server = INVALID_SOCKET; @@ -2413,10 +2431,12 @@ struct ioctl_params void *out_buffer; ULONG out_size; NTSTATUS ret; - HANDLE complete_event; + HANDLE complete_event, handle_closed_event; BOOL kill_thread; }; +static HANDLE other_process; + static DWORD WINAPI async_ioctl_thread(void *params) { struct ioctl_params *io = params; @@ -2424,20 +2444,22 @@ static DWORD WINAPI async_ioctl_thread(void *params) io->ret = NtDeviceIoControlFile(io->handle, io->event, io->apc, io->apc_context, io->io, io->code, io->in_buffer, io->in_size, io->out_buffer, io->out_size); SetEvent(io->complete_event); + WaitForSingleObject(io->handle_closed_event, INFINITE); if (io->kill_thread) Sleep(3000); return io->ret; } -static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, HANDLE handle, HANDLE event, +static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, BOOL other_process_handle, HANDLE *handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc_context, IO_STATUS_BLOCK *io, ULONG code, void *in_buffer, ULONG in_size, void *out_buffer, ULONG out_size) { + HANDLE thread, handle2; struct ioctl_params p; - HANDLE thread; DWORD ret; + BOOL bret; - p.handle = handle; + p.handle = *handle; p.event = event; p.apc = apc; p.apc_context = apc_context; @@ -2447,13 +2469,20 @@ static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, HANDLE han p.in_size = in_size; p.out_buffer = out_buffer; p.out_size = out_size; + p.handle_closed_event = CreateEventW(NULL, FALSE, FALSE, NULL); p.complete_event = CreateEventW(NULL, FALSE, FALSE, NULL); p.kill_thread = kill_thread; + bret = DuplicateHandle(GetCurrentProcess(), *handle, other_process_handle ? other_process : GetCurrentProcess(), + &handle2, 0, TRUE, DUPLICATE_SAME_ACCESS); + ok(bret, "failed, error %lu.\n", GetLastError()); + thread = CreateThread(NULL, 0, async_ioctl_thread, &p, 0, NULL); ok(!!thread, "got NULL.\n"); ret = WaitForSingleObject(p.complete_event, INFINITE); ok(ret == WAIT_OBJECT_0, "got ret %#lx.\n", ret); + CloseHandle(*handle); + SetEvent(p.handle_closed_event); if (kill_thread) TerminateThread(thread, -1); CloseHandle(p.complete_event); @@ -2461,6 +2490,7 @@ static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, HANDLE han ok(ret == WAIT_OBJECT_0, "got ret %#lx.\n", ret); CloseHandle(thread); SleepEx(0, TRUE); + *handle = other_process_handle ? NULL : handle2; return p.ret; } @@ -2479,6 +2509,7 @@ static void test_async_thread_termination(void) BOOL event; PIO_APC_ROUTINE apc; void *apc_context; + BOOL other_process_handle; } tests[] = { @@ -2498,6 +2529,24 @@ static void test_async_thread_termination(void) {TRUE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef}, {FALSE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef}, {TRUE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef}, + + /* other process handle */ + {FALSE, TRUE, NULL, NULL, TRUE}, + {TRUE, TRUE, NULL, NULL, TRUE}, + {FALSE, FALSE, NULL, NULL, TRUE}, + {TRUE, FALSE, NULL, NULL, TRUE}, + {FALSE, TRUE, test_async_thread_termination_apc, NULL, TRUE}, + {TRUE, TRUE, test_async_thread_termination_apc, NULL, TRUE}, + {FALSE, FALSE, test_async_thread_termination_apc, NULL, TRUE}, + {TRUE, FALSE, test_async_thread_termination_apc, NULL, TRUE}, + {FALSE, TRUE, NULL, (void *)0xdeadbeef, TRUE}, + {TRUE, TRUE, NULL, (void *)0xdeadbeef, TRUE}, + {FALSE, FALSE, NULL, (void *)0xdeadbeef, TRUE}, + {TRUE, FALSE, NULL, (void *)0xdeadbeef, TRUE}, + {FALSE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, + {TRUE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, + {FALSE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, + {TRUE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, }; const struct sockaddr_in bind_addr = {.sin_family = AF_INET, .sin_addr.s_addr = htonl(INADDR_LOOPBACK)}; @@ -2510,10 +2559,13 @@ static void test_async_thread_termination(void) ULONG_PTR key, value; IO_STATUS_BLOCK io; ULONG params_size; + NTSTATUS expected; SOCKET listener; unsigned int i; int ret; + other_process = create_process("sleep"); + event = CreateEventW(NULL, FALSE, FALSE, NULL); in_params->count = 1; @@ -2537,7 +2589,7 @@ static void test_async_thread_termination(void) memset(&io, 0xcc, sizeof(io)); ResetEvent(event); - ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, (HANDLE)listener, tests[i].event ? event : NULL, + ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, tests[i].other_process_handle, (HANDLE *)&listener, tests[i].event ? event : NULL, tests[i].apc, tests[i].apc_context, &io, IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); ok(ret == STATUS_PENDING, "got %#x\n", ret); @@ -2558,7 +2610,7 @@ static void test_async_thread_termination(void) for (i = 0; i < ARRAY_SIZE(tests); ++i) { - winetest_push_context("test %u", i); + winetest_push_context("test %u, other process %d", i, tests[i].other_process_handle); listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); ret = bind(listener, (const struct sockaddr *)&bind_addr, sizeof(bind_addr)); @@ -2572,7 +2624,7 @@ static void test_async_thread_termination(void) memset(&io, 0xcc, sizeof(io)); ResetEvent(event); - ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, (HANDLE)listener, tests[i].event ? event : NULL, + ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, tests[i].other_process_handle, (HANDLE *)&listener, tests[i].event ? event : NULL, tests[i].apc, tests[i].apc_context, &io, IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); if (tests[i].apc) @@ -2582,9 +2634,14 @@ static void test_async_thread_termination(void) continue; } ok(ret == STATUS_PENDING, "got %#x\n", ret); - if (!tests[i].apc_context || tests[i].event) + if (tests[i].other_process_handle || !tests[i].apc_context || tests[i].event) { - ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + if (tests[i].other_process_handle && !tests[i].event && !tests[i].apc && !!tests[i].apc_context) + expected = 0xcccccccc; + else + expected = STATUS_CANCELLED; + todo_wine_if(expected == 0xcccccccc) + ok(io.Status == expected, "got %#lx, expected %#lx.\n", io.Status, expected); memset(&io, 0xcc, sizeof(io)); key = 0xcc; value = 0; @@ -2623,6 +2680,7 @@ static void test_async_thread_termination(void) CloseHandle(port); CloseHandle(event); + TerminateProcess(other_process, 0); } static DWORD WINAPI sync_read_file_thread(void *arg) @@ -2770,6 +2828,20 @@ static void test_read_write(void) START_TEST(afd) { WSADATA data; + char **argv; + int argc; + + argc = winetest_get_mainargs(&argv); + + if (argc >= 3) + { + if (!strcmp(argv[2], "sleep")) + { + Sleep(5000); + return; + } + return; + } WSAStartup(MAKEWORD(2, 2), &data); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3329
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/afd.c | 82 +++++++++++++++++++++++++++-------------- server/async.c | 14 +++++++ server/file.h | 1 + server/sock.c | 2 + 4 files changed, 72 insertions(+), 27 deletions(-) diff --git a/dlls/ws2_32/tests/afd.c b/dlls/ws2_32/tests/afd.c index 4e76d201e49..f30d5d48d66 100644 --- a/dlls/ws2_32/tests/afd.c +++ b/dlls/ws2_32/tests/afd.c @@ -2450,9 +2450,16 @@ static DWORD WINAPI async_ioctl_thread(void *params) return io->ret; } -static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, BOOL other_process_handle, HANDLE *handle, HANDLE event, - PIO_APC_ROUTINE apc, void *apc_context, IO_STATUS_BLOCK *io, ULONG code, void *in_buffer, ULONG in_size, - void *out_buffer, ULONG out_size) +enum test_close_handle_type +{ + TEST_CLOSE_SAME_PROCESS, + TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT, + TEST_CLOSE_OTHER_AFTER_THREAD_EXIT, +}; + +static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, enum test_close_handle_type other_process_handle, + HANDLE *handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc_context, IO_STATUS_BLOCK *io, ULONG code, + void *in_buffer, ULONG in_size, void *out_buffer, ULONG out_size) { HANDLE thread, handle2; struct ioctl_params p; @@ -2481,7 +2488,8 @@ static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, BOOL other ok(!!thread, "got NULL.\n"); ret = WaitForSingleObject(p.complete_event, INFINITE); ok(ret == WAIT_OBJECT_0, "got ret %#lx.\n", ret); - CloseHandle(*handle); + if (other_process_handle != TEST_CLOSE_OTHER_AFTER_THREAD_EXIT) + CloseHandle(*handle); SetEvent(p.handle_closed_event); if (kill_thread) TerminateThread(thread, -1); @@ -2489,6 +2497,8 @@ static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, BOOL other ret = WaitForSingleObject(thread, INFINITE); ok(ret == WAIT_OBJECT_0, "got ret %#lx.\n", ret); CloseHandle(thread); + if (other_process_handle == TEST_CLOSE_OTHER_AFTER_THREAD_EXIT) + CloseHandle(*handle); SleepEx(0, TRUE); *handle = other_process_handle ? NULL : handle2; return p.ret; @@ -2509,7 +2519,7 @@ static void test_async_thread_termination(void) BOOL event; PIO_APC_ROUTINE apc; void *apc_context; - BOOL other_process_handle; + enum test_close_handle_type close_type; } tests[] = { @@ -2530,23 +2540,41 @@ static void test_async_thread_termination(void) {FALSE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef}, {TRUE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef}, - /* other process handle */ - {FALSE, TRUE, NULL, NULL, TRUE}, - {TRUE, TRUE, NULL, NULL, TRUE}, - {FALSE, FALSE, NULL, NULL, TRUE}, - {TRUE, FALSE, NULL, NULL, TRUE}, - {FALSE, TRUE, test_async_thread_termination_apc, NULL, TRUE}, - {TRUE, TRUE, test_async_thread_termination_apc, NULL, TRUE}, - {FALSE, FALSE, test_async_thread_termination_apc, NULL, TRUE}, - {TRUE, FALSE, test_async_thread_termination_apc, NULL, TRUE}, - {FALSE, TRUE, NULL, (void *)0xdeadbeef, TRUE}, - {TRUE, TRUE, NULL, (void *)0xdeadbeef, TRUE}, - {FALSE, FALSE, NULL, (void *)0xdeadbeef, TRUE}, - {TRUE, FALSE, NULL, (void *)0xdeadbeef, TRUE}, - {FALSE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, - {TRUE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, - {FALSE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, - {TRUE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TRUE}, + /* closing handle before thread exit */ + {FALSE, TRUE, NULL, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, TRUE, NULL, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, FALSE, NULL, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, FALSE, NULL, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, TRUE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, TRUE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, FALSE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, FALSE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, TRUE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, TRUE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, FALSE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, FALSE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + + /* closing handle after thread exit */ + {FALSE, TRUE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, TRUE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, FALSE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, FALSE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, TRUE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, TRUE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, FALSE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, FALSE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, TRUE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, TRUE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, FALSE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, FALSE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, }; const struct sockaddr_in bind_addr = {.sin_family = AF_INET, .sin_addr.s_addr = htonl(INADDR_LOOPBACK)}; @@ -2589,7 +2617,7 @@ static void test_async_thread_termination(void) memset(&io, 0xcc, sizeof(io)); ResetEvent(event); - ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, tests[i].other_process_handle, (HANDLE *)&listener, tests[i].event ? event : NULL, + ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, tests[i].close_type, (HANDLE *)&listener, tests[i].event ? event : NULL, tests[i].apc, tests[i].apc_context, &io, IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); ok(ret == STATUS_PENDING, "got %#x\n", ret); @@ -2610,7 +2638,7 @@ static void test_async_thread_termination(void) for (i = 0; i < ARRAY_SIZE(tests); ++i) { - winetest_push_context("test %u, other process %d", i, tests[i].other_process_handle); + winetest_push_context("test %u, other process %d", i, tests[i].close_type); listener = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); ret = bind(listener, (const struct sockaddr *)&bind_addr, sizeof(bind_addr)); @@ -2624,7 +2652,7 @@ static void test_async_thread_termination(void) memset(&io, 0xcc, sizeof(io)); ResetEvent(event); - ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, tests[i].other_process_handle, (HANDLE *)&listener, tests[i].event ? event : NULL, + ret = thread_NtDeviceIoControlFile(tests[i].kill_thread, tests[i].close_type, (HANDLE *)&listener, tests[i].event ? event : NULL, tests[i].apc, tests[i].apc_context, &io, IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); if (tests[i].apc) @@ -2634,9 +2662,9 @@ static void test_async_thread_termination(void) continue; } ok(ret == STATUS_PENDING, "got %#x\n", ret); - if (tests[i].other_process_handle || !tests[i].apc_context || tests[i].event) + if (tests[i].close_type || !tests[i].apc_context || tests[i].event) { - if (tests[i].other_process_handle && !tests[i].event && !tests[i].apc && !!tests[i].apc_context) + if (tests[i].close_type && !tests[i].event && !tests[i].apc && !!tests[i].apc_context) expected = 0xcccccccc; else expected = STATUS_CANCELLED; diff --git a/server/async.c b/server/async.c index 95e86c6f603..649a296472f 100644 --- a/server/async.c +++ b/server/async.c @@ -632,6 +632,20 @@ restart: } } +void cancel_terminated_threads_asyncs( struct process *process, struct object *obj ) +{ + struct async *async; + +restart: + LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) + { + if (async->terminated || async->canceled || async->thread->state != TERMINATED) continue; + async->canceled = 1; + fd_cancel_async( async->fd, async ); + goto restart; + } +} + /* wake up async operations on the queue */ void async_wake_up( struct async_queue *queue, unsigned int status ) { diff --git a/server/file.h b/server/file.h index 210fcec5e78..bc8537ac54c 100644 --- a/server/file.h +++ b/server/file.h @@ -246,6 +246,7 @@ extern struct thread *async_get_thread( struct async *async ); extern struct async *find_pending_async( struct async_queue *queue ); extern void cancel_process_asyncs( struct process *process ); extern void cancel_terminating_thread_asyncs( struct thread *thread ); +extern void cancel_terminated_threads_asyncs( struct process *process, struct object *obj ); static inline void init_async_queue( struct async_queue *queue ) { diff --git a/server/sock.c b/server/sock.c index 34be6ea22ef..af618c9e425 100644 --- a/server/sock.c +++ b/server/sock.c @@ -1655,6 +1655,8 @@ static int sock_close_handle( struct object *obj, struct process *process, obj_h if (signaled) complete_async_poll( poll_req, STATUS_SUCCESS ); } } + if (sock->obj.handle_count == 1 || get_obj_handle_count( process, obj ) == 1) + cancel_terminated_threads_asyncs( process, obj ); return 1; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3329
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/afd.c:
CloseHandle(port); CloseHandle(event); + TerminateProcess(other_process, 0);
We should probably still wait for and close this process handle. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_38995
Zebediah Figura (@zfigura) commented about server/sock.c:
if (signaled) complete_async_poll( poll_req, STATUS_SUCCESS ); } } + if (sock->obj.handle_count == 1 || get_obj_handle_count( process, obj ) == 1)
This condition looks redundant, but I guess it's just to try to short-circuit doing the whole loop, right? Is this logic really specific to sockets? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_38998
Zebediah Figura (@zfigura) commented about server/async.c:
} }
+void cancel_terminated_threads_asyncs( struct process *process, struct object *obj )
This function doesn't actually care about the identity of obj. Should it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_38999
Do we know that the thread actually has to die for this condition to kick in? Maybe this is evident somewhere in the tests, but I'm having a hard time finding it [they are pretty large and complex, though this is probably unavoidable]. I.e. I'm seeing two conditions for asyncs to get terminated: (1) APC port + no event + thread dies (2) no handles to the object left in originating process [+ thread dies?] We have (1) already implemented, and this patch series is adding (2), but it's not clear to me just from this patch that the "thread dies" part is necessary for (2). Is it? Am I missing a test somewhere? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39000
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/afd.c:
return io->ret; }
-static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, BOOL other_process_handle, HANDLE *handle, HANDLE event, - PIO_APC_ROUTINE apc, void *apc_context, IO_STATUS_BLOCK *io, ULONG code, void *in_buffer, ULONG in_size, - void *out_buffer, ULONG out_size) +enum test_close_handle_type +{ + TEST_CLOSE_SAME_PROCESS, + TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT, + TEST_CLOSE_OTHER_AFTER_THREAD_EXIT, +}; + +static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, enum test_close_handle_type other_process_handle,
As long as we're changing "other_process_handle" to "close_type" elsewhere we should do it here. I also suspect that things would be a bit more clear if we didn't do "if (close_type)" kind of comparisons. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39001
Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/afd.c:
+ {FALSE, TRUE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, TRUE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, FALSE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, FALSE, NULL, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, TRUE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, TRUE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, FALSE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, FALSE, test_async_thread_termination_apc, NULL, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, TRUE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, TRUE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, FALSE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {TRUE, FALSE, NULL, (void *)0xdeadbeef, TEST_CLOSE_OTHER_BEFORE_THREAD_EXIT}, + {FALSE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {FALSE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, + {TRUE, FALSE, test_async_thread_termination_apc, (void *)0xdeadbeef, TEST_CLOSE_OTHER_AFTER_THREAD_EXIT}, It might be worth doing a nested loop at least for the close type where there are 3 options.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39002
On Fri Jul 14 18:32:56 2023 +0000, Zebediah Figura wrote:
This condition looks redundant, but I guess it's just to try to short-circuit doing the whole loop, right? Is this logic really specific to sockets? Yes, it is a shortcut.
I don't know if this part is specific to sockets, but since we don't have it unified across all the other objects (and not sure how much it is feasible to unify) I suppose the other object behaviour is not directly related? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39003
On Fri Jul 14 18:32:56 2023 +0000, Zebediah Figura wrote:
This function doesn't actually care about the identity of obj. Should it? It should cancel asyncs only for the socket being closed (in a process), not for every async left alive for any terminated thread for any socket. Or am I misunderstanding the question?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39004
On Fri Jul 14 18:44:09 2023 +0000, Zebediah Figura wrote:
Do we know that the thread actually has to die for this condition to kick in? Maybe this is evident somewhere in the tests, but I'm having a hard time finding it [they are pretty large and complex, though this is probably unavoidable]. I.e. I'm seeing two conditions for asyncs to get terminated: (1) APC port + no event + thread dies (2) no handles to the object left in originating process [+ thread dies?] We have (1) already implemented, and this patch series is adding (2), but it's not clear to me just from this patch that the "thread dies" part is necessary for (2). Is it? Am I missing a test somewhere? I think test with CloseHandle() before terminating thread covers this case? With only condition (1) in place the tests with 'other process handle' enabled will fail. Test without "other process" still closes the original handle, although duplicates that into the same process thus showing that handles per process make a difference.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39007
On Fri Jul 14 18:44:09 2023 +0000, Paul Gofman wrote:
I think test with CloseHandle() before terminating thread covers this case? With only condition (1) in place the tests with 'other process handle' enabled will fail. Test without "other process" still closes the original handle, although duplicates that into the same process thus showing that handles per process make a difference. But you're still terminating the thread. Do we have a test where we create an async, duplicate the object handle into another process, then close every handle in the originating process, but without the originating thread exiting or being terminated?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39013
On Fri Jul 14 18:41:24 2023 +0000, Paul Gofman wrote:
Yes, it is a shortcut. I don't know if this part is specific to sockets, but since we don't have it unified across all the other objects (and not sure how much it is feasible to unify) I suppose the other object behaviour is not directly related? Some of the behaviour seems specific to sockets, but it's not clear that this part should be. It seems quite reasonable that it might apply to other object types as well. This is why I've been mentioning trying to reproduce the same cancellation with named pipes or something.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39014
On Fri Jul 14 18:41:26 2023 +0000, Paul Gofman wrote:
It should cancel asyncs only for the socket being closed (in a process), not for every async left alive for any terminated thread for any socket. Or am I misunderstanding the question? From the function signature, it seems like that was the intention, yes, and that seems reasonable. But the function doesn't actually use "obj", I guess unintentionally?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39015
On Fri Jul 14 20:11:50 2023 +0000, Zebediah Figura wrote:
Some of the behaviour seems specific to sockets, but it's not clear that this part should be. It seems quite reasonable that it might apply to other object types as well. This is why I've been mentioning trying to reproduce the same cancellation with named pipes or something. Yes, it might be interesting, but do you think it is strictly related to this patchset? Reproducing that for other objects types and fixing those is a lot more than this patchset was intended to do. Unless you think that if e. g., named pipes needs the same this part (which is only a few lines in the socket specific part) this should be done some way entirely different?
Just e. g., there are also other thing which tests here uncovered, that async cancelation with completion (and some other conditions) doesn't update IO status until removing completion. Meaning if we do that without removing completion when Windows doesn't we might be accessing the already free memory. We can't fix everything at once. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39016
On Fri Jul 14 20:07:31 2023 +0000, Zebediah Figura wrote:
But you're still terminating the thread. Do we have a test where we create an async, duplicate the object handle into another process, then close every handle in the originating process, but without the originating thread exiting or being terminated? Yes, this is what added in the last patch with handle close type switch?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39017
On Fri Jul 14 20:25:26 2023 +0000, Paul Gofman wrote:
Yes, this is what added in the last patch with handle close type switch? Ah, I see what you mean, it is not exactly that. I did tested it separately, closing socket handle with another handle open in another process didn't cancel any asyncs in this case. I will try to add it somehow.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39018
On Fri Jul 14 20:13:07 2023 +0000, Zebediah Figura wrote:
From the function signature, it seems like that was the intention, yes, and that seems reasonable. But the function doesn't actually use "obj", I guess unintentionally? Oh yeah, sorry, looks like the check that async is for given object is plainly missing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39019
On Fri Jul 14 20:24:27 2023 +0000, Paul Gofman wrote:
Yes, it might be interesting, but do you think it is strictly related to this patchset? Reproducing that for other objects types and fixing those is a lot more than this patchset was intended to do. Unless you think that if e. g., named pipes needs the same this part (which is only a few lines in the socket specific part) this should be done some way entirely different? Just e. g., there are also other thing which tests here uncovered, that async cancelation with completion (and some other conditions) doesn't update IO status until removing completion. Meaning if we do that without removing completion when Windows doesn't we might be accessing the already free memory. We can't fix everything at once. Well, if it applies to all file types, it suggests that we may want to fix this in the caller to close_handle(), so yes, there would be a relatively simple way to fix it for all file types. There's not quite precedent for that, but I think it wouldn't be unreasonable.
Perhaps more saliently, though, if the condition for termination is "all process handles are closed and the thread is terminated", well... we're handling that in two places, based on which condition happens first, but in one case we're being agnostic as to the file type and in the other place we're not. That decidedly doesn't look right. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39020
On Fri Jul 14 20:34:41 2023 +0000, Zebediah Figura wrote:
Well, if it applies to all file types, it suggests that we may want to fix this in the caller to close_handle(), so yes, there would be a relatively simple way to fix it for all file types. There's not quite precedent for that, but I think it wouldn't be unreasonable. Perhaps more saliently, though, if the condition for termination is "all process handles are closed and the thread is terminated", well... we're handling that in two places, based on which condition happens first, but in one case we're being agnostic as to the file type and in the other place we're not. That decidedly doesn't look right. I see, do you think testing that with just named pipes would be enough?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39021
On Fri Jul 14 20:43:58 2023 +0000, Paul Gofman wrote:
I see, do you think testing that with just named pipes would be enough? Yeah, if it happens with named pipes that's probably sufficiently convincing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39022
So additional testing with closing handle without any thread termination showed that actually missing bit here has nothing to do with thread termination. It is just closing the last handle in a process triggers async cancellation, but that happens under specific conditions only and those conditions are reverse to the conditions which lead to not canceling async when thread that queued it exits. That's why I missed that in my earlier testing, I didn't reproduce those conditions when closing handle, but now when I tried integrating that into the test which has all the combinations of related async options it became clear. In fact, it makes more sense. If the thread which queued async terminated and all the object handles in process are closed, the async is guaranteed to be canceled. Depending on apc_context, event and completion port presence that happens either during thread exit or in close_handle. I also tested that with named pipes and those seem to behave the same. I opened a new !3339 instead of this one. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329#note_39061
This merge request was closed by Paul Gofman. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3329
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)