[PATCH v7 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. -- v7: 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 | 182 +++++++++++++++++++++++++++++++++++++++++------ server/process.c | 4 +- server/process.h | 2 +- 3 files changed, 164 insertions(+), 24 deletions(-) diff --git a/server/async.c b/server/async.c index 20f906c5d78..0caeb4d0bd2 100644 --- a/server/async.c +++ b/server/async.c @@ -62,6 +62,8 @@ 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; }; static void async_dump( struct object *obj, int verbose ); @@ -145,6 +147,8 @@ 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) @@ -161,6 +165,8 @@ static void async_destroy( struct object *obj ) release_object( async->thread ); } +static void async_complete_cancel( struct async *async ); + /* notifies client thread of new status of its async request */ void async_terminate( struct async *async, unsigned int status ) { @@ -282,6 +288,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; @@ -541,6 +548,8 @@ 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) { list_remove( &async->queue_entry ); @@ -573,34 +582,122 @@ int async_waiting( struct async_queue *queue ) return !async->terminated; } -static void cancel_async( struct process *process, struct async *async ) +struct async_cancel +{ + struct object obj; /* object header */ + struct process *process; + struct thread *thread; + struct list asyncs; + obj_handle_t wait_handle; + unsigned int count; + struct list async_cancels_entry; +}; + +static void async_cancel_dump( struct object *obj, int verbose ); +static int async_cancel_signaled( struct object *obj, struct wait_queue_entry *entry ); +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 */ + add_queue, /* add_queue */ + remove_queue, /* remove_queue */ + async_cancel_signaled, /* signaled */ + no_satisfied, /* satisfied */ + no_signal, /* signal */ + no_get_fd, /* get_fd */ + default_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 process=%p, thread=%p, wait_handle %u, count %u\n", + cancel->process, cancel->thread, cancel->wait_handle, cancel->count ); +} + +static int async_cancel_signaled( struct object *obj, struct wait_queue_entry *entry ) +{ + struct async_cancel *cancel = (struct async_cancel *)obj; + + assert( obj->ops == &async_cancel_ops ); + return !cancel->count; +} + +static void async_cancel_destroy( struct object *obj ) +{ + struct async_cancel *cancel = (struct async_cancel *)obj; + + assert( obj->ops == &async_cancel_ops ); + list_remove( &cancel->async_cancels_entry ); + if (cancel->thread) release_object( cancel->thread ); + release_object( cancel->process ); +} + +static struct async_cancel *create_async_cancel( struct process *process, struct thread *thread ) +{ + struct async_cancel *cancel; + + if (!(cancel = alloc_object( &async_cancel_ops ))) return NULL; + cancel->process = (struct process *)grab_object( process ); + cancel->thread = thread ? (struct thread *)grab_object( thread ) : 0; + list_init(&cancel->asyncs); + cancel->wait_handle = 0; + cancel->count = 0; + list_add_tail( &process->async_cancels, &cancel->async_cancels_entry ); + return cancel; +} + +static void cancel_async( struct async_cancel *cancel, struct async *async ) { list_remove( &async->process_entry ); - list_add_tail( &process->cancelled_asyncs, &async->process_entry ); + list_add_tail( &cancel->asyncs, &async->process_entry ); + grab_object( async ); + grab_object( cancel ); + async->async_cancel = cancel; + cancel->count++; 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( async, &process->cancelled_asyncs, struct async, process_entry ) - if (async->data.user == user) return async; + LIST_FOR_EACH_ENTRY( cancel, &process->async_cancels, struct async_cancel, async_cancels_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->asyncs, struct async, process_entry ) if (async->data.user == user) return async; return NULL; } -static int cancel_process_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) +static struct async_cancel *cancel_process_async( struct process *process, struct object *obj, struct thread *thread, + client_ptr_t iosb ) { + struct async_cancel *cancel; 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( process, thread ))) 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,19 +706,37 @@ restart: (!thread || async->thread == thread) && (!iosb || async->data.iosb == iosb)) { - cancel_async( process, async ); + cancel_async( cancel, async ); woken++; goto restart; } } - return woken; + if (cancel->count) cancel->wait_handle = alloc_handle( process, cancel, SYNCHRONIZE, 0 ); + release_object( cancel ); + return woken ? cancel : NULL; +} + +static void async_complete_cancel( struct async *async ) +{ + struct async_cancel *cancel; + + if ((cancel = async->async_cancel)) + { + if (!--cancel->count && cancel->wait_handle) wake_up( &cancel->obj, 0 ); + release_object( cancel ); + async->async_cancel = NULL; + release_object( async ); + } } static int cancel_blocking( struct process *process, struct thread *thread, client_ptr_t iosb ) { + struct async_cancel *cancel; struct async *async; int woken = 0; + if (!(cancel = create_async_cancel( process, thread ))) return 0; + restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { @@ -629,25 +744,42 @@ restart: if (async->blocking && async->thread == thread && (!iosb || async->data.iosb == iosb)) { - cancel_async( process, async ); + cancel_async( cancel, async ); woken++; goto restart; } } + + /* There's no waiting for blocking asyncs */ + release_object( cancel ); return woken; } void cancel_terminating_process_asyncs( struct process *process ) { - struct async *async; + struct async_cancel *cancel, *next_cancel; + struct async *async, *next_async; + + if (!(cancel = create_async_cancel( process, NULL ))) return; restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { if (async->terminated) continue; - cancel_async( process, async ); + cancel_async( cancel, async ); goto restart; } + release_object( cancel ); + + LIST_FOR_EACH_ENTRY_SAFE( cancel, next_cancel, &process->async_cancels, struct async_cancel, async_cancels_entry ) + { + LIST_FOR_EACH_ENTRY_SAFE( async, next_async, &cancel->asyncs, struct async, process_entry ) + { + async->async_cancel = NULL; + release_object( cancel ); + release_object( async ); + } + } } int async_close_obj_handle( struct object *obj, struct process *process, obj_handle_t handle ) @@ -655,34 +787,42 @@ int async_close_obj_handle( struct object *obj, struct process *process, obj_han /* Handle a special case when the last object handle in the given process is closed. * If this is the last object handle overall that is handled in object's close_handle and * destruction. */ + struct async_cancel *cancel; struct async *async; if (obj->handle_count == 1 || get_obj_handle_count( process, obj ) != 1) return 1; + if (!(cancel = create_async_cancel( process, NULL ))) return 0; + restart: LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) { 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( cancel, async ); goto restart; } + release_object( cancel ); return 1; } void cancel_terminating_thread_asyncs( struct thread *thread ) { + struct async_cancel *cancel; struct async *async; + if (!(cancel = create_async_cancel( thread->process, thread ))) return; + restart: LIST_FOR_EACH_ENTRY( async, &thread->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( cancel, async ); goto restart; } + release_object( cancel ); } /* wake up async operations on the queue */ @@ -827,13 +967,13 @@ 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 async_cancel *cancel; - if (obj) - { - int count = cancel_process_async( current->process, obj, thread, req->iosb ); - if (!count && !thread) set_error( STATUS_NOT_FOUND ); - release_object( obj ); - } + if (!obj) return; + + cancel = cancel_process_async( current->process, obj, thread, req->iosb ); + if (!cancel && !thread) set_error( STATUS_NOT_FOUND ); + release_object( obj ); } /* get async result from associated iosb */ diff --git a/server/process.c b/server/process.c index 268744ef2cf..722195ecc2f 100644 --- a/server/process.c +++ b/server/process.c @@ -705,7 +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->async_cancels ); list_init( &process->classes ); list_init( &process->views ); @@ -781,7 +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( list_empty( &process->async_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..1de426eed53 100644 --- a/server/process.h +++ b/server/process.h @@ -67,7 +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 async_cancels; /* list of 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 | 18 ++++++++++++++---- 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, 28 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index a43d464403d..8f86f7beae3 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,22 @@ 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 ); + 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 0caeb4d0bd2..1251c07c9bf 100644 --- a/server/async.c +++ b/server/async.c @@ -974,6 +974,7 @@ DECL_HANDLER(cancel_async) cancel = cancel_process_async( current->process, obj, thread, req->iosb ); if (!cancel && !thread) set_error( STATUS_NOT_FOUND ); release_object( obj ); + if (cancel) reply->cancel_handle = cancel->wait_handle; } /* get async result from associated iosb */ 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
Rémi Bernon (@rbernon) commented about server/async.c:
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 */
```suggestion:-0+0 struct list process_entry; /* entry in process / cancel list */ ``` Or, as it's not process-only now, maybe even: ```suggestion:-0+0 struct list entry; /* entry in process / cancel list */ ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111814
Rémi Bernon (@rbernon) commented about server/async.c:
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; };
```suggestion:-5+0 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 */ }; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111813
Rémi Bernon (@rbernon) commented about server/async.c:
+static void async_cancel_dump( struct object *obj, int verbose ); +static int async_cancel_signaled( struct object *obj, struct wait_queue_entry *entry ); +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 */ + add_queue, /* add_queue */ + remove_queue, /* remove_queue */ + async_cancel_signaled, /* signaled */ + no_satisfied, /* satisfied */ + no_signal, /* signal */ + no_get_fd, /* get_fd */ + default_get_sync, /* get_sync */ Let's use an event sync, its signaled logic should be simple enough.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111815
Rémi Bernon (@rbernon) commented about server/async.c:
return !async->terminated; }
-static void cancel_async( struct process *process, struct async *async ) +struct async_cancel +{ + struct object obj; /* object header */ + struct process *process; + struct thread *thread; + struct list asyncs; + obj_handle_t wait_handle; + unsigned int count; + struct list async_cancels_entry; +};
```suggestion:-7+0 struct object obj; /* object header */ struct process *process; /* asyncs owner process */ struct thread *thread; /* asyncs owner thread when cancelling a single thread asyncs */ struct list asyncs; /* list of asyncs in the cancel group */ obj_handle_t wait_handle; /* handle for client to wait upon */ unsigned int count; /* number of asyncs in the cancel group */ struct list entry; /* entry in process async_cancels list */ }; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111817
Rémi Bernon (@rbernon) commented about server/async.c:
(!thread || async->thread == thread) && (!iosb || async->data.iosb == iosb)) { - cancel_async( process, async ); + cancel_async( cancel, async ); woken++; goto restart; } } - return woken; + if (cancel->count) cancel->wait_handle = alloc_handle( process, cancel, SYNCHRONIZE, 0 ); + release_object( cancel ); + return woken ? cancel : NULL;
Doesn't look right to return an object after releasing the owned reference. Returning its wait_handle should be enough for the next commit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111818
Rémi Bernon (@rbernon) commented about server/async.c:
+{ + struct async_cancel *cancel = (struct async_cancel *)obj; + + assert( obj->ops == &async_cancel_ops ); + list_remove( &cancel->async_cancels_entry ); + if (cancel->thread) release_object( cancel->thread ); + release_object( cancel->process ); +} + +static struct async_cancel *create_async_cancel( struct process *process, struct thread *thread ) +{ + struct async_cancel *cancel; + + if (!(cancel = alloc_object( &async_cancel_ops ))) return NULL; + cancel->process = (struct process *)grab_object( process ); + cancel->thread = thread ? (struct thread *)grab_object( thread ) : 0; Seems a bit weird to me that a cancel group should need to own a reference to its process/thread, it should never outlive them in the first place. Does it even need pointers to process/thread? It doesn't looks like it in this commit at least.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111819
Rémi Bernon (@rbernon) commented about server/async.c:
+ cancel->process = (struct process *)grab_object( process ); + cancel->thread = thread ? (struct thread *)grab_object( thread ) : 0; + list_init(&cancel->asyncs); + cancel->wait_handle = 0; + cancel->count = 0; + list_add_tail( &process->async_cancels, &cancel->async_cancels_entry ); + return cancel; +} + +static void cancel_async( struct async_cancel *cancel, struct async *async ) { list_remove( &async->process_entry ); - list_add_tail( &process->cancelled_asyncs, &async->process_entry ); + list_add_tail( &cancel->asyncs, &async->process_entry ); + grab_object( async ); + grab_object( cancel ); I think this makes the cancel / async ownership unclear. I think the cancel group should own its async as they are kept in a list, but not the other way around.
I understand however that the cancel objects also need to be kept alive until they get signaled, but I think this should match their insertion/removal from the `process->async_cancels` list. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111820
Rémi Bernon (@rbernon) commented about server/async.c:
+ struct async_cancel *cancel = (struct async_cancel *)obj; + + assert( obj->ops == &async_cancel_ops ); + list_remove( &cancel->async_cancels_entry ); + if (cancel->thread) release_object( cancel->thread ); + release_object( cancel->process ); +} + +static struct async_cancel *create_async_cancel( struct process *process, struct thread *thread ) +{ + struct async_cancel *cancel; + + if (!(cancel = alloc_object( &async_cancel_ops ))) return NULL; + cancel->process = (struct process *)grab_object( process ); + cancel->thread = thread ? (struct thread *)grab_object( thread ) : 0; + list_init(&cancel->asyncs);
list_init( &cancel->asyncs );
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_111816
participants (3)
-
Matteo Bruni -
Matteo Bruni (＠Mystral)
-
Rémi Bernon