[PATCH 0/1] MR5789: server: Remove limitation for waiting on idle_event of the current process.
[test_WaitForInputIdle.tar.bz2](/uploads/a20c5b119741cd7b0b4813488854a992/test_WaitForInputIdle.tar.bz2) Attached test replicates the problem. Basically the application does process = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId()); ret = WaitForInputIdle(process, 0x7fffffff); assert(ret == 0); With current wine.git WaitForInputIdle() returns WAIT_FAILED because the server call get_process_idle_event() refuses to return idle_event for the current process. If that limitation is removed then wineserver crashes, and other parts of the patch fix the crash. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- server/process.c | 2 +- server/queue.c | 5 ++++- server/thread.c | 2 ++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/server/process.c b/server/process.c index f6d1641cb94..733b0288f72 100644 --- a/server/process.c +++ b/server/process.c @@ -1686,7 +1686,7 @@ DECL_HANDLER(get_process_idle_event) reply->event = 0; if ((process = get_process_from_handle( req->handle, PROCESS_QUERY_INFORMATION ))) { - if (process->idle_event && process != current->process) + if (process->idle_event) reply->event = alloc_handle( current->process, process->idle_event, EVENT_ALL_ACCESS, 0 ); release_object( process ); diff --git a/server/queue.c b/server/queue.c index e881e40271d..8c2b823488e 100644 --- a/server/queue.c +++ b/server/queue.c @@ -1106,11 +1106,14 @@ static int msg_queue_add_queue( struct object *obj, struct wait_queue_entry *ent set_error( STATUS_ACCESS_DENIED ); return 0; } + + add_queue( obj, entry ); + if (process->idle_event && !(queue->wake_mask & QS_SMRESULT)) set_event( process->idle_event ); if (queue->fd && list_empty( &obj->wait_queue )) /* first on the queue */ set_fd_events( queue->fd, POLLIN ); - add_queue( obj, entry ); + return 1; } diff --git a/server/thread.c b/server/thread.c index 55bd63d3030..35f925eaa02 100644 --- a/server/thread.c +++ b/server/thread.c @@ -998,6 +998,8 @@ static int select_on( const select_op_t *select_op, data_size_t op_size, client_ } if (!wait_on_handles( select_op, count, select_op->wait.handles, flags, when )) return 1; + /* check if we woke ourselves up */ + if (!current->wait) return 1; break; case SELECT_SIGNAL_AND_WAIT: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5789
For the reference: it's the .Net WaitForInputIdle() implementation that throws an exception: https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5a... https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5a... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789#note_72346
I think that setting the event in `msg_queue_add_queue` is dangerous. It just works by chance because the queue is always last in the wait handles, but it could simply break any time if wait_on accesses the wait objects again after all `obj->ops->add_queue` calls. What about making `wait_on` aware of idle moving the set_event out of there? Like that: ```diff --git a/server/queue.c b/server/queue.c index e881e40271d..5a077e383e3 100644 --- a/server/queue.c +++ b/server/queue.c @@ -1098,7 +1098,6 @@ static int is_queue_hung( struct msg_queue *queue ) static int msg_queue_add_queue( struct object *obj, struct wait_queue_entry *entry ) { struct msg_queue *queue = (struct msg_queue *)obj; - struct process *process = get_wait_queue_thread(entry)->process; /* a thread can only wait on its own queue */ if (get_wait_queue_thread(entry)->queue != queue) @@ -1106,7 +1105,6 @@ static int msg_queue_add_queue( struct object *obj, struct wait_queue_entry *ent set_error( STATUS_ACCESS_DENIED ); return 0; } - if (process->idle_event && !(queue->wake_mask & QS_SMRESULT)) set_event( process->idle_event ); if (queue->fd && list_empty( &obj->wait_queue )) /* first on the queue */ set_fd_events( queue->fd, POLLIN ); @@ -1255,6 +1253,13 @@ static int check_queue_input_window( struct msg_queue *queue, user_handle_t wind return ret; } +void set_thread_idle( struct thread *thread ) +{ + struct msg_queue *queue = thread->queue; + if ((queue->wake_mask & QS_SMRESULT)) return; + if (thread->process->idle_event) set_event( thread->process->idle_event ); +} + /* make sure the specified thread has a queue */ int init_thread_queue( struct thread *thread ) { diff --git a/server/thread.c b/server/thread.c index 55bd63d3030..8447d82f0a5 100644 --- a/server/thread.c +++ b/server/thread.c @@ -778,7 +778,7 @@ static int wait_on( const select_op_t *select_op, unsigned int count, struct obj { struct thread_wait *wait; struct wait_queue_entry *entry; - unsigned int i; + unsigned int i, idle = 0; if (!(wait = mem_alloc( FIELD_OFFSET(struct thread_wait, queues[count]) ))) return 0; wait->next = current->wait; @@ -802,8 +802,12 @@ static int wait_on( const select_op_t *select_op, unsigned int count, struct obj end_wait( current, get_error() ); return 0; } + + if (obj == (struct object *)current->queue) idle = 1; } - return 1; + + if (idle) set_thread_idle( current ); + return current->wait ? 1 : 0; } static int wait_on_handles( const select_op_t *select_op, unsigned int count, const obj_handle_t *handles, diff --git a/server/user.h b/server/user.h index 1cd42506c23..afe9b807836 100644 --- a/server/user.h +++ b/server/user.h @@ -112,6 +112,7 @@ extern void set_queue_hooks( struct thread *thread, struct hook_table *hooks ); extern void inc_queue_paint_count( struct thread *thread, int incr ); extern void queue_cleanup_window( struct thread *thread, user_handle_t win ); extern int init_thread_queue( struct thread *thread ); +extern void set_thread_idle( struct thread *thread ); extern int attach_thread_input( struct thread *thread_from, struct thread *thread_to ); extern void detach_thread_input( struct thread *thread_from ); extern void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789#note_72349
On Tue Jun 4 17:31:19 2024 +0000, Rémi Bernon wrote:
I think that setting the event in `msg_queue_add_queue` is dangerous. It just works by chance because the queue is always last in the wait handles, but it could simply break any time if `wait_on` accesses the wait object again after all `obj->ops->add_queue` calls. What about making `wait_on` aware of idlenes and moving the set_event out of there? Like that: ```diff diff --git a/server/queue.c b/server/queue.c index e881e40271d..5a077e383e3 100644 --- a/server/queue.c +++ b/server/queue.c @@ -1098,7 +1098,6 @@ static int is_queue_hung( struct msg_queue *queue ) static int msg_queue_add_queue( struct object *obj, struct wait_queue_entry *entry ) { struct msg_queue *queue = (struct msg_queue *)obj; - struct process *process = get_wait_queue_thread(entry)->process;
/* a thread can only wait on its own queue */ if (get_wait_queue_thread(entry)->queue != queue) @@ -1106,7 +1105,6 @@ static int msg_queue_add_queue( struct object *obj, struct wait_queue_entry *ent set_error( STATUS_ACCESS_DENIED ); return 0; } - if (process->idle_event && !(queue->wake_mask & QS_SMRESULT)) set_event( process->idle_event );
if (queue->fd && list_empty( &obj->wait_queue )) /* first on the queue */ set_fd_events( queue->fd, POLLIN ); @@ -1255,6 +1253,13 @@ static int check_queue_input_window( struct msg_queue *queue, user_handle_t wind return ret; }
+void set_thread_idle( struct thread *thread ) +{ + struct msg_queue *queue = thread->queue; + if ((queue->wake_mask & QS_SMRESULT)) return; + if (thread->process->idle_event) set_event( thread->process->idle_event ); +} + /* make sure the specified thread has a queue */ int init_thread_queue( struct thread *thread ) { diff --git a/server/thread.c b/server/thread.c index 55bd63d3030..8447d82f0a5 100644 --- a/server/thread.c +++ b/server/thread.c @@ -778,7 +778,7 @@ static int wait_on( const select_op_t *select_op, unsigned int count, struct obj { struct thread_wait *wait; struct wait_queue_entry *entry; - unsigned int i; + unsigned int i, idle = 0;
if (!(wait = mem_alloc( FIELD_OFFSET(struct thread_wait, queues[count]) ))) return 0; wait->next = current->wait; @@ -802,8 +802,12 @@ static int wait_on( const select_op_t *select_op, unsigned int count, struct obj end_wait( current, get_error() ); return 0; } + + if (obj == (struct object *)current->queue) idle = 1; } - return 1; + + if (idle) set_thread_idle( current ); + return current->wait ? 1 : 0; }
static int wait_on_handles( const select_op_t *select_op, unsigned int count, const obj_handle_t *handles, diff --git a/server/user.h b/server/user.h index 1cd42506c23..afe9b807836 100644 --- a/server/user.h +++ b/server/user.h @@ -112,6 +112,7 @@ extern void set_queue_hooks( struct thread *thread, struct hook_table *hooks ); extern void inc_queue_paint_count( struct thread *thread, int incr ); extern void queue_cleanup_window( struct thread *thread, user_handle_t win ); extern int init_thread_queue( struct thread *thread ); +extern void set_thread_idle( struct thread *thread ); extern int attach_thread_input( struct thread *thread_from, struct thread *thread_to ); extern void detach_thread_input( struct thread *thread_from ); extern void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, ``` This doesn't belong to this patch and probably should be sent as a separate change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789#note_72350
On Tue Jun 4 17:31:19 2024 +0000, Dmitry Timoshkov wrote:
This doesn't belong to this patch and probably should be sent as a separate change. Well this MR works around the issue instead of fixing it. It should fix it first, then remove the process idle_event limitation.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789#note_72369
On Tue Jun 4 20:19:12 2024 +0000, Rémi Bernon wrote:
Well this MR works around the issue instead of fixing it. It should fix it first, then remove the process idle_event limitation. Since this is your patch could you please send it separately? And once it's accepted I'll update this MR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789#note_72370
On Tue Jun 4 20:25:02 2024 +0000, Dmitry Timoshkov wrote:
Since this is your patch could you please send it separately? And once it's accepted I'll update this MR. https://gitlab.winehq.org/wine/wine/-/merge_requests/5792
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789#note_72372
On Wed Jun 5 08:10:48 2024 +0000, Rémi Bernon wrote:
https://gitlab.winehq.org/wine/wine/-/merge_requests/5792 Looks good, with your patch this MR could be simplified to just simple
- if (process->idle_event && process != current->process) + if (process->idle_event) Thanks! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5789#note_72426
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Rémi Bernon