[PATCH 0/3] MR3339: server: Cancel asyncs when the last handle in process is closed.
Supersedes !3329. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3339
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/tests/pipe.c | 143 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index 243b2794888..c9cfdb03826 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -131,6 +131,24 @@ static BOOL init_func_ptrs(void) return TRUE; } +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 inline BOOL is_signaled( HANDLE obj ) { return WaitForSingleObject( obj, 0 ) == WAIT_OBJECT_0; @@ -2971,13 +2989,137 @@ static void test_pipe_names(void) } } +static void test_async_cancel_on_handle_close(void) +{ + static const struct + { + BOOL event; + PIO_APC_ROUTINE apc; + BOOL apc_context; + } + tests[] = + { + {TRUE, NULL}, + {FALSE, NULL}, + {TRUE, ioapc}, + {FALSE, ioapc}, + {TRUE, NULL, TRUE}, + {FALSE, NULL, TRUE}, + {TRUE, ioapc, TRUE}, + {FALSE, ioapc, TRUE}, + }; + + FILE_IO_COMPLETION_NOTIFICATION_INFORMATION info; + char read_buf[16]; + HANDLE port, write, read, event, handle2, process_handle; + IO_STATUS_BLOCK io; + NTSTATUS status; + unsigned int i, other_process; + DWORD ret; + BOOL bret; + + create_pipe_pair(&read, &write, FILE_FLAG_OVERLAPPED | PIPE_ACCESS_DUPLEX, + PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE, 4096); + + status = pNtQueryInformationFile(read, &io, &info, sizeof(info), + FileIoCompletionNotificationInformation); + ok(status == STATUS_SUCCESS || broken(status == STATUS_INVALID_INFO_CLASS), + "status = %lx\n", status); + CloseHandle(read); + CloseHandle(write); + if (status) + { + win_skip("FileIoCompletionNotificationInformation is not supported.\n"); + return; + } + + process_handle = create_process("sleep"); + event = CreateEventW(NULL, FALSE, FALSE, NULL); + + for (other_process = 0; other_process < 2; ++other_process) + { + for (i = 0; i < ARRAY_SIZE(tests); ++i) + { + winetest_push_context("other_process %u, i %u", other_process, i); + create_pipe_pair(&read, &write, FILE_FLAG_OVERLAPPED | PIPE_ACCESS_DUPLEX, + PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE, 4096); + port = CreateIoCompletionPort(read, NULL, 0, 0); + ok(!!port, "got %p.\n", port); + + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + status = NtReadFile(read, tests[i].event ? event : NULL, tests[i].apc, tests[i].apc_context ? &io : NULL, &io, + read_buf, 16, NULL, NULL); + if (tests[i].apc) + { + ok(status == STATUS_INVALID_PARAMETER, "got %#lx.\n", status); + CloseHandle(read); + CloseHandle(write); + CloseHandle(port); + winetest_pop_context(); + continue; + } + ok(status == STATUS_PENDING, "got %#lx.\n", status); + ok(io.Status == 0xcccccccc, "got %#lx.\n", io.Status); + + bret = DuplicateHandle(GetCurrentProcess(), read, other_process ? process_handle : GetCurrentProcess(), + &handle2, 0, FALSE, DUPLICATE_SAME_ACCESS); + ok(bret, "failed, error %lu.\n", GetLastError()); + + CloseHandle(read); + ok(io.Status == 0xcccccccc, "got %#lx.\n", io.Status); + if (other_process && tests[i].apc_context && !tests[i].event) + todo_wine test_queued_completion(port, &io, STATUS_CANCELLED, 0); + else + test_no_queued_completion(port); + + ret = WaitForSingleObject(event, 0); + ok(ret == WAIT_TIMEOUT, "got %#lx.\n", ret); + + if (other_process) + { + bret = DuplicateHandle(process_handle, handle2, GetCurrentProcess(), &read, 0, FALSE, + DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE); + ok(bret, "failed, error %lu.\n", GetLastError()); + } + else + { + read = handle2; + } + CloseHandle(read); + CloseHandle(write); + CloseHandle(port); + winetest_pop_context(); + } + } + + CloseHandle(event); + TerminateProcess(process_handle, 0); + WaitForSingleObject(process_handle, INFINITE); + CloseHandle(process_handle); +} + START_TEST(pipe) { + char **argv; + int argc; + if (!init_func_ptrs()) return; if (!pIsWow64Process || !pIsWow64Process( GetCurrentProcess(), &is_wow64 )) is_wow64 = FALSE; + argc = winetest_get_mainargs(&argv); + if (argc >= 3) + { + if (!strcmp(argv[2], "sleep")) + { + Sleep(5000); + return; + } + return; + } + trace("starting invalid create tests\n"); test_create_invalid(); @@ -3031,6 +3173,7 @@ START_TEST(pipe) test_security_info(); test_empty_name(); test_pipe_names(); + test_async_cancel_on_handle_close(); pipe_for_each_state(create_pipe_server, connect_pipe, test_pipe_state); pipe_for_each_state(create_pipe_server, connect_and_write_pipe, test_pipe_with_data_state); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3339
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/afd.c | 184 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 172 insertions(+), 12 deletions(-) diff --git a/dlls/ws2_32/tests/afd.c b/dlls/ws2_32/tests/afd.c index 1644a9dc4ff..ef3e6385c7f 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; @@ -2464,11 +2482,11 @@ static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, HANDLE han return p.ret; } -static unsigned int test_async_thread_termination_apc_count; +static unsigned int test_apc_count; -static void WINAPI test_async_thread_termination_apc( void *arg, IO_STATUS_BLOCK *iosb, ULONG reserved ) +static void WINAPI test_apc_proc( void *arg, IO_STATUS_BLOCK *iosb, ULONG reserved ) { - ++test_async_thread_termination_apc_count; + ++test_apc_count; } static void test_async_thread_termination(void) @@ -2486,18 +2504,18 @@ static void test_async_thread_termination(void) {TRUE, TRUE, NULL, NULL}, {FALSE, FALSE, NULL, NULL}, {TRUE, FALSE, NULL, NULL}, - {FALSE, TRUE, test_async_thread_termination_apc, NULL}, - {TRUE, TRUE, test_async_thread_termination_apc, NULL}, - {FALSE, FALSE, test_async_thread_termination_apc, NULL}, - {TRUE, FALSE, test_async_thread_termination_apc, NULL}, + {FALSE, TRUE, test_apc_proc, NULL}, + {TRUE, TRUE, test_apc_proc, NULL}, + {FALSE, FALSE, test_apc_proc, NULL}, + {TRUE, FALSE, test_apc_proc, NULL}, {FALSE, TRUE, NULL, (void *)0xdeadbeef}, {TRUE, TRUE, NULL, (void *)0xdeadbeef}, {FALSE, FALSE, NULL, (void *)0xdeadbeef}, {TRUE, FALSE, NULL, (void *)0xdeadbeef}, - {FALSE, TRUE, test_async_thread_termination_apc, (void *)0xdeadbeef}, - {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}, + {FALSE, TRUE, test_apc_proc, (void *)0xdeadbeef}, + {TRUE, TRUE, test_apc_proc, (void *)0xdeadbeef}, + {FALSE, FALSE, test_apc_proc, (void *)0xdeadbeef}, + {TRUE, FALSE, test_apc_proc, (void *)0xdeadbeef}, }; const struct sockaddr_in bind_addr = {.sin_family = AF_INET, .sin_addr.s_addr = htonl(INADDR_LOOPBACK)}; @@ -2550,7 +2568,7 @@ static void test_async_thread_termination(void) } SleepEx(0, TRUE); - ok(!test_async_thread_termination_apc_count, "got APC.\n"); + ok(!test_apc_count, "got APC.\n"); port = CreateIoCompletionPort((HANDLE)listener, NULL, 0, 0); @@ -2754,12 +2772,153 @@ static void test_read_write(void) CloseHandle(event); } +static void test_async_cancel_on_handle_close(void) +{ + static const struct + { + BOOL event; + PIO_APC_ROUTINE apc; + void *apc_context; + } + tests[] = + { + {TRUE, NULL, NULL}, + {FALSE, NULL, NULL}, + {TRUE, test_apc_proc, NULL}, + {FALSE, test_apc_proc, NULL}, + {TRUE, NULL, (void *)0xdeadbeef}, + {FALSE, NULL, (void *)0xdeadbeef}, + {TRUE, test_apc_proc, (void *)0xdeadbeef}, + {FALSE, test_apc_proc, (void *)0xdeadbeef}, + }; + + const struct sockaddr_in bind_addr = {.sin_family = AF_INET, .sin_addr.s_addr = htonl(INADDR_LOOPBACK)}; + char in_buffer[offsetof(struct afd_poll_params, sockets[3])]; + char out_buffer[offsetof(struct afd_poll_params, sockets[3])]; + struct afd_poll_params *in_params = (struct afd_poll_params *)in_buffer; + struct afd_poll_params *out_params = (struct afd_poll_params *)out_buffer; + unsigned int i, other_process; + LARGE_INTEGER zero = {{0}}; + HANDLE process_handle; + ULONG_PTR key, value; + IO_STATUS_BLOCK io; + HANDLE event, port; + ULONG params_size; + SOCKET listener; + HANDLE handle2; + DWORD ret; + BOOL bret; + + process_handle = create_process("sleep"); + + event = CreateEventW(NULL, FALSE, FALSE, NULL); + + in_params->count = 1; + in_params->exclusive = FALSE; + in_params->sockets[0].flags = ~0; + in_params->sockets[0].status = 0xdeadbeef; + params_size = offsetof(struct afd_poll_params, sockets[1]); + in_params->timeout = -10 * 1000 * 1000 * 5; + + for (other_process = 0; other_process < 2; ++other_process) + { + for (i = 0; i < ARRAY_SIZE(tests); ++i) + { + winetest_push_context("other_process %u, i %u", other_process, 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()); + + port = CreateIoCompletionPort((HANDLE)listener, NULL, 0, 0); + ok(!!port, "got %p.\n", port); + + in_params->sockets[0].socket = listener; + + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + ret = NtDeviceIoControlFile((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) + { + ok(ret == STATUS_INVALID_PARAMETER, "got %#lx\n", ret); + winetest_pop_context(); + continue; + } + ok(ret == STATUS_PENDING, "got %#lx.\n", ret); + ok(io.Status == 0xcccccccc, "got %#lx.\n", io.Status); + + bret = DuplicateHandle(GetCurrentProcess(), (HANDLE)listener, + other_process ? process_handle : GetCurrentProcess(), + &handle2, 0, FALSE, DUPLICATE_SAME_ACCESS); + ok(bret, "failed, error %lu.\n", GetLastError()); + + closesocket(listener); + + ok(io.Status == 0xcccccccc, "got %#lx\n", io.Status); + memset(&io, 0xcc, sizeof(io)); + key = 0xcc; + value = 0; + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + if (other_process && tests[i].apc_context && !tests[i].event) + { + todo_wine ok(!ret, "got %#lx\n", ret); + todo_wine ok(!key, "got key %#Ix\n", key); + todo_wine ok(value == 0xdeadbeef, "got value %#Ix\n", value); + todo_wine ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + } + else + { + ok(ret == WAIT_TIMEOUT, "got %#lx\n", ret); + } + + ret = WaitForSingleObject(event, 0); + ok(ret == WAIT_TIMEOUT, "got %#lx.\n", ret); + + if (other_process) + { + bret = DuplicateHandle(process_handle, handle2, GetCurrentProcess(), (HANDLE *)&listener, 0, FALSE, + DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE); + ok(bret, "failed, error %lu.\n", GetLastError()); + } + else + { + listener = (SOCKET)handle2; + } + + CloseHandle((HANDLE)listener); + CloseHandle(port); + winetest_pop_context(); + } + } + CloseHandle(event); + TerminateProcess(process_handle, 0); + WaitForSingleObject(process_handle, INFINITE); + CloseHandle(process_handle); +} + START_TEST(afd) { WSADATA data; + char **argv; + int argc; WSAStartup(MAKEWORD(2, 2), &data); + argc = winetest_get_mainargs(&argv); + if (argc >= 3) + { + if (!strcmp(argv[2], "sleep")) + { + Sleep(5000); + return; + } + return; + } + test_open_device(); test_poll(); test_poll_exclusive(); @@ -2773,6 +2932,7 @@ START_TEST(afd) test_getsockname(); test_async_thread_termination(); test_read_write(); + test_async_cancel_on_handle_close(); WSACleanup(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3339
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/tests/pipe.c | 5 ++++- dlls/ws2_32/tests/afd.c | 11 +++++++---- server/async.c | 16 ++++++++++++++++ server/file.h | 1 + server/handle.c | 18 ++++++++++++++++++ 5 files changed, 46 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index c9cfdb03826..a1115bf47b9 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -3067,9 +3067,12 @@ static void test_async_cancel_on_handle_close(void) ok(bret, "failed, error %lu.\n", GetLastError()); CloseHandle(read); + /* Canceled asyncs with completion port and no event do not update IOSB before removing completion. */ + todo_wine_if(other_process && tests[i].apc_context && !tests[i].event) ok(io.Status == 0xcccccccc, "got %#lx.\n", io.Status); + if (other_process && tests[i].apc_context && !tests[i].event) - todo_wine test_queued_completion(port, &io, STATUS_CANCELLED, 0); + test_queued_completion(port, &io, STATUS_CANCELLED, 0); else test_no_queued_completion(port); diff --git a/dlls/ws2_32/tests/afd.c b/dlls/ws2_32/tests/afd.c index ef3e6385c7f..97139605bf1 100644 --- a/dlls/ws2_32/tests/afd.c +++ b/dlls/ws2_32/tests/afd.c @@ -2858,17 +2858,20 @@ static void test_async_cancel_on_handle_close(void) closesocket(listener); + /* Canceled asyncs with completion port and no event do not update IOSB before removing completion. */ + todo_wine_if(other_process && tests[i].apc_context && !tests[i].event) ok(io.Status == 0xcccccccc, "got %#lx\n", io.Status); + memset(&io, 0xcc, sizeof(io)); key = 0xcc; value = 0; ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); if (other_process && tests[i].apc_context && !tests[i].event) { - todo_wine ok(!ret, "got %#lx\n", ret); - todo_wine ok(!key, "got key %#Ix\n", key); - todo_wine ok(value == 0xdeadbeef, "got value %#Ix\n", value); - todo_wine ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + ok(!ret, "got %#lx\n", ret); + 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); } else { diff --git a/server/async.c b/server/async.c index 26946b5f5ce..b1d09129938 100644 --- a/server/async.c +++ b/server/async.c @@ -614,6 +614,22 @@ void cancel_process_asyncs( struct process *process ) cancel_async( process, NULL, NULL, 0 ); } +void cancel_asyncs_on_handles_closed( 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 || get_fd_user( async->fd ) != obj) continue; + if (!async->completion || !async->data.apc_context || async->event) continue; + + async->canceled = 1; + fd_cancel_async( async->fd, async ); + goto restart; + } +} + void cancel_terminating_thread_asyncs( struct thread *thread ) { struct async *async; diff --git a/server/file.h b/server/file.h index 210fcec5e78..778a8c7a065 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_asyncs_on_handles_closed( struct process *process, struct object *obj ); static inline void init_async_queue( struct async_queue *queue ) { diff --git a/server/handle.c b/server/handle.c index 38ad80da267..5f48e458431 100644 --- a/server/handle.c +++ b/server/handle.c @@ -37,6 +37,7 @@ #include "process.h" #include "thread.h" #include "security.h" +#include "file.h" #include "request.h" struct handle_entry @@ -417,6 +418,21 @@ struct handle_table *copy_handle_table( struct process *process, struct process return table; } +/* return number of open handles to the object in the process */ +static 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; +} + /* close a handle and decrement the refcount of the associated object */ unsigned int close_handle( struct process *process, obj_handle_t handle ) { @@ -428,6 +444,8 @@ unsigned int close_handle( struct process *process, obj_handle_t handle ) if (entry->access & RESERVED_CLOSE_PROTECT) return STATUS_HANDLE_NOT_CLOSABLE; obj = entry->ptr; if (!obj->ops->close_handle( obj, process, handle )) return STATUS_HANDLE_NOT_CLOSABLE; + if (obj->handle_count == 1 || get_obj_handle_count( process, obj ) == 1) + cancel_asyncs_on_handles_closed( process, obj ); entry->ptr = NULL; table = handle_is_global(handle) ? global_table : process->handles; if (entry < table->entries + table->free) table->free = entry - table->entries; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3339
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134890 Your paranoid android. === debian11 (32 bit report) === ntdll: file.c:1182: Test failed: Invalid iosb.Status: c0000120
participants (3)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)