[PATCH v4 0/2] MR135: Cancel asyncs queued by thread when the thread is terminated
Inspired by the discussion for MR 127 (https://www.winehq.org/pipermail/wine-devel/2022-May/217832.html) and is aimed to replace the second patch (ws2_32: Use allocated IO status block in select().) in that MR. When the thread calls server's terminate_thread for itself queuing the system APC though SIGUSR1 might race with the thread exit, there seems no easy and reliable way to be sure that signal is processed before the thread's stack and TEB are freed. Thus I am making the system APCs to be queued to the thread to be processed by server_select() after (terminate_thread) server call. When terminate_thread is called for the other thread the system APC will always be delivered through some other thread. Thread stack and TEB are not freed when the thread is killed externally, so completing the async from another thread should probably be fine. -- v4: ws2_32/tests: Add tests for terminated thread asyncs completion. ntdll: Cancel asyncs when thread is terminated. https://gitlab.winehq.org/wine/wine/-/merge_requests/135
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/thread.c | 24 +++++++++++++----------- server/async.c | 16 ++++++++++++++++ server/file.h | 1 + server/thread.c | 1 + 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index 15bb3be34b2..ad47a5fce74 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -1610,20 +1610,22 @@ NTSTATUS WINAPI NtAlertThread( HANDLE handle ) NTSTATUS WINAPI NtTerminateThread( HANDLE handle, LONG exit_code ) { NTSTATUS ret; - BOOL self = (handle == GetCurrentThread()); + BOOL self; - if (!self || exit_code) + SERVER_START_REQ( terminate_thread ) { - SERVER_START_REQ( terminate_thread ) - { - req->handle = wine_server_obj_handle( handle ); - req->exit_code = exit_code; - ret = wine_server_call( req ); - self = !ret && reply->self; - } - SERVER_END_REQ; + req->handle = wine_server_obj_handle( handle ); + req->exit_code = exit_code; + ret = wine_server_call( req ); + self = !ret && reply->self; + } + SERVER_END_REQ; + + if (self) + { + server_select( NULL, 0, SELECT_INTERRUPTIBLE, 0, NULL, NULL ); + exit_thread( exit_code ); } - if (self) exit_thread( exit_code ); return ret; } diff --git a/server/async.c b/server/async.c index a4fbeab555e..4832d69b7bf 100644 --- a/server/async.c +++ b/server/async.c @@ -593,6 +593,22 @@ void cancel_process_asyncs( struct process *process ) cancel_async( process, NULL, NULL, 0 ); } +void cancel_terminating_thread_asyncs( struct thread *thread ) +{ + struct async *async; + +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; + + 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 9f9d4cd4e1a..0ffe0e2c8dc 100644 --- a/server/file.h +++ b/server/file.h @@ -245,6 +245,7 @@ extern struct iosb *async_get_iosb( struct async *async ); 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 ); static inline void init_async_queue( struct async_queue *queue ) { diff --git a/server/thread.c b/server/thread.c index 467ccd1f0db..f49fbf40b78 100644 --- a/server/thread.c +++ b/server/thread.c @@ -1462,6 +1462,7 @@ DECL_HANDLER(terminate_thread) thread->exit_code = req->exit_code; if (thread != current) kill_thread( thread, 1 ); else reply->self = 1; + cancel_terminating_thread_asyncs( thread ); release_object( thread ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/135
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/afd.c | 235 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+) diff --git a/dlls/ws2_32/tests/afd.c b/dlls/ws2_32/tests/afd.c index 5b64ccd7715..3701c635bce 100644 --- a/dlls/ws2_32/tests/afd.c +++ b/dlls/ws2_32/tests/afd.c @@ -2355,6 +2355,240 @@ static void test_getsockname(void) CloseHandle(event); } +struct ioctl_params +{ + 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; + NTSTATUS ret; + HANDLE complete_event; + BOOL kill_thread; +}; + +static DWORD WINAPI async_ioctl_thread(void *params) +{ + struct ioctl_params *io = 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); + if (io->kill_thread) + Sleep(3000); + return io->ret; +} + +static NTSTATUS WINAPI thread_NtDeviceIoControlFile(BOOL kill_thread, 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) +{ + struct ioctl_params p; + HANDLE thread; + DWORD ret; + + p.handle = handle; + p.event = event; + p.apc = apc; + p.apc_context = apc_context; + p.io = io; + p.code = code; + p.in_buffer = in_buffer; + p.in_size = in_size; + p.out_buffer = out_buffer; + p.out_size = out_size; + p.complete_event = CreateEventW(NULL, FALSE, FALSE, NULL); + p.kill_thread = kill_thread; + + 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); + if (kill_thread) + TerminateThread(thread, -1); + CloseHandle(p.complete_event); + ret = WaitForSingleObject(thread, INFINITE); + ok(ret == WAIT_OBJECT_0, "got ret %#lx.\n", ret); + CloseHandle(thread); + SleepEx(0, TRUE); + return p.ret; +} + +static unsigned int test_async_thread_termination_apc_count; + +static void WINAPI test_async_thread_termination_apc( void *arg, IO_STATUS_BLOCK *iosb, ULONG reserved ) +{ + ++test_async_thread_termination_apc_count; +} + +static void test_async_thread_termination(void) +{ + static const struct + { + BOOL kill_thread; + BOOL event; + PIO_APC_ROUTINE apc; + void *apc_context; + } + tests_no_port[] = + { + {FALSE, TRUE, NULL, NULL}, + {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, 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}, + }; + + 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; + LARGE_INTEGER zero = {{0}}; + ULONG_PTR key, value; + IO_STATUS_BLOCK io; + HANDLE event, port; + ULONG params_size; + SOCKET listener; + unsigned int i; + int ret; + + 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]); + in_params->timeout = -10 * 1000 * 1000 * 5; + + /* asyncs without completion port are always cancelled on thread exit. */ + for (i = 0; i < ARRAY_SIZE(tests_no_port); ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, tests_no_port[i].event ? event : NULL, + tests_no_port[i].apc, tests_no_port[i].apc_context, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + if (tests_no_port[i].event) + { + ret = WaitForSingleObject(event, 1000); + ok(!ret, "got %#x\n", ret); + } + winetest_pop_context(); + } + + SleepEx(0, TRUE); + ok(!test_async_thread_termination_apc_count, "got APC.\n"); + + port = CreateIoCompletionPort((HANDLE)listener, NULL, 0, 0); + + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, event, NULL, (void *)0xdeadbeef, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + ret = WaitForSingleObject(event, 1000); + ok(!ret, "got %#x\n", ret); + + memset(&io, 0xcc, sizeof(io)); + key = 0xcc; + value = 0; + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(!ret, "got %#x\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); + winetest_pop_context(); + } + + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, event, NULL, NULL, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); + winetest_pop_context(); + } + + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, NULL, NULL, NULL, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); + winetest_pop_context(); + } + + /* async is not cancelled if there is a completion port and no event. */ + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, NULL, NULL, (void *)0xdeadbeef, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + 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); + ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); + CancelIoEx((HANDLE)listener, NULL); + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(!ret, "got %#x\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); + winetest_pop_context(); + } + + CloseHandle(port); + CloseHandle(event); + closesocket(listener); +} + START_TEST(afd) { WSADATA data; @@ -2372,6 +2606,7 @@ START_TEST(afd) test_get_events_reset(); test_bind(); test_getsockname(); + test_async_thread_termination(); WSACleanup(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/135
On 6/6/22 16:55, Paul Gofman wrote:
+ /* asyncs without completion port are always cancelled on thread exit. */ + for (i = 0; i < ARRAY_SIZE(tests_no_port); ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, tests_no_port[i].event ? event : NULL,
Shouldn't "i" here be "tests_no_port[i].kill_thread"?
+ tests_no_port[i].apc, tests_no_port[i].apc_context, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + if (tests_no_port[i].event) + { + ret = WaitForSingleObject(event, 1000); + ok(!ret, "got %#x\n", ret); + } + winetest_pop_context(); + } + + SleepEx(0, TRUE); + ok(!test_async_thread_termination_apc_count, "got APC.\n"); + + port = CreateIoCompletionPort((HANDLE)listener, NULL, 0, 0); + + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, event, NULL, (void *)0xdeadbeef, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + ret = WaitForSingleObject(event, 1000); + ok(!ret, "got %#x\n", ret); + + memset(&io, 0xcc, sizeof(io)); + key = 0xcc; + value = 0; + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(!ret, "got %#x\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); + winetest_pop_context(); + } + + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ResetEvent(event); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, event, NULL, NULL, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); + winetest_pop_context(); + } + + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, NULL, NULL, NULL, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); + + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); + winetest_pop_context(); + } + + /* async is not cancelled if there is a completion port and no event. */ + for (i = 0; i < 2; ++i) + { + winetest_push_context("test %u", i); + memset(&io, 0xcc, sizeof(io)); + ret = thread_NtDeviceIoControlFile(i, (HANDLE)listener, NULL, NULL, (void *)0xdeadbeef, &io, + IOCTL_AFD_POLL, in_params, params_size, out_params, params_size); + ok(ret == STATUS_PENDING, "got %#x\n", ret); + 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); + ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); + CancelIoEx((HANDLE)listener, NULL); + ret = NtRemoveIoCompletion(port, &key, &value, &io, &zero); + ok(!ret, "got %#x\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); + winetest_pop_context(); + }
FWIW, in cases like this I often find it preferable to iterate over the same loop but do something like if (tests[i].apc_context && !tests[i].event) { ok(io.Status == 0xcccccccc, "got %#lx\n", io.Status); /* other tests... */ } else { ok(io.Status == STATUS_CANCELLED, "got %#lx\n", io.Status); /* other tests... */ } Just something to consider.
v4 changes: 1. Get server_select() in NtTerminateThread() back for terminating self; 2. Make (terminate_thread) server call unconditional in NtTerminateThread() (that was missed earlier, that call should always be there for properly canceling asyncs); 3. After p. 2. it appeared that things are more convoluted that we initially thought as the tests in ws2_32/tests/sock.c:test_iocp() started failing. There are tests which show that, e. g., an async from overlapped WSARecv() doesn't get canceled when thread which queued it exits. It required some investigation and much more testing, and so also after some discussion of intermediate results with Zebediah I figured out that the asyncs are not actual canceled after the following conditions: - The async has an associated completion port; - The async is queued with nonzero 'apc_context' (that is, the async completion is actually going to be delivered to the completion port); - There is no event associated with async. So I added much more tests, added corresponding condition to the server part of the patch, and moved test after the implementation to avoid obscure temporary bypassing of test failures which becomes a bit convoluted. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/135#note_1629
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Zebediah Figura