[PATCH v10 0/8] 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. -- v10: hidclass: Set Status for pending IRPs of removed devices to STATUS_DEVICE_NOT_CONNECTED. ntdll/tests: Test IOSB values of the cancel operation. ntdll/tests: Add more NtCancelIoFile[Ex]() tests. ntdll: Wait for all asyncs to handle cancel in NtCancelIoFile[Ex](). server: Introduce a separate server object for the async cancel operation. server: Keep cancelled async in a separate list. server: Introduce a find_async_from_user helper. ntdll: Factor out a cancel_io() function. 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 4b9d063ff3d..a43d464403d 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -6493,19 +6493,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; @@ -6518,28 +6515,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: Rémi Bernon <rbernon(a)codeweavers.com> --- server/async.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/server/async.c b/server/async.c index 1ed241dcb65..6ab9ede4a0d 100644 --- a/server/async.c +++ b/server/async.c @@ -575,6 +575,16 @@ int async_waiting( struct async_queue *queue ) return !async->terminated; } +static struct async *find_async_from_user( struct process *process, client_ptr_t user ) +{ + struct async *async; + + LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) + if (async->data.user == user) return async; + + return NULL; +} + static int cancel_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) { struct async *async; @@ -829,17 +839,10 @@ DECL_HANDLER(cancel_async) /* get async result from associated iosb */ DECL_HANDLER(get_async_result) { - struct iosb *iosb = NULL; struct async *async; + struct iosb *iosb; - LIST_FOR_EACH_ENTRY( async, ¤t->process->asyncs, struct async, process_entry ) - if (async->data.user == req->user_arg) - { - iosb = async->iosb; - break; - } - - if (!iosb) + if (!(async = find_async_from_user( current->process, req->user_arg )) || !(iosb = async->iosb)) { set_error( STATUS_INVALID_PARAMETER ); return; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Rémi Bernon <rbernon(a)codeweavers.com> --- server/async.c | 44 ++++++++++++++++++++++---------------------- server/file.h | 2 +- server/process.c | 4 +++- server/process.h | 1 + 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/server/async.c b/server/async.c index 6ab9ede4a0d..20f906c5d78 100644 --- a/server/async.c +++ b/server/async.c @@ -54,7 +54,6 @@ struct async unsigned int direct_result :1;/* a flag if we're passing result directly from request instead of APC */ unsigned int alerted :1; /* fd is signaled, but we are waiting for client-side I/O */ unsigned int terminated :1; /* async has been terminated */ - unsigned int canceled :1; /* have we already queued cancellation for this async? */ unsigned int unknown_status :1; /* initial status is not known yet */ unsigned int blocking :1; /* async is blocking */ unsigned int is_system :1; /* background system operation not affecting userspace visible state. */ @@ -276,7 +275,6 @@ struct async *create_async( struct fd *fd, struct thread *thread, const struct a async->direct_result = 0; async->alerted = 0; async->terminated = 0; - async->canceled = 0; async->unknown_status = 0; async->blocking = !is_fd_overlapped( fd ); async->is_system = 0; @@ -575,17 +573,26 @@ int async_waiting( struct async_queue *queue ) return !async->terminated; } +static void cancel_async( struct process *process, struct async *async ) +{ + list_remove( &async->process_entry ); + list_add_tail( &process->cancelled_asyncs, &async->process_entry ); + fd_cancel_async( async->fd, async ); +} + static struct async *find_async_from_user( struct process *process, client_ptr_t user ) { struct async *async; + LIST_FOR_EACH_ENTRY( async, &process->cancelled_asyncs, struct async, process_entry ) + if (async->data.user == user) return async; LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) if (async->data.user == user) return async; return NULL; } -static int cancel_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) +static int cancel_process_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) { struct async *async; int woken = 0; @@ -597,13 +604,12 @@ static int cancel_async( struct process *process, struct object *obj, struct thr restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { - if (async->terminated || async->canceled || async->is_system) continue; + if (async->terminated || async->is_system) continue; if ((!obj || (get_fd_user( async->fd ) == obj)) && (!thread || async->thread == thread) && (!iosb || async->data.iosb == iosb)) { - async->canceled = 1; - fd_cancel_async( async->fd, async ); + cancel_async( process, async ); woken++; goto restart; } @@ -619,12 +625,11 @@ static int cancel_blocking( struct process *process, struct thread *thread, clie restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { - if (async->terminated || async->canceled) continue; + if (async->terminated) continue; if (async->blocking && async->thread == thread && (!iosb || async->data.iosb == iosb)) { - async->canceled = 1; - fd_cancel_async( async->fd, async ); + cancel_async( process, async ); woken++; goto restart; } @@ -632,16 +637,15 @@ restart: return woken; } -void cancel_process_asyncs( struct process *process ) +void cancel_terminating_process_asyncs( struct process *process ) { struct async *async; restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { - if (async->terminated || async->canceled) continue; - async->canceled = 1; - fd_cancel_async( async->fd, async ); + if (async->terminated) continue; + cancel_async( process, async ); goto restart; } } @@ -658,11 +662,9 @@ int async_close_obj_handle( struct object *obj, struct process *process, obj_han 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->terminated || 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 ); + cancel_async( process, async ); goto restart; } return 1; @@ -675,12 +677,10 @@ void cancel_terminating_thread_asyncs( struct thread *thread ) restart: LIST_FOR_EACH_ENTRY( async, &thread->process->asyncs, struct async, process_entry ) { - if (async->thread != thread || async->terminated || async->canceled) continue; + if (async->thread != thread || async->terminated) continue; if (async->completion && async->data.apc_context && !async->event) continue; if (async->is_system) continue; - - async->canceled = 1; - fd_cancel_async( async->fd, async ); + cancel_async( thread->process, async ); goto restart; } } @@ -830,7 +830,7 @@ DECL_HANDLER(cancel_async) if (obj) { - int count = cancel_async( current->process, obj, thread, req->iosb ); + int count = cancel_process_async( current->process, obj, thread, req->iosb ); if (!count && !thread) set_error( STATUS_NOT_FOUND ); release_object( obj ); } diff --git a/server/file.h b/server/file.h index 567194bf00a..7f2d0185bd4 100644 --- a/server/file.h +++ b/server/file.h @@ -278,7 +278,7 @@ extern void fd_copy_completion( struct fd *src, struct fd *dst ); 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_process_asyncs( struct process *process ); extern void cancel_terminating_thread_asyncs( struct thread *thread ); extern int async_close_obj_handle( struct object *obj, struct process *process, obj_handle_t handle ); diff --git a/server/process.c b/server/process.c index 9b3b0b8fc6c..268744ef2cf 100644 --- a/server/process.c +++ b/server/process.c @@ -705,6 +705,7 @@ struct process *create_process( int fd, struct process *parent, unsigned int fla list_init( &process->thread_list ); list_init( &process->locks ); list_init( &process->asyncs ); + list_init( &process->cancelled_asyncs ); list_init( &process->classes ); list_init( &process->views ); @@ -780,6 +781,7 @@ static void process_destroy( struct object *obj ) /* we can't have a thread remaining */ assert( list_empty( &process->thread_list )); assert( list_empty( &process->asyncs )); + assert( list_empty( &process->cancelled_asyncs )); assert( !process->sigkill_timeout ); /* timeout should hold a reference to the process */ @@ -986,7 +988,7 @@ static void process_killed( struct process *process ) close_process_desktop( process ); process->winstation = 0; process->desktop = 0; - cancel_process_asyncs( process ); + cancel_terminating_process_asyncs( process ); close_process_handles( process ); if (process->idle_event) release_object( process->idle_event ); process->idle_event = NULL; diff --git a/server/process.h b/server/process.h index d9b78004afa..0e01fa9eafb 100644 --- a/server/process.h +++ b/server/process.h @@ -67,6 +67,7 @@ struct process struct job *job; /* job object associated with this process */ struct list job_entry; /* list entry for job object */ struct list asyncs; /* list of async object owned by the process */ + struct list cancelled_asyncs;/* list of async object owned by the process */ struct list locks; /* list of file locks owned by the process */ struct list classes; /* window classes owned by the process */ struct console *console; /* console input */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- server/async.c | 162 +++++++++++++++++++++++++++++++++++++++++------ server/process.c | 2 + server/process.h | 3 +- 3 files changed, 147 insertions(+), 20 deletions(-) diff --git a/server/async.c b/server/async.c index 20f906c5d78..43b62343037 100644 --- a/server/async.c +++ b/server/async.c @@ -34,12 +34,88 @@ #include "process.h" #include "handle.h" +struct async_cancel +{ + struct object obj; /* object header */ + struct event_sync *sync; /* sync object for wait/signal */ + struct list asyncs; /* list of asyncs in the cancel group */ + struct list entry; /* entry in process client_cancels list */ +}; + +static void async_cancel_dump( struct object *obj, int verbose ); +static struct object *async_cancel_get_sync( struct object *obj ); +static void async_cancel_destroy( struct object *obj ); + +static const struct object_ops async_cancel_ops = +{ + sizeof(struct async_cancel), /* size */ + &no_type, /* type */ + async_cancel_dump, /* dump */ + NULL, /* add_queue */ + NULL, /* remove_queue */ + NULL, /* signaled */ + NULL, /* satisfied */ + no_signal, /* signal */ + no_get_fd, /* get_fd */ + async_cancel_get_sync, /* get_sync */ + default_map_access, /* map_access */ + default_get_sd, /* get_sd */ + default_set_sd, /* set_sd */ + no_get_full_name, /* get_full_name */ + no_lookup_name, /* lookup_name */ + no_link_name, /* link_name */ + NULL, /* unlink_name */ + no_open_file, /* open_file */ + no_kernel_obj_list, /* get_kernel_obj_list */ + no_close_handle, /* close_handle */ + async_cancel_destroy /* destroy */ +}; + +static void async_cancel_dump( struct object *obj, int verbose ) +{ + struct async_cancel *cancel = (struct async_cancel *)obj; + assert( obj->ops == &async_cancel_ops ); + fprintf( stderr, "async_cancel %p\n", cancel ); +} + +static struct object *async_cancel_get_sync( struct object *obj ) +{ + struct async_cancel *cancel = (struct async_cancel *)obj; + + assert( obj->ops == &async_cancel_ops ); + return grab_object( cancel->sync ); +} + +static void async_cancel_destroy( struct object *obj ) +{ + struct async_cancel *cancel = (struct async_cancel *)obj; + + assert( obj->ops == &async_cancel_ops ); + if (cancel->sync) release_object( cancel->sync ); +} + +static struct async_cancel *create_async_cancel(void) +{ + struct async_cancel *cancel; + + if (!(cancel = alloc_object( &async_cancel_ops ))) return NULL; + cancel->sync = NULL; + list_init( &cancel->asyncs ); + + if (!(cancel->sync = create_event_sync( 1, 0 ))) + { + release_object( cancel ); + return NULL; + } + return cancel; +} + struct async { struct object obj; /* object header */ struct thread *thread; /* owning thread */ struct list queue_entry; /* entry in async queue list */ - struct list process_entry; /* entry in process list */ + struct list process_entry; /* entry in process / cancel list */ struct async_queue *queue; /* queue containing this async */ struct fd *fd; /* fd associated with an unqueued async */ struct timeout_user *timeout; @@ -62,6 +138,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 async_cancel *async_cancel; /* cancel object if async is being cancelled */ }; static void async_dump( struct object *obj, int verbose ); @@ -145,6 +222,7 @@ static void async_destroy( struct object *obj ) struct async *async = (struct async *)obj; assert( obj->ops == &async_ops ); + assert( !async->async_cancel ); list_remove( &async->process_entry ); if (async->queue) @@ -282,6 +360,7 @@ struct async *create_async( struct fd *fd, struct thread *thread, const struct a async->comp_flags = 0; async->completion_callback = NULL; async->completion_callback_private = NULL; + async->async_cancel = NULL; if (iosb) async->iosb = (struct iosb *)grab_object( iosb ); else async->iosb = NULL; @@ -483,6 +562,23 @@ static void add_async_completion( struct async *async, apc_param_t cvalue, unsig if (async->completion) add_completion( async->completion, async->comp_key, cvalue, status, information ); } +static void async_complete_cancel( struct async *async ) +{ + struct async_cancel *cancel; + + if (!(cancel = async->async_cancel)) return; + async->async_cancel = NULL; + list_remove( &async->process_entry ); + list_init( &async->process_entry ); + + if (list_empty( &cancel->asyncs )) + { + signal_sync( cancel->sync ); + list_remove( &cancel->entry ); + release_object( cancel ); + } +} + /* store the result of the client-side async callback */ void async_set_result( struct object *obj, unsigned int status, apc_param_t total ) { @@ -540,6 +636,7 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota } async_call_completion_callback( async ); + async_complete_cancel( async ); if (async->queue) { @@ -573,17 +670,21 @@ int async_waiting( struct async_queue *queue ) return !async->terminated; } -static void cancel_async( struct process *process, struct async *async ) +static void cancel_async( struct list *cancelled_asyncs, struct async *async ) { list_remove( &async->process_entry ); - list_add_tail( &process->cancelled_asyncs, &async->process_entry ); + list_add_tail( cancelled_asyncs, &async->process_entry ); fd_cancel_async( async->fd, async ); } static struct async *find_async_from_user( struct process *process, client_ptr_t user ) { + struct async_cancel *cancel; struct async *async; + LIST_FOR_EACH_ENTRY( cancel, &process->client_cancels, struct async_cancel, entry ) + LIST_FOR_EACH_ENTRY( async, &cancel->asyncs, struct async, process_entry ) + if (async->data.user == user) return async; LIST_FOR_EACH_ENTRY( async, &process->cancelled_asyncs, struct async, process_entry ) if (async->data.user == user) return async; LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) @@ -592,15 +693,16 @@ static struct async *find_async_from_user( struct process *process, client_ptr_t return NULL; } -static int cancel_process_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) +static obj_handle_t cancel_process_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) { + struct async_cancel *cancel; + obj_handle_t handle = 0; struct async *async; - int woken = 0; - /* FIXME: it would probably be nice to replace the "canceled" flag with a - * single LIST_FOR_EACH_ENTRY_SAFE, but currently cancelling an async can - * cause other asyncs to be removed via async_reselect() */ + if (!(cancel = create_async_cancel())) return 0; + /* Cancelling an async can cause other asyncs to be removed via + * async_reselect() */ restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { @@ -609,12 +711,19 @@ restart: (!thread || async->thread == thread) && (!iosb || async->data.iosb == iosb)) { - cancel_async( process, async ); - woken++; + cancel_async( &cancel->asyncs, async ); + async->async_cancel = cancel; goto restart; } } - return woken; + + if (list_empty( &cancel->asyncs )) release_object( cancel ); + else + { + handle = alloc_handle( process, cancel, SYNCHRONIZE, 0 ); + list_add_tail( &process->client_cancels, &cancel->entry ); + } + return handle; } static int cancel_blocking( struct process *process, struct thread *thread, client_ptr_t iosb ) @@ -629,7 +738,7 @@ restart: if (async->blocking && async->thread == thread && (!iosb || async->data.iosb == iosb)) { - cancel_async( process, async ); + cancel_async( &process->cancelled_asyncs, async ); woken++; goto restart; } @@ -639,15 +748,29 @@ restart: void cancel_terminating_process_asyncs( struct process *process ) { - struct async *async; + struct async_cancel *cancel, *next_cancel; + struct async *async, *next_async; restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { if (async->terminated) continue; - cancel_async( process, async ); + cancel_async( &process->cancelled_asyncs, async ); goto restart; } + + LIST_FOR_EACH_ENTRY_SAFE( cancel, next_cancel, &process->client_cancels, struct async_cancel, entry ) + { + LIST_FOR_EACH_ENTRY_SAFE( async, next_async, &cancel->asyncs, struct async, process_entry ) + { + async->async_cancel = NULL; + list_remove( &async->process_entry ); + list_init( &async->process_entry ); + } + + list_remove( &cancel->entry ); + release_object( cancel ); + } } int async_close_obj_handle( struct object *obj, struct process *process, obj_handle_t handle ) @@ -664,7 +787,7 @@ restart: { if (async->terminated || get_fd_user( async->fd ) != obj) continue; if (!async->completion || !async->data.apc_context || async->event) continue; - cancel_async( process, async ); + cancel_async( &process->cancelled_asyncs, async ); goto restart; } return 1; @@ -672,15 +795,16 @@ restart: void cancel_terminating_thread_asyncs( struct thread *thread ) { + struct process *process = thread->process; struct async *async; restart: - LIST_FOR_EACH_ENTRY( async, &thread->process->asyncs, struct async, process_entry ) + LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { if (async->thread != thread || async->terminated) continue; if (async->completion && async->data.apc_context && !async->event) continue; if (async->is_system) continue; - cancel_async( thread->process, async ); + cancel_async( &process->cancelled_asyncs, async ); goto restart; } } @@ -830,8 +954,8 @@ DECL_HANDLER(cancel_async) if (obj) { - int count = cancel_process_async( current->process, obj, thread, req->iosb ); - if (!count && !thread) set_error( STATUS_NOT_FOUND ); + obj_handle_t wait_handle = cancel_process_async( current->process, obj, thread, req->iosb ); + if (!wait_handle && !thread) set_error( STATUS_NOT_FOUND ); release_object( obj ); } } diff --git a/server/process.c b/server/process.c index 268744ef2cf..45e74cac9c0 100644 --- a/server/process.c +++ b/server/process.c @@ -706,6 +706,7 @@ struct process *create_process( int fd, struct process *parent, unsigned int fla list_init( &process->locks ); list_init( &process->asyncs ); list_init( &process->cancelled_asyncs ); + list_init( &process->client_cancels ); list_init( &process->classes ); list_init( &process->views ); @@ -782,6 +783,7 @@ static void process_destroy( struct object *obj ) assert( list_empty( &process->thread_list )); assert( list_empty( &process->asyncs )); assert( list_empty( &process->cancelled_asyncs )); + assert( list_empty( &process->client_cancels )); assert( !process->sigkill_timeout ); /* timeout should hold a reference to the process */ diff --git a/server/process.h b/server/process.h index 0e01fa9eafb..7d5d9538677 100644 --- a/server/process.h +++ b/server/process.h @@ -67,7 +67,8 @@ struct process struct job *job; /* job object associated with this process */ struct list job_entry; /* list entry for job object */ struct list asyncs; /* list of async object owned by the process */ - struct list cancelled_asyncs;/* list of async object owned by the process */ + struct list cancelled_asyncs;/* list of server-cancelled async objects */ + struct list client_cancels; /* list of client async_cancel objects */ struct list locks; /* list of file locks owned by the process */ struct list classes; /* window classes owned by the process */ struct console *console; /* console input */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/ntdll/unix/file.c | 23 +++++++++++++++++++---- include/wine/server_protocol.h | 4 +++- server/async.c | 1 + server/protocol.def | 2 ++ server/request_handlers.h | 2 ++ server/request_trace.h | 7 ++++++- 6 files changed, 33 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index a43d464403d..0de0b838410 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -6496,6 +6496,7 @@ 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 ) { + HANDLE cancel_handle; unsigned int status; SERVER_START_REQ( cancel_async ) @@ -6504,13 +6505,27 @@ static NTSTATUS cancel_io( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK * req->iosb = wine_server_client_ptr( io ); req->only_thread = only_thread; if (!(status = wine_server_call( req ))) - { - io_status->Status = status; - io_status->Information = 0; - } + cancel_handle = wine_server_ptr_handle( reply->cancel_handle ); } SERVER_END_REQ; + if (!status && cancel_handle) + { + NtWaitForMultipleObjects( 1, &cancel_handle, FALSE, TRUE, NULL ); + NtClose( cancel_handle ); + } + else if (status == STATUS_INVALID_HANDLE) + { + return status; + } + + /* TODO: Add a test for the timing of setting IOSB values? It's tricky + * since we probably have to check from a different thread without using + * win32 IPC for synchronization to avoid syncing on system APC + * execution. */ + io_status->Status = status; + io_status->Information = 0; + return status; } diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index 1f9348e9e86..08ef0a3900c 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -3280,6 +3280,8 @@ struct cancel_async_request struct cancel_async_reply { struct reply_header __header; + obj_handle_t cancel_handle; + char __pad_12[4]; }; @@ -6846,6 +6848,6 @@ union generic_reply struct set_keyboard_repeat_reply set_keyboard_repeat_reply; }; -#define SERVER_PROTOCOL_VERSION 880 +#define SERVER_PROTOCOL_VERSION 881 #endif /* __WINE_WINE_SERVER_PROTOCOL_H */ diff --git a/server/async.c b/server/async.c index 43b62343037..d323d7a1ad8 100644 --- a/server/async.c +++ b/server/async.c @@ -956,6 +956,7 @@ DECL_HANDLER(cancel_async) { obj_handle_t wait_handle = cancel_process_async( current->process, obj, thread, req->iosb ); if (!wait_handle && !thread) set_error( STATUS_NOT_FOUND ); + else reply->cancel_handle = wait_handle; release_object( obj ); } } diff --git a/server/protocol.def b/server/protocol.def index 22470e33ae0..4bd40a90c64 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2461,6 +2461,8 @@ 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 + obj_handle_t cancel_handle; @END diff --git a/server/request_handlers.h b/server/request_handlers.h index f9e5423ff40..001b9d6ff25 100644 --- a/server/request_handlers.h +++ b/server/request_handlers.h @@ -1400,6 +1400,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, cancel_handle) == 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 8824eab2d09..6df8b962c64 100644 --- a/server/request_trace.h +++ b/server/request_trace.h @@ -1574,6 +1574,11 @@ 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, " cancel_handle=%04x", req->cancel_handle ); +} + static void dump_get_async_result_request( const struct get_async_result_request *req ) { dump_uint64( " user_arg=", &req->user_arg ); @@ -3816,7 +3821,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 14a6148a2c5..61d1aba1edf 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> --- dlls/ntdll/tests/pipe.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index 61d1aba1edf..500feb02820 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -589,6 +589,8 @@ static DWORD WINAPI cancelioex_thread_func(void *arg) ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); ok(WaitForSingleObject(ctx->event, 0) == 0, "hEvent not signaled\n"); + ok(!cancel_sb.Status, "Unexpected Status %lx\n", cancel_sb.Status); + ok(!cancel_sb.Information, "Unexpected Information %Iu\n", cancel_sb.Information); return 0; } @@ -617,6 +619,8 @@ static void test_cancelio(void) * but before peeking at Status here. */ status = iosb.Status; ok(!res, "NtCancelIoFile returned %lx\n", res); + ok(!cancel_sb.Status, "Unexpected Status %lx\n", cancel_sb.Status); + ok(!cancel_sb.Information, "Unexpected Information %Iu\n", cancel_sb.Information); ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); ok(WaitForSingleObject(hEvent, 0) == 0, "hEvent not signaled\n"); @@ -630,6 +634,8 @@ static void test_cancelio(void) res = pNtCancelIoFile(hPipe, &cancel_sb); ok(!res, "NtCancelIoFile returned %lx\n", res); ok(iosb.Status == STATUS_CANCELLED, "Wrong iostatus %lx\n", iosb.Status); + ok(!cancel_sb.Status, "Unexpected Status %lx\n", cancel_sb.Status); + ok(!cancel_sb.Information, "Unexpected Information %Iu\n", cancel_sb.Information); CloseHandle(hPipe); @@ -645,6 +651,8 @@ static void test_cancelio(void) res = pNtCancelIoFileEx(hPipe, &iosb, &cancel_sb); status = iosb.Status; ok(!res, "NtCancelIoFileEx returned %lx\n", res); + ok(!cancel_sb.Status, "Unexpected Status %lx\n", cancel_sb.Status); + ok(!cancel_sb.Information, "Unexpected Information %Iu\n", cancel_sb.Information); ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); ok(WaitForSingleObject(hEvent, 0) == 0, "hEvent not signaled\n"); @@ -653,6 +661,8 @@ static void test_cancelio(void) res = pNtCancelIoFileEx(hPipe, NULL, &cancel_sb); ok(res == STATUS_NOT_FOUND, "NtCancelIoFileEx returned %lx\n", res); ok(iosb.Status == 0xdeadbeef, "Wrong iostatus %lx\n", iosb.Status); + ok(cancel_sb.Status == STATUS_NOT_FOUND, "Unexpected Status %lx\n", cancel_sb.Status); + ok(!cancel_sb.Information, "Unexpected Information %Iu\n", cancel_sb.Information); memset(&iosb, 0x55, sizeof(iosb)); res = listen_pipe(hPipe, hEvent, &iosb, FALSE); @@ -739,6 +749,8 @@ static void test_cancelsynchronousio(void) ok(ret == WAIT_TIMEOUT, "WaitForSingleObject returned %lu (error %lu)\n", ret, GetLastError()); memset(&iosb, 0x55, sizeof(iosb)); res = pNtCancelSynchronousIoFile(thread, NULL, &iosb); + ok(ctx.iosb.Status == 0xdeadbabe, "Unexpected Status %lx\n", ctx.iosb.Status); + ok(ctx.iosb.Information == 0xdeadbeef, "Unexpected Information %Iu\n", ctx.iosb.Information); ok(res == STATUS_SUCCESS, "Failed to cancel I/O\n"); ok(iosb.Status == STATUS_SUCCESS, "iosb.Status got changed to %lx\n", iosb.Status); ok(iosb.Information == 0, "iosb.Information got changed to %Iu\n", iosb.Information); -- 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 c2539483398..623e92af187 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
On Wed Aug 6 18:54:42 2025 +0000, Rémi Bernon wrote:
This looks mostly good to me but I think there's still some things that can be improved: * Cancel objects seems to be only useful for client-originated cancels, we could keep using the cancelled_asyncs list for server-cancelled asyncs. This avoids unnecessary cancel object creation, and the sync creation could be made greedy instead. * The ownership of the asyncs still is a bit unclear. They were only weakly owned by their process (in asyncs or cancelled_asyncs lists), and removed from these lists when the async is last released. So I don't think they should be owned by the cancel object either, especially as the same list entry is used. The cancel objects can be owned by their process (or not, if we want to rely solely on the handle reference, not sure it makes much difference). I have implemented these two points in https://gitlab.winehq.org/rbernon/wine/-/commits/wip/7797, please have a look and update the MR with the changes if they look good to you. There's another thing which I'm unsure about: The cancel object signaling is only triggered in `async_set_result`, it's unclear to me whether this is guaranteed to always be called in an async lifetime, after it has been cancelled and before it is last released. We were waiting on every async to be signaled before, it seems to me that this can happen through different call paths as well. As I'm suggesting to keep the async weakly referenced by their cancel object, I think we can also call `async_complete_cancel` in `async_destroy` just in case (and I did that in the branch above). I'm then wondering whether it is then still necessary to call it in `async_set_result`? Thanks a lot Rémi! I took almost all of your changes, especially the main thing you mention WRT only using the `async_cancel` object for client-initiated cancellations, as that makes things quite a bit simpler / clearer with very little downsides.
I didn't take the change you mention at the end of the comment since I'm pretty convinced that is already okay. I did spend some time making my previous version handle refcounting and ownership of the `async` and `async_cancel` objects properly, as I had mentioned before. In the end there is no reason not to switch to your approach, but having debugged a bunch of edge-cases of async cancellation, especially during process or thread termination, I'm convinced it's fine to only call `async_complete_cancel` from `async_set_result`, everything should be ordered properly. Also if there was something wrong with async termination in general, things would most likely be broken in a way that our tests catch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_112264
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
``` /* FIXME: it would probably be nice to replace the "canceled" flag with a * single LIST_FOR_EACH_ENTRY_SAFE, but currently cancelling an async can * cause other asyncs to be removed via async_reselect() */ ``` After 3/8 this comment in cancel_async() is off, since there's no "canceled" flag. The comment is still basically true, mind—it's ugly to have to restart the iteration every time, and having async_reselect() mess with other asyncs makes things awfully complicated, including here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_112625
4/8: ``` +struct async_cancel +{ + struct object obj; /* object header */ + struct event_sync *sync; /* sync object for wait/signal */ + struct list asyncs; /* list of asyncs in the cancel group */ + struct list entry; /* entry in process client_cancels list */ +}; ``` In the original CW bug (unfortunately private) there seem to be two contradictory claims about whether CancelIoEx() should wait or not. Should it? [Also, nobody seems to have ever checked CancelSynchronousIo()?] If CancelIoEx() or CancelSynchronousIo() are indeed supposed to wait, then there's a good chance that they're both supposed to wait for the *same* asyncs if called simultaneously, which means we can't store asyncs in the async_cancel's list like this. We'd need to use an array instead, and keep the asyncs in process->cancelled_asyncs. This is probably easiest to test with ntoskrnl, using asyncs that can't actually be canceled, which means that CancelIo() should wait for them to *actually* complete. ``` if (obj) { - int count = cancel_process_async( current->process, obj, thread, req->iosb ); - if (!count && !thread) set_error( STATUS_NOT_FOUND ); + obj_handle_t wait_handle = cancel_process_async( current->process, obj, thread, req->iosb ); + if (!wait_handle && !thread) set_error( STATUS_NOT_FOUND ); release_object( obj ); } ``` This leaks. Probably we should just squash with 5/8. 5/8: ``` + NtWaitForMultipleObjects( 1, &cancel_handle, FALSE, TRUE, NULL ); + NtClose( cancel_handle ); ``` You can probably make this handle automatically close itself when satisfied, like async handles. ``` + else if (status == STATUS_INVALID_HANDLE) + { + return status; + } + + /* TODO: Add a test for the timing of setting IOSB values? It's tricky + * since we probably have to check from a different thread without using + * win32 IPC for synchronization to avoid syncing on system APC + * execution. */ + io_status->Status = status; + io_status->Information = 0; + return status; ``` I guess it doesn't make a difference here, but more generally the iosb is set on !NT_ERROR(status), so that may be more idiomatic to write instead of checking for STATUS_INVALID_HANDLE specifically. [And regarding that comment, the aforementioned ntoskrnl test would probably work here too.] -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_112626
On Mon Aug 11 18:04:43 2025 +0000, Elizabeth Figura wrote:
``` /* FIXME: it would probably be nice to replace the "canceled" flag with a * single LIST_FOR_EACH_ENTRY_SAFE, but currently cancelling an async can * cause other asyncs to be removed via async_reselect() */ ``` After 3/8 this comment in cancel_async() is off, since there's no "canceled" flag. The comment is still basically true, mind—it's ugly to have to restart the iteration every time, and having async_reselect() mess with other asyncs makes things awfully complicated, including here.
I do tweak the comment in 4/8, although that somewhat loses the ugliness connotation. I could move that change to 3/8 and tweak it further. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_112631
In the original CW bug (unfortunately private) there seem to be two contradictory claims about whether CancelIoEx() should wait or not. Should it? \[Also, nobody seems to have ever checked CancelSynchronousIo()?\]
`CancelIoEx()` should wait, the tests in 6/8 should cover that. Those don't check `CancelSynchronousIo()`, although I had the impression that there was something in the preexisting tests implying that it does not wait? I'll have another look.
This is probably easiest to test with ntoskrnl, using asyncs that can't actually be canceled, which means that CancelIo() should wait for them to _actually_ complete.
Ah, good idea. I'll look into this as well. Generally :thumbsup: on the rest. Thanks for the review! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_112632
In the original CW bug (unfortunately private) there seem to be two contradictory claims about whether CancelIoEx() should wait or not. Should it? \[Also, nobody seems to have ever checked CancelSynchronousIo()?\]
`CancelIoEx()` should wait, the tests in 6/8 should cover that. Those don't check `CancelSynchronousIo()`, although I had the impression that there was something in the preexisting tests implying that it does not wait? I'll have another look.
Well, kind of, but there's some subtlety. In a sense, I think the tests aren't exactly convincing, because I think the entire cancellation process is more asynchronous on Wine than it is on Windows. * I suspect that NtCancelIo() and variants synchronously call the cancellation routine, even if they don't wait for the IRPs in question to complete. This is probably testable, although it may get close to unnecessarily poking at internals. * The cancellation routine "must" [1] complete the IRP with STATUS_CANCELLED. The language doesn't *say* "synchronously", but I think it's arguably implied. I haven't tested what happens if you don't do this—maybe it works, or maybe it trips up an assertion in the kernel, or maybe it causes more subtle bugs. On the other hand, if you don't *have* a cancellation routine, there's nothing to cancel. In any case I suspect that all of the native drivers cancel synchronously, *if* their IRPs are cancellable. * I suspect that IoCompleteRequest() will in fact fill the IOSB (and user output buffers, if not direct) and signal everything synchronously. This is probably not possible to prove (how can you be sure that it's synchronous vs a very fast kernel APC?) The reason Wine doesn't already do this synchronously is because it can't deliver the IOSB and user output buffers synchronously, and waiting for the async to be complete is difficult with its current asynchronous structure (as evidenced by, well, the amount of changes this merge request requires). Not to mention winedevice IRPs. If all this is correct, the only real difference there *can* be on Windows is whether NtCancelIo() etc. wait for non-cancellable I/O. The other test performed in the CW bug—that suggested that CancelIo() waits but CancelIoEx() doesn't—was the rather interesting approach of trying NtWriteFile() with enormous buffers, thus limiting oneself by disk space. IIRC disk I/O is not cancellable. Of course, if CancelIoEx() *effectively* waits for the cancel routine, doing it the way we do here may be the right approach anyway. You could probably also find a way to make some async_cancels wait for only those asyncs actually terminated by the cancel routine, although it gets tricky when the cancel routine itself will be asynchronous for winedevice IRPs. [1] https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nc-wdm-dr... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_112658
5/8:
``` + NtWaitForMultipleObjects( 1, &cancel_handle, FALSE, TRUE, NULL ); + NtClose( cancel_handle ); ```
You can probably make this handle automatically close itself when satisfied, like async handles.
I've got something like this, not sure it's an improvement: [cancelio-wait-autoclose-handle.txt](/uploads/430042376bc45cc47c128b481024cdcf/cancelio-wait-autoclose-handle.txt) Just as a sidenote. I'm still to write those ntoskrnl tests for the interesting questions you brought up. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_113952
``` + else if (status == STATUS_INVALID_HANDLE) + { + return status; + } + + /* TODO: Add a test for the timing of setting IOSB values? It's tricky + * since we probably have to check from a different thread without using + * win32 IPC for synchronization to avoid syncing on system APC + * execution. */ + io_status->Status = status; + io_status->Information = 0; + return status; ```
I guess it doesn't make a difference here, but more generally the iosb is set on !NT_ERROR(status), so that may be more idiomatic to write instead of checking for STATUS_INVALID_HANDLE specifically.
Annoyingly it does matter here: one of the tests in 7/8 fails with the change (specifically, the cancel operation IOSB is in fact updated for STATUS_NOT_FOUND). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_113955
On Tue Aug 26 23:54:18 2025 +0000, Matteo Bruni wrote:
``` + else if (status == STATUS_INVALID_HANDLE) + { + return status; + } + + /* TODO: Add a test for the timing of setting IOSB values? It's tricky + * since we probably have to check from a different thread
without using
+ * win32 IPC for synchronization to avoid syncing on system APC + * execution. */ + io_status->Status = status; + io_status->Information = 0; + return status; ```
I guess it doesn't make a difference here, but more generally the iosb is set on !NT_ERROR(status), so that may be more idiomatic to write instead of checking for STATUS_INVALID_HANDLE specifically. Annoyingly it does matter here: one of the tests in 7/8 fails with the change (specifically, the cancel operation IOSB is in fact updated for STATUS_NOT_FOUND). I'm attaching some new ntoskrnl tests which pass for me on up to date Windows 10 and 11. There's room for cleanup but I think they answer most of the questions you had; I'm curious to know if you think this is convincing enough or otherwise you'd like some other piece tested. Or anything else, really (e.g. maybe I'm taking the wrong conclusions from those test results...)
It looks like only `CancelIo()`, not `CancelIoEx()`, waits for the operations to complete. It does wait for completion even if the operations aren't actually cancelled but eventually end regularly on their own. On the other hand, `CancelSynchronousIo()` really wants the completion routine itself to call `IoCompleteRequest()`. The values set in the IOSB `Status` and `Information` fields are apparently not particularly important, or at least nothing breaks immediately if those are set to unexpected values. The thread ID check I added in `cancel_ioctl_irp()` does seem to strongly suggest that the cancel routine is called synchronously. Not sure if this test in particular is "safe" for upstream. Assuming all this makes sense, it looks to me that the approach from this MR is okay once I limit it to `CancelIo()`, leaving `CancelIoEx()` to the old mechanism. Luckily there is no concern with multiple "cancels" having to wait on the same operations. I guess I could also use `async_cancel` for `CancelSynchronousIo()` and get closer to native, although that's never going to be quite right until the whole async cancellation / completion becomes more synchronous. [ntoskrnl-cancel-tests.txt](/uploads/6067830a1d7b7355df5c048a5d67c4df/ntoskrnl-cancel-tests.txt) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_114435
participants (4)
-
Elizabeth Figura (＠zfigura) -
Matteo Bruni
-
Matteo Bruni (＠Mystral)
-
Rémi Bernon