[PATCH 0/4] MR7797: ntdll: Wait for all pending operations to be cancelled in NtCancelIoFile[Ex]().
This fixes Trials Fusion often crashing when disconnecting a controller while there are more still connected. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/ntdll/unix/file.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index a475fcc3225..aacd96f5a3d 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -6421,19 +6421,16 @@ NTSTATUS WINAPI NtFlushBuffersFileEx( HANDLE handle, ULONG flags, void *params, } -/************************************************************************** - * NtCancelIoFile (NTDLL.@) - */ -NTSTATUS WINAPI NtCancelIoFile( HANDLE handle, IO_STATUS_BLOCK *io_status ) +static NTSTATUS cancel_io( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK *io_status, + BOOL only_thread ) { unsigned int status; - TRACE( "%p %p\n", handle, io_status ); - SERVER_START_REQ( cancel_async ) { req->handle = wine_server_obj_handle( handle ); - req->only_thread = TRUE; + req->iosb = wine_server_client_ptr( io ); + req->only_thread = only_thread; if (!(status = wine_server_call( req ))) { io_status->Status = status; @@ -6446,28 +6443,25 @@ NTSTATUS WINAPI NtCancelIoFile( HANDLE handle, IO_STATUS_BLOCK *io_status ) } +/************************************************************************** + * NtCancelIoFile (NTDLL.@) + */ +NTSTATUS WINAPI NtCancelIoFile( HANDLE handle, IO_STATUS_BLOCK *io_status ) +{ + TRACE( "%p %p\n", handle, io_status ); + + return cancel_io( handle, NULL, io_status, TRUE ); +} + + /************************************************************************** * NtCancelIoFileEx (NTDLL.@) */ NTSTATUS WINAPI NtCancelIoFileEx( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK *io_status ) { - unsigned int status; - TRACE( "%p %p %p\n", handle, io, io_status ); - SERVER_START_REQ( cancel_async ) - { - req->handle = wine_server_obj_handle( handle ); - req->iosb = wine_server_client_ptr( io ); - if (!(status = wine_server_call( req ))) - { - io_status->Status = status; - io_status->Information = 0; - } - } - SERVER_END_REQ; - - return status; + return cancel_io( handle, io, io_status, FALSE ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/ntdll/unix/file.c | 51 ++++++++++++++++++++----- include/wine/server_protocol.h | 5 ++- server/async.c | 69 ++++++++++++++++++++++++++++++---- server/protocol.def | 3 ++ server/request_handlers.h | 2 + server/request_trace.h | 8 +++- 6 files changed, 120 insertions(+), 18 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index aacd96f5a3d..f934bcc24e7 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -6424,20 +6424,53 @@ NTSTATUS WINAPI NtFlushBuffersFileEx( HANDLE handle, ULONG flags, void *params, static NTSTATUS cancel_io( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK *io_status, BOOL only_thread ) { - unsigned int status; + unsigned int status, i, count = 8, size; + obj_handle_t *cancelled_handles = NULL; + data_size_t handles_size = 0; + union select_op select_op; - SERVER_START_REQ( cancel_async ) + for (;;) { - req->handle = wine_server_obj_handle( handle ); - req->iosb = wine_server_client_ptr( io ); - req->only_thread = only_thread; - if (!(status = wine_server_call( req ))) + size = count * sizeof(*cancelled_handles); + cancelled_handles = malloc( size ); + if (!cancelled_handles) + return STATUS_NO_MEMORY; + + SERVER_START_REQ( cancel_async ) { - io_status->Status = status; - io_status->Information = 0; + req->handle = wine_server_obj_handle( handle ); + req->iosb = wine_server_client_ptr( io ); + req->only_thread = only_thread; + wine_server_set_reply( req, cancelled_handles, size ); + if (!(status = wine_server_call( req ))) + { + io_status->Status = status; + io_status->Information = 0; + handles_size = wine_server_reply_size( reply ); + } + else if (status == STATUS_BUFFER_OVERFLOW) + { + free( cancelled_handles ); + count = reply->handles_size / sizeof(*cancelled_handles); + } } + SERVER_END_REQ; + if (status != STATUS_BUFFER_OVERFLOW) + break; + } + + if (handles_size) + { + for (i = 0; i < handles_size / sizeof(*cancelled_handles); ++i) + { + select_op.wait.op = SELECT_WAIT; + select_op.wait.handles[0] = cancelled_handles[i]; + + server_select( &select_op, offsetof(union select_op, wait.handles[1]), + SELECT_INTERRUPTIBLE, TIMEOUT_INFINITE, NULL, NULL ); + } + free( cancelled_handles ); } - SERVER_END_REQ; return status; } diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index 3cdc9375e2d..4a5075ea33f 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -3238,6 +3238,9 @@ struct cancel_async_request struct cancel_async_reply { struct reply_header __header; + data_size_t handles_size; + /* VARARG(handles,uints,handles_size); */ + char __pad_12[4]; }; @@ -6798,6 +6801,6 @@ union generic_reply struct set_keyboard_repeat_reply set_keyboard_repeat_reply; }; -#define SERVER_PROTOCOL_VERSION 863 +#define SERVER_PROTOCOL_VERSION 864 #endif /* __WINE_WINE_SERVER_PROTOCOL_H */ diff --git a/server/async.c b/server/async.c index d2d929c9709..c697a2e6a02 100644 --- a/server/async.c +++ b/server/async.c @@ -63,6 +63,7 @@ struct async unsigned int comp_flags; /* completion flags */ async_completion_callback completion_callback; /* callback to be called on completion */ void *completion_callback_private; /* argument to completion_callback */ + struct list canceled_entry; }; static void async_dump( struct object *obj, int verbose ); @@ -118,8 +119,8 @@ static void async_satisfied( struct object *obj, struct wait_queue_entry *entry struct async *async = (struct async *)obj; assert( obj->ops == &async_ops ); - /* we only return an async handle for asyncs created via create_request_async() */ - assert( async->iosb ); + if (!async->iosb) + return; if (async->direct_result) { @@ -574,7 +575,27 @@ int async_waiting( struct async_queue *queue ) return !async->terminated; } -static int cancel_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) +static int cancel_count_async( struct process *process, struct object *obj, struct thread *thread, + client_ptr_t iosb ) +{ + struct async *async; + int count = 0; + + LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) + { + if (async->terminated || async->canceled || async->is_system) continue; + if ((!obj || (get_fd_user( async->fd ) == obj)) && + (!thread || async->thread == thread) && + (!iosb || async->data.iosb == iosb)) + { + count++; + } + } + return count; +} + +static int cancel_async( struct process *process, struct object *obj, struct thread *thread, + client_ptr_t iosb, struct list *canceled_list ) { struct async *async; int woken = 0; @@ -592,7 +613,9 @@ restart: (!iosb || async->data.iosb == iosb)) { async->canceled = 1; + async->signaled = 0; fd_cancel_async( async->fd, async ); + list_add_tail( canceled_list, &async->canceled_entry ); woken++; goto restart; } @@ -815,12 +838,44 @@ DECL_HANDLER(cancel_async) { struct object *obj = get_handle_obj( current->process, req->handle, 0, NULL ); struct thread *thread = req->only_thread ? current : NULL; + struct list canceled_list = LIST_INIT(canceled_list); + unsigned int count = 0, capacity, i; + struct async *async, *next_async; + obj_handle_t *ptr; + + if (!obj) + return; + + count = cancel_count_async( current->process, obj, thread, req->iosb ); + if (!count && !thread) set_error( STATUS_NOT_FOUND ); + release_object( obj ); + capacity = get_reply_max_size() / sizeof(*ptr); - if (obj) + if (count) { - int count = cancel_async( current->process, obj, thread, req->iosb ); - if (!count && !thread) set_error( STATUS_NOT_FOUND ); - release_object( obj ); + if (count > capacity) + { + set_error( STATUS_BUFFER_OVERFLOW ); + reply->handles_size = count * sizeof(*ptr); + return; + } + count = cancel_async( current->process, obj, thread, req->iosb, &canceled_list ); + count = min( count, capacity ); + i = 0; + if ((ptr = set_reply_data_size( count * sizeof(*ptr) ))) + { + LIST_FOR_EACH_ENTRY_SAFE( async, next_async, &canceled_list, struct async, canceled_entry ) + { + if (i++ < capacity) + { + if (!async->wait_handle) + async->wait_handle = alloc_handle( current->process, async, SYNCHRONIZE, 0 ); + *ptr++ = async->wait_handle; + } + list_remove( &async->canceled_entry ); + } + reply->handles_size = count * sizeof(*ptr); + } } } diff --git a/server/protocol.def b/server/protocol.def index 63bb0111473..7cf330f2cf2 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2431,6 +2431,9 @@ enum message_type obj_handle_t handle; /* handle to comm port, socket or file */ client_ptr_t iosb; /* I/O status block (NULL=all) */ int only_thread; /* cancel matching this thread */ +(a)REPLY + data_size_t handles_size; + VARARG(handles,uints,handles_size); /* handles list */ @END diff --git a/server/request_handlers.h b/server/request_handlers.h index 2afc9707869..4652f1808d9 100644 --- a/server/request_handlers.h +++ b/server/request_handlers.h @@ -1386,6 +1386,8 @@ C_ASSERT( offsetof(struct cancel_async_request, handle) == 12 ); C_ASSERT( offsetof(struct cancel_async_request, iosb) == 16 ); C_ASSERT( offsetof(struct cancel_async_request, only_thread) == 24 ); C_ASSERT( sizeof(struct cancel_async_request) == 32 ); +C_ASSERT( offsetof(struct cancel_async_reply, handles_size) == 8 ); +C_ASSERT( sizeof(struct cancel_async_reply) == 16 ); C_ASSERT( offsetof(struct get_async_result_request, user_arg) == 16 ); C_ASSERT( sizeof(struct get_async_result_request) == 24 ); C_ASSERT( sizeof(struct get_async_result_reply) == 8 ); diff --git a/server/request_trace.h b/server/request_trace.h index bd961791024..91154a3ce87 100644 --- a/server/request_trace.h +++ b/server/request_trace.h @@ -1552,6 +1552,12 @@ static void dump_cancel_async_request( const struct cancel_async_request *req ) fprintf( stderr, ", only_thread=%d", req->only_thread ); } +static void dump_cancel_async_reply( const struct cancel_async_reply *req ) +{ + fprintf( stderr, " handles_size=%u", req->handles_size ); + dump_varargs_uints( ", handles=", min( cur_size, req->handles_size )); +} + static void dump_get_async_result_request( const struct get_async_result_request *req ) { dump_uint64( " user_arg=", &req->user_arg ); @@ -3792,7 +3798,7 @@ static const dump_func reply_dumpers[REQ_NB_REQUESTS] = NULL, NULL, NULL, - NULL, + (dump_func)dump_cancel_async_reply, (dump_func)dump_get_async_result_reply, (dump_func)dump_set_async_direct_result_reply, (dump_func)dump_read_reply, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/ntdll/tests/pipe.c | 63 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index cf98d555455..36bb1f1194c 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -569,13 +569,36 @@ static void test_nonalertable(void) CloseHandle(hPipe); } -static void test_cancelio(void) +struct cancelio_ctx { - IO_STATUS_BLOCK iosb; + HANDLE pipe; + HANDLE event; + IO_STATUS_BLOCK *iosb; + BOOL null_iosb; +}; + +static DWORD WINAPI cancelioex_thread_func(void *arg) +{ + struct cancelio_ctx *ctx = arg; IO_STATUS_BLOCK cancel_sb; - HANDLE hEvent; - HANDLE hPipe; - NTSTATUS res; + NTSTATUS res, status; + + res = pNtCancelIoFileEx(ctx->pipe, ctx->null_iosb ? NULL : ctx->iosb, &cancel_sb); + status = ctx->iosb->Status; + ok(!res, "NtCancelIoFileEx returned %lx\n", res); + + ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); + ok(WaitForSingleObject(ctx->event, 0) == 0, "hEvent not signaled\n"); + + return 0; +} + +static void test_cancelio(void) +{ + IO_STATUS_BLOCK cancel_sb, iosb; + HANDLE hEvent, hPipe, thread; + struct cancelio_ctx ctx; + NTSTATUS res, status; hEvent = CreateEventW(NULL, TRUE, FALSE, NULL); ok(hEvent != INVALID_HANDLE_VALUE, "can't create event, GetLastError: %lx\n", GetLastError()); @@ -589,9 +612,13 @@ static void test_cancelio(void) ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); res = pNtCancelIoFile(hPipe, &cancel_sb); + /* Save Status first thing after the call. We want to make very unlikely + * that the kernel APC updating Status could be executed after the call + * but before peeking at Status here. */ + status = iosb.Status; ok(!res, "NtCancelIoFile returned %lx\n", res); - ok(iosb.Status == STATUS_CANCELLED, "Wrong iostatus %lx\n", iosb.Status); + ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); ok(WaitForSingleObject(hEvent, 0) == 0, "hEvent not signaled\n"); ok(!ioapc_called, "IOAPC ran too early\n"); @@ -616,9 +643,10 @@ static void test_cancelio(void) ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); res = pNtCancelIoFileEx(hPipe, &iosb, &cancel_sb); + status = iosb.Status; ok(!res, "NtCancelIoFileEx returned %lx\n", res); - ok(iosb.Status == STATUS_CANCELLED, "Wrong iostatus %lx\n", iosb.Status); + ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); ok(WaitForSingleObject(hEvent, 0) == 0, "hEvent not signaled\n"); iosb.Status = 0xdeadbeef; @@ -626,6 +654,27 @@ static void test_cancelio(void) ok(res == STATUS_NOT_FOUND, "NtCancelIoFileEx returned %lx\n", res); ok(iosb.Status == 0xdeadbeef, "Wrong iostatus %lx\n", iosb.Status); + memset(&iosb, 0x55, sizeof(iosb)); + res = listen_pipe(hPipe, hEvent, &iosb, FALSE); + ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); + + ctx.pipe = hPipe; + ctx.event = hEvent; + ctx.iosb = &iosb; + ctx.null_iosb = FALSE; + thread = CreateThread(NULL, 0, cancelioex_thread_func, &ctx, 0, NULL); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + + memset(&iosb, 0x55, sizeof(iosb)); + res = listen_pipe(hPipe, hEvent, &iosb, FALSE); + ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); + + ctx.null_iosb = TRUE; + thread = CreateThread(NULL, 0, cancelioex_thread_func, &ctx, 0, NULL); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + CloseHandle(hPipe); } else -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> Also adjust xinput to handle that. --- dlls/hidclass.sys/device.c | 2 +- dlls/xinput1_3/main.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index cb65391e443..852c8ebfbff 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -122,7 +122,7 @@ void hid_queue_remove_pending_irps( struct hid_queue *queue ) while ((irp = hid_queue_pop_irp( queue ))) { - irp->IoStatus.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Status = STATUS_DEVICE_NOT_CONNECTED; IoCompleteRequest( irp, IO_NO_INCREMENT ); } } diff --git a/dlls/xinput1_3/main.c b/dlls/xinput1_3/main.c index 28f57e93bce..379a1d2f0b8 100644 --- a/dlls/xinput1_3/main.c +++ b/dlls/xinput1_3/main.c @@ -573,7 +573,11 @@ static void read_controller_state(struct xinput_controller *controller) if (!GetOverlappedResult(controller->device, &controller->hid.read_ovl, &read_len, TRUE)) { if (GetLastError() == ERROR_OPERATION_ABORTED) return; - if (GetLastError() == ERROR_ACCESS_DENIED || GetLastError() == ERROR_INVALID_HANDLE) controller_destroy(controller, TRUE); + if (GetLastError() == ERROR_ACCESS_DENIED || GetLastError() == ERROR_INVALID_HANDLE || + GetLastError() == ERROR_DEVICE_NOT_CONNECTED) + { + controller_destroy(controller, TRUE); + } else ERR("Failed to read input report, GetOverlappedResult failed with error %lu\n", GetLastError()); return; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/file.c:
} SERVER_END_REQ; + if (status != STATUS_BUFFER_OVERFLOW) + break; + } + + if (handles_size) + { + for (i = 0; i < handles_size / sizeof(*cancelled_handles); ++i) + { + select_op.wait.op = SELECT_WAIT; + select_op.wait.handles[0] = cancelled_handles[i]; + + server_select( &select_op, offsetof(union select_op, wait.handles[1]), + SELECT_INTERRUPTIBLE, TIMEOUT_INFINITE, NULL, NULL ); + }
Any reason not to use SELECT_WAIT_ALL instead of a loop? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_100761
On Mon Apr 14 14:36:39 2025 +0000, Rémi Bernon wrote:
Any reason not to use SELECT_WAIT_ALL instead of a loop? I don't think so, not sure why I didn't do that...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_100798
On Mon Apr 14 15:35:25 2025 +0000, Matteo Bruni wrote:
I don't think so, not sure why I didn't do that... Fwiw this could probably use NtWaitForMultipleObjects too.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_100802
On Mon Apr 14 16:55:38 2025 +0000, Rémi Bernon wrote:
Fwiw this could probably use NtWaitForMultipleObjects too. Or alternatively maybe, we could perhaps introduce a new select op that cancels and wait for objects? I don't know if that would be a good idea, it would be a bit specific to this async use case, but it would save some round-trip and avoid having to collect the handles.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_100803
Fwiw this could probably use NtWaitForMultipleObjects too.
It can, although `NtWaitForMultipleObjects` takes `HANDLE`s but we get `obj_handle_t`s from the server, and doing the (trivial) conversion looks a bit awkward.
Or alternatively maybe, we could perhaps introduce a new select op that cancels and wait for objects?
That would avoid that particular issue as well, I guess. Not sure how much use it would get but I'm going to give it a go and see how it looks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_100943
Alex Henrie (@alexhenrie) commented about dlls/ntdll/tests/pipe.c:
ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res);
res = pNtCancelIoFile(hPipe, &cancel_sb); + /* Save Status first thing after the call. We want to make very unlikely + * that the kernel APC updating Status could be executed after the call + * but before peeking at Status here. */ + status = iosb.Status;
If we want to be sure that `status` is saved immediately, should we make it volatile? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_101201
On Wed Apr 16 21:33:06 2025 +0000, Alex Henrie wrote:
If we want to be sure that `status` is saved immediately, should we make it volatile? I guess we could, although in practice it doesn't seem necessary. The following ok() generates a couple of function calls (the first one being to `winetest_get_thread_data()`) which basically force sequencing already.
In general, the idea here is to make sure that the work related to the first `ok()` call doesn't allow the second `ok()` to succeed, which could happen if e.g. one runs the test with `WINETEST_REPORT_SUCCESS=1`, since in that case there's a syscall involved and thus an opportunity for the APC to execute. An alternative is (and my original solution was) to swap the two `ok()` calls instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_101209
Or alternatively maybe, we could perhaps introduce a new select op that cancels and wait for objects?
That would avoid that particular issue as well, I guess. Not sure how much use it would get but I'm going to give it a go and see how it looks.
This was looking pretty good, except I think it doesn't quite work in general: `server_select()` restarts the `select` server call when the original one is interrupted by a `KERNEL_APC`. Depending on timing, we could get: 1. The initial `select` server call cancels an async 2. The initial `select` server call is interrupted by the KERNEL_APC triggered by the cancellation 3. `server_select()` restarts the `select` server call 4. the second instance of `select` doesn't find any pending async, returning `STATUS_NOT_FOUND` and that sometimes breaks one of the existing pipe tests. I'm sure we could sidestep this issue in some way (worst case by partially duplicating / reimplementing `server_select()`), but this probably suggests that we don't want to take this route. Unless I'm missing something else (certainly possible). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_101439
participants (4)
-
Alex Henrie (＠alexhenrie) -
Matteo Bruni
-
Matteo Bruni (＠Mystral)
-
Rémi Bernon