[PATCH v3 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.
-- v3: hidclass: Set Status for pending IRPs of removed devices to STATUS_DEVICE_NOT_CONNECTED. ntdll/tests: Add more NtCancelIoFile[Ex]() tests. ntdll: Wait for all pending operations to be cancelled in NtCancelIoFile[Ex]().
From: Matteo Bruni mbruni@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 d7ab8b74734..679c2c44332 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 ); }
From: Matteo Bruni mbruni@codeweavers.com
--- dlls/ntdll/unix/file.c | 46 ++++++++++++++++++----- 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, 115 insertions(+), 18 deletions(-)
diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 679c2c44332..b5b34b59b3b 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -6424,20 +6424,48 @@ 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, count = 8, size; + obj_handle_t *cancelled_handles; + HANDLE *handles; + int i;
- 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, count * sizeof(*handles) ); + if (!(status = wine_server_call( req ))) + { + io_status->Status = status; + io_status->Information = 0; + } + 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 (!status) + { + handles = (HANDLE *)cancelled_handles; + for (i = count - 1; i >= 0; i--) + handles[i] = wine_server_ptr_handle( cancelled_handles[i] ); + NtWaitForMultipleObjects( count, handles, FALSE, TRUE, 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 */ +@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,
From: Matteo Bruni mbruni@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
From: Matteo Bruni mbruni@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; }
On Tue Apr 29 14:39:42 2025 +0000, Rémi Bernon wrote:
How about allocating and using only one array of HANDLE instead of having to handle allocation failures twice? sizeof(HANDLE) >= sizeof(obj_handle_t) so you can fill it with obj_handle_t and fix them up before waiting by moving them inline backwards.
I had thought of something along those lines but I was unsure if it was appropriate. Done now :slight_smile:
Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/file.c:
static NTSTATUS cancel_io( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK *io_status, BOOL only_thread ) {
- unsigned int status;
- unsigned int status, count = 8, size;
- obj_handle_t *cancelled_handles;
- HANDLE *handles;
- int i;
- for (;;)
- {
size = count * sizeof(*cancelled_handles);
```suggestion:-0+0 size = count * sizeof(*handles); ```
Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/file.c:
HANDLE *handles;
int i;
for (;;)
{
size = count * sizeof(*cancelled_handles);cancelled_handles = malloc( size );if (!cancelled_handles)return STATUS_NO_MEMORY; SERVER_START_REQ( cancel_async ) { 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, count * sizeof(*handles) );
```suggestion:-0+0 wine_server_set_reply( req, cancelled_handles, count * sizeof(*cancelled_handles) ); ```
Rémi Bernon (@rbernon) commented about dlls/ntdll/unix/file.c:
SERVER_END_REQ;
if (status != STATUS_BUFFER_OVERFLOW)break;}
if (!status)
{
handles = (HANDLE *)cancelled_handles;for (i = count - 1; i >= 0; i--)handles[i] = wine_server_ptr_handle( cancelled_handles[i] );NtWaitForMultipleObjects( count, handles, FALSE, TRUE, NULL );free( cancelled_handles );}
return status;
}
But I would also rather do it like this, to reduce the scope of the pointer aliasing, and with some additional fixups:
```suggestion:-45+0 { unsigned int status, count = 8; HANDLE *handles; int i;
do { if (!(handles = malloc( count * sizeof(*handles) ))) return STATUS_NO_MEMORY;
SERVER_START_REQ( cancel_async ) { object_handle_t *server_handles = (object_handle_t *)handles;
req->handle = wine_server_obj_handle( handle ); req->iosb = wine_server_client_ptr( io ); req->only_thread = only_thread; wine_server_set_reply( req, server_handles, count * sizeof(*server_handles) ); if (!(status = wine_server_call( req ))) { for (i = count - 1; i >= 0; i--) handles[i] = wine_server_ptr_handle( server_handles[i] ); io_status->Status = status; io_status->Information = 0; } else if (status == STATUS_BUFFER_OVERFLOW) { count = reply->handles_size / sizeof(*server_handles); free( handles ); } } SERVER_END_REQ; } while (status == STATUS_BUFFER_OVERFLOW);
if (!status) NtWaitForMultipleObjects( count, handles, FALSE, TRUE, NULL ); free( handles ); return status; } ```
On Tue Apr 29 15:04:19 2025 +0000, Rémi Bernon wrote:
But I would also rather do it like this, to reduce the scope of the pointer aliasing, and with some additional fixups:
{ unsigned int status, count = 8; HANDLE *handles; int i; do { if (!(handles = malloc( count * sizeof(*handles) ))) return STATUS_NO_MEMORY; SERVER_START_REQ( cancel_async ) { object_handle_t *server_handles = (object_handle_t *)handles; req->handle = wine_server_obj_handle( handle ); req->iosb = wine_server_client_ptr( io ); req->only_thread = only_thread; wine_server_set_reply( req, server_handles, count * sizeof(*server_handles) ); if (!(status = wine_server_call( req ))) { for (i = count - 1; i >= 0; i--) handles[i] = wine_server_ptr_handle( server_handles[i] ); io_status->Status = status; io_status->Information = 0; } else if (status == STATUS_BUFFER_OVERFLOW) { count = reply->handles_size / sizeof(*server_handles); free( handles ); } } SERVER_END_REQ; } while (status == STATUS_BUFFER_OVERFLOW); if (!status) NtWaitForMultipleObjects( count, handles, FALSE, TRUE, NULL ); free( handles ); return status; }
Thanks a lot, sorry that I messed up the two arrays in the current version...
There's a subtle issue with this: `count` needs to be updated on success as well, otherwise we wait on garbage handles.
There are a few alternatives to tackle that, unsure which one you'd prefer: - duplicate the `count` update in both `if` branches - move the `count` update back after the branch and the handle type conversion out of the loop (thus also moving back `server_handles` to the outer scope) - move the `count` update back after the branch and put both the handle conversion and the actual wait inside the loop, after the `SERVER_END_REQ` (i.e. `server_handles` goes inside the `do`)
The last option above is kinda an in-between and what I tried, it doesn't look too terrible but I don't know that it's particularly preferable.
On Tue Apr 29 15:46:58 2025 +0000, Matteo Bruni wrote:
Thanks a lot, sorry that I messed up the two arrays in the current version... There's a subtle issue with this: `count` needs to be updated on success as well, otherwise we wait on garbage handles. There are a few alternatives to tackle that, unsure which one you'd prefer:
- duplicate the `count` update in both `if` branches
- move the `count` update back after the branch and the handle type
conversion out of the loop (thus also moving back `server_handles` to the outer scope)
- move the `count` update back after the branch and put both the handle
conversion and the actual wait inside the loop, after the `SERVER_END_REQ` (i.e. `server_handles` goes inside the `do`) The last option above is kinda an in-between and what I tried, it doesn't look too terrible but I don't know that it's particularly preferable.
Ah right, duplicating it before the for loop that converts handles looks alright.
-
Matteo Bruni -
Matteo Bruni (＠Mystral)
-
Rémi Bernon (＠rbernon)