From: Qian Hong Subject: [PATCH] explorer: make process as system process. Message-Id: <564E8F36.60309@codeweavers.com> Date: Fri, 20 Nov 2015 11:10:46 +0800 Hi, This patch fix a desktop user count leak in wineserver uncovered by 5da10c9a0e405c4b502be820ae0385d634306a76. Below is how the desktop user reference count leaked before the patch: 1. When the first user process created normally, explorer.exe also created normally with new desktop "Default" 2. One user process creates Thread 2, Thread 2 creates a new desktop called "desk2", then switch its thread desktop to "desk2" and creates a window, this will bring up a new explorer.exe which handles "desk2". 3. Thread 2 destroy the window, and switch the thread desktop back to "Default". At this time, there is no user of "desk 2" other than the second explorer.exe itself, however the user refcount doesn't decrease because we increase/decrease refcount base on process rather than thread. 4. All user process exit, the user refcount of "Default" decrease to 0 so it post a message to close the first explorer.exe. However, the second explorer.exe never exit because the refcount leaking in step 3. One possible solution is to count threads as users rather than processes, but explorer.exe itself could have more than on thread, which makes this way complex. The attached patch use another way, just make explorer.exe an system process, and it will quit automatically few seconds after all user process exit. I don't know if there is a historical reason not to make explorer.exe a system process, or if there is a compatible reason, any background is great appreciated. I agree this patch might not be safe enough for code freeze, just put it here for code review. If anyone is going to implement alternative way to fix this problem I'm also glad to see. Thank you! Signed-off-by: Qian Hong --- programs/explorer/desktop.c | 12 +++++++++- server/process.c | 2 -- server/user.h | 2 -- server/winstation.c | 54 --------------------------------------------- 4 files changed, 11 insertions(+), 59 deletions(-) diff --git a/programs/explorer/desktop.c b/programs/explorer/desktop.c index 2b8502b..a474efd 100644 --- a/programs/explorer/desktop.c +++ b/programs/explorer/desktop.c @@ -37,6 +37,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(explorer); +extern HANDLE CDECL __wine_make_process_system(void); + #define DESKTOP_CLASS_ATOM ((LPCWSTR)MAKEINTATOM(32769)) #define DESKTOP_ALL_ACCESS 0x01ff @@ -1024,8 +1026,16 @@ void manage_desktop( WCHAR *arg ) /* run the desktop message loop */ if (hwnd) { + HANDLE exit_event = __wine_make_process_system(); WINE_TRACE( "desktop message loop starting on hwnd %p\n", hwnd ); - while (GetMessageW( &msg, 0, 0, 0 )) DispatchMessageW( &msg ); + while (MsgWaitForMultipleObjectsEx( 1, &exit_event, INFINITE, QS_ALLINPUT, 0 ) == WAIT_OBJECT_0 + 1) + { + while (PeekMessageW( &msg, NULL, 0, 0, PM_REMOVE )) + { + TranslateMessage( &msg ); + DispatchMessageW( &msg ); + } + } WINE_TRACE( "desktop message loop exiting for hwnd %p\n", hwnd ); } diff --git a/server/process.c b/server/process.c index e00b429..a248aff 100644 --- a/server/process.c +++ b/server/process.c @@ -829,7 +829,6 @@ static void process_killed( struct process *process ) assert( list_empty( &process->thread_list )); process->end_time = current_time; - if (!process->is_system) close_process_desktop( process ); process->winstation = 0; process->desktop = 0; close_process_handles( process ); @@ -1526,7 +1525,6 @@ DECL_HANDLER(make_process_system) if (!process->is_system) { process->is_system = 1; - close_process_desktop( process ); 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/user.h b/server/user.h index 08d0959..3feaa9c 100644 --- a/server/user.h +++ b/server/user.h @@ -73,7 +73,6 @@ struct desktop struct list hotkeys; /* list of registered hotkeys */ struct timeout_user *close_timeout; /* timeout before closing the desktop */ struct thread_input *foreground_input; /* thread input of foreground thread */ - unsigned int users; /* processes and threads using this desktop */ struct global_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ }; @@ -181,7 +180,6 @@ extern struct desktop *get_thread_desktop( struct thread *thread, unsigned int a extern void connect_process_winstation( struct process *process, struct thread *parent ); extern void set_process_default_desktop( struct process *process, struct desktop *desktop, obj_handle_t handle ); -extern void close_process_desktop( struct process *process ); extern void close_thread_desktop( struct thread *thread ); /* mirror a rectangle respective to the window client area */ diff --git a/server/winstation.c b/server/winstation.c index 08389bb..c4e7694 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -231,7 +231,6 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned desktop->global_hooks = NULL; desktop->close_timeout = NULL; desktop->foreground_input = NULL; - desktop->users = 0; memset( &desktop->cursor, 0, sizeof(desktop->cursor) ); memset( desktop->keystate, 0, sizeof(desktop->keystate) ); list_add_tail( &winstation->desktops, &desktop->entry ); @@ -300,40 +299,6 @@ struct desktop *get_thread_desktop( struct thread *thread, unsigned int access ) return get_desktop_obj( thread->process, thread->desktop, access ); } -static void close_desktop_timeout( void *private ) -{ - struct desktop *desktop = private; - - desktop->close_timeout = NULL; - unlink_named_object( &desktop->obj ); /* make sure no other process can open it */ - post_desktop_message( desktop, WM_CLOSE, 0, 0 ); /* and signal the owner to quit */ -} - -/* add a user of the desktop and cancel the close timeout */ -static void add_desktop_user( struct desktop *desktop ) -{ - desktop->users++; - if (desktop->close_timeout) - { - remove_timeout_user( desktop->close_timeout ); - desktop->close_timeout = NULL; - } -} - -/* remove a user of the desktop and start the close timeout if necessary */ -static void remove_desktop_user( struct desktop *desktop ) -{ - 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 ); - desktop->close_timeout = add_timeout_user( -TICKS_PER_SEC, close_desktop_timeout, desktop ); - } -} - /* set the process default desktop handle */ void set_process_default_desktop( struct process *process, struct desktop *desktop, obj_handle_t handle ) @@ -350,12 +315,6 @@ void set_process_default_desktop( struct process *process, struct desktop *deskt 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 ); } @@ -401,19 +360,6 @@ done: clear_error(); } -/* close the desktop of a given process */ -void close_process_desktop( struct process *process ) -{ - struct desktop *desktop; - - if (process->desktop && (desktop = get_desktop_obj( process, process->desktop, 0 ))) - { - remove_desktop_user( desktop ); - release_object( desktop ); - } - clear_error(); /* ignore errors */ -} - /* close the desktop of a given thread */ void close_thread_desktop( struct thread *thread ) {