[PATCH v2] server: Track desktop users per thread instead of per process.
As some thread may use a different desktop from their process. This fixes the user32 win tests, which leaks a desktop that never gets closed. The test_shell_window test creates a new desktop, which spawns explorer.exe process, incrementing the desktop user count to 1, then associates the desktop to a thread, which closes it on exit. Never the user count is incremented to 2, and closing the thread desktop doesn't either check whether the desktop process should be terminated. Reversely, it is possible to create a desktop, associate it with a thread /and/ a process, and this time the desktop process would be terminated when the process exits, although the thread may still be using it. Tracking the users per thread is more robust and fixes the problem as set_thread_desktop increments the desktop user count, and thread exit decrements it. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v2: Close the system process threads desktops before setting the is_system flag. server/process.c | 5 +++- server/thread.c | 10 +++++++- server/user.h | 2 ++ server/winstation.c | 62 +++++++++++++++++++++++++++------------------ 4 files changed, 52 insertions(+), 27 deletions(-) diff --git a/server/process.c b/server/process.c index 8ad5a59a20b..3419f82c153 100644 --- a/server/process.c +++ b/server/process.c @@ -1503,6 +1503,7 @@ DECL_HANDLER(get_process_idle_event) DECL_HANDLER(make_process_system) { struct process *process = current->process; + struct thread *thread; if (!shutdown_event) { @@ -1515,8 +1516,10 @@ DECL_HANDLER(make_process_system) if (!process->is_system) { - process->is_system = 1; + LIST_FOR_EACH_ENTRY( thread, &process->thread_list, struct thread, proc_entry ) + close_thread_desktop( thread ); close_process_desktop( process ); + process->is_system = 1; if (!--user_processes && !shutdown_stage && master_socket_timeout != TIMEOUT_INFINITE) shutdown_timeout = add_timeout_user( master_socket_timeout, server_shutdown_timeout, NULL ); } diff --git a/server/thread.c b/server/thread.c index fe9f9bdec37..5375b055758 100644 --- a/server/thread.c +++ b/server/thread.c @@ -306,6 +306,7 @@ static struct context *create_thread_context( struct thread *thread ) /* create a new thread */ struct thread *create_thread( int fd, struct process *process, const struct security_descriptor *sd ) { + struct desktop *desktop; struct thread *thread; int request_pipe[2]; @@ -342,7 +343,7 @@ struct thread *create_thread( int fd, struct process *process, const struct secu init_thread_structure( thread ); thread->process = (struct process *)grab_object( process ); - thread->desktop = process->desktop; + thread->desktop = 0; thread->affinity = process->affinity; if (!current) current = thread; @@ -369,6 +370,13 @@ struct thread *create_thread( int fd, struct process *process, const struct secu return NULL; } + if (process->desktop && (desktop = get_desktop_obj( process, process->desktop, 0 ))) + { + set_thread_default_desktop( process, thread, desktop, process->desktop ); + release_object( desktop ); + } + clear_error(); /* ignore errors */ + set_fd_events( thread->request_fd, POLLIN ); /* start listening to events */ add_process_thread( thread->process, thread ); return thread; diff --git a/server/user.h b/server/user.h index 6267f3e2881..a5f906cea51 100644 --- a/server/user.h +++ b/server/user.h @@ -185,6 +185,8 @@ extern struct winstation *get_process_winstation( struct process *process, unsig extern struct desktop *get_thread_desktop( struct thread *thread, unsigned int access ); extern void connect_process_winstation( struct process *process, struct thread *parent_thread, struct process *parent_process ); +extern void set_thread_default_desktop( struct process *process, struct thread *thread, + struct desktop *desktop, obj_handle_t handle ); extern void set_process_default_desktop( struct process *process, struct desktop *desktop, obj_handle_t handle ); extern void close_process_desktop( struct process *process ); diff --git a/server/winstation.c b/server/winstation.c index 1c7552f0687..94527899311 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -324,15 +324,23 @@ static void add_desktop_user( struct desktop *desktop ) /* remove a user of the desktop and start the close timeout if necessary */ static void remove_desktop_user( struct desktop *desktop ) { + struct process *process; assert( desktop->users > 0 ); desktop->users--; /* if we have one remaining user, it has to be the manager of the desktop window */ - if (desktop->users == 1 && get_top_window_owner( desktop )) - { - assert( !desktop->close_timeout ); + if ((process = get_top_window_owner( desktop )) && desktop->users == process->running_threads && !desktop->close_timeout) desktop->close_timeout = add_timeout_user( -TICKS_PER_SEC, close_desktop_timeout, desktop ); - } +} + +/* set the thread desktop to the process default desktop */ +void set_thread_default_desktop( struct process *process, struct thread *thread, + struct desktop *desktop, obj_handle_t handle ) +{ + if (thread->desktop || thread->desktop == handle) return; /* nothing to do */ + thread->desktop = handle; + + if (!process->is_system) add_desktop_user( desktop ); } /* set the process default desktop handle */ @@ -340,24 +348,13 @@ void set_process_default_desktop( struct process *process, struct desktop *deskt obj_handle_t handle ) { struct thread *thread; - struct desktop *old_desktop; if (process->desktop == handle) return; /* nothing to do */ - - if (!(old_desktop = get_desktop_obj( process, process->desktop, 0 ))) clear_error(); process->desktop = handle; /* set desktop for threads that don't have one yet */ LIST_FOR_EACH_ENTRY( thread, &process->thread_list, struct thread, proc_entry ) - if (!thread->desktop) thread->desktop = handle; - - if (!process->is_system && desktop != old_desktop) - { - add_desktop_user( desktop ); - if (old_desktop) remove_desktop_user( old_desktop ); - } - - if (old_desktop) release_object( old_desktop ); + set_thread_default_desktop( process, thread, desktop, handle ); } /* connect a process to its window station */ @@ -413,23 +410,31 @@ done: /* close the desktop of a given process */ void close_process_desktop( struct process *process ) { - struct desktop *desktop; + obj_handle_t handle; - if (process->desktop && (desktop = get_desktop_obj( process, process->desktop, 0 ))) - { - remove_desktop_user( desktop ); - release_object( desktop ); - } - clear_error(); /* ignore errors */ + if (!(handle = process->desktop)) return; + process->desktop = 0; + + close_handle( process, handle ); } /* close the desktop of a given thread */ void close_thread_desktop( struct thread *thread ) { - obj_handle_t handle = thread->desktop; + struct desktop *desktop; + obj_handle_t handle; + if (!(handle = thread->desktop)) return; thread->desktop = 0; - if (handle) close_handle( thread->process, handle ); + + if (!thread->process->is_system && (desktop = get_desktop_obj( thread->process, handle, 0 ))) + { + remove_desktop_user( desktop ); + release_object( desktop ); + } + clear_error(); /* ignore errors */ + + close_handle( thread->process, handle ); } /* create a window station */ @@ -624,7 +629,14 @@ DECL_HANDLER(set_thread_desktop) if (old_desktop != new_desktop && current->desktop_users > 0) set_error( STATUS_DEVICE_BUSY ); else + { current->desktop = req->handle; /* FIXME: should we close the old one? */ + if (!current->process->is_system && old_desktop != new_desktop) + { + add_desktop_user( new_desktop ); + if (old_desktop) remove_desktop_user( old_desktop ); + } + } if (!current->process->desktop) set_process_default_desktop( current->process, new_desktop, req->handle ); -- 2.31.0
Rémi Bernon <rbernon(a)codeweavers.com> writes:
As some thread may use a different desktop from their process.
This fixes the user32 win tests, which leaks a desktop that never gets closed. The test_shell_window test creates a new desktop, which spawns explorer.exe process, incrementing the desktop user count to 1, then associates the desktop to a thread, which closes it on exit.
Never the user count is incremented to 2, and closing the thread desktop doesn't either check whether the desktop process should be terminated.
Reversely, it is possible to create a desktop, associate it with a thread /and/ a process, and this time the desktop process would be terminated when the process exits, although the thread may still be using it.
Tracking the users per thread is more robust and fixes the problem as set_thread_desktop increments the desktop user count, and thread exit decrements it.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v2: Close the system process threads desktops before setting the is_system flag.
server/process.c | 5 +++- server/thread.c | 10 +++++++- server/user.h | 2 ++ server/winstation.c | 62 +++++++++++++++++++++++++++------------------ 4 files changed, 52 insertions(+), 27 deletions(-)
This breaks the tests here: tools/runtest -q -P wine -T . -M services.exe -p programs/services/tests/services.exe_test.exe service && touch programs/services/tests/service.ok service.c:422: Test failed: service: GetDesktopWindow returned invalid window 00000000 service.c:422: Test failed: service: CreateWindow failed: 1411 service.c:422: Test failed: service: style = 0, expected visible service.c:422: Test failed: service: ShowWindow returned 0 service.c:422: Test failed: service: DestroyWindow failed: 00000005 make: *** [Makefile:157374: programs/services/tests/service.ok] Error 5 -- Alexandre Julliard julliard(a)winehq.org
On 4/20/21 10:51 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
As some thread may use a different desktop from their process.
This fixes the user32 win tests, which leaks a desktop that never gets closed. The test_shell_window test creates a new desktop, which spawns explorer.exe process, incrementing the desktop user count to 1, then associates the desktop to a thread, which closes it on exit.
Never the user count is incremented to 2, and closing the thread desktop doesn't either check whether the desktop process should be terminated.
Reversely, it is possible to create a desktop, associate it with a thread /and/ a process, and this time the desktop process would be terminated when the process exits, although the thread may still be using it.
Tracking the users per thread is more robust and fixes the problem as set_thread_desktop increments the desktop user count, and thread exit decrements it.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> ---
v2: Close the system process threads desktops before setting the is_system flag.
server/process.c | 5 +++- server/thread.c | 10 +++++++- server/user.h | 2 ++ server/winstation.c | 62 +++++++++++++++++++++++++++------------------ 4 files changed, 52 insertions(+), 27 deletions(-)
This breaks the tests here:
tools/runtest -q -P wine -T . -M services.exe -p programs/services/tests/services.exe_test.exe service && touch programs/services/tests/service.ok service.c:422: Test failed: service: GetDesktopWindow returned invalid window 00000000 service.c:422: Test failed: service: CreateWindow failed: 1411 service.c:422: Test failed: service: style = 0, expected visible service.c:422: Test failed: service: ShowWindow returned 0 service.c:422: Test failed: service: DestroyWindow failed: 00000005 make: *** [Makefile:157374: programs/services/tests/service.ok] Error 5
Interesting... the problem is that we now reset the thread desktop handle to 0 when making system processes, were we were previously only messing with the process (default) desktop handle. Interestingly, I also saw that this could be reversed later, as the process default desktop can be set after make_process_system was called already, restoring process and thread desktop handles to non-0 values. I'm not completely sure but I think we can consider releasing only the desktop user reference count when making a process system, without touching the thread (or the process) desktop handles? Threads would keep a handle to a possibly already terminated desktop, but I think it's already the case when the process default desktop handle is later changed. -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (2)
-
Alexandre Julliard -
Rémi Bernon