From af7ccb921a4d0355a84fe0cf7308c7a5fc63c279 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 20 May 2016 08:43:39 +0200 Subject: [PATCH] Simplify windows implementation of _dbus_spawn_async_with_babysittery_sitter(). The child process is now started from the main thread. Afterwards a second thread is created to provide a blocking free child exit detection. Also make the implementation thread safe by using a mutex to protect access to some DBusBabysitter struct members. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=95191 --- dbus/dbus-spawn-win.c | 249 +++++++++++++++++++++++++++----------------------- 1 file changed, 135 insertions(+), 114 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 5cb3044..36ab5c1 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -55,35 +55,59 @@ #include #endif +/* Local implementations of win related mutex functions to be able to track them */ + +static DBusCMutex * +_dbus_win_cmutex_new (void) +{ + HANDLE handle; + handle = CreateMutex (NULL, FALSE, NULL); + return (DBusCMutex *) handle; +} + +static void +_dbus_win_cmutex_free (DBusCMutex *mutex) +{ + CloseHandle ((HANDLE *) mutex); +} + +static void +_dbus_win_cmutex_lock (DBusCMutex *mutex) +{ + WaitForSingleObject ((HANDLE *) mutex, INFINITE); +} + +static void +_dbus_win_cmutex_unlock (DBusCMutex *mutex) +{ + ReleaseMutex ((HANDLE *) mutex); +} + + /** * Babysitter implementation details */ struct DBusBabysitter { - DBusAtomic refcount; - - HANDLE start_sync_event; - - char *log_name; - - int argc; - char **argv; - char **envp; + DBusCMutex *mutex; - HANDLE thread_handle; + /* used by both threads and guarded by a mutex */ + DBusAtomic refcount; HANDLE child_handle; - DBusSocket socket_to_babysitter; /* Connection to the babysitter thread */ + dbus_bool_t have_spawn_errno; + int spawn_errno; + dbus_bool_t have_child_status; + int child_status; + char *log_name; DBusSocket socket_to_main; + /* used by main thread only */ + HANDLE thread_handle; DBusWatchList *watches; DBusWatch *sitter_watch; + DBusSocket socket_to_babysitter; /* Connection to the babysitter thread */ DBusBabysitterFinishedFunc finished_cb; void *finished_data; - - dbus_bool_t have_spawn_errno; - int spawn_errno; - dbus_bool_t have_child_status; - int child_status; }; static void @@ -110,25 +134,21 @@ _dbus_babysitter_new (void) if (sitter == NULL) return NULL; - old_refcount = _dbus_atomic_inc (&sitter->refcount); - - _dbus_babysitter_trace_ref (sitter, old_refcount, old_refcount+1, __FUNCTION__); - - sitter->start_sync_event = CreateEvent (NULL, FALSE, FALSE, NULL); - if (sitter->start_sync_event == NULL) + sitter->mutex = _dbus_win_cmutex_new (); + if (sitter->mutex == NULL) { _dbus_babysitter_unref (sitter); return NULL; } + old_refcount = _dbus_atomic_inc (&sitter->refcount); + + _dbus_babysitter_trace_ref (sitter, old_refcount, old_refcount+1, __FUNCTION__); + sitter->child_handle = NULL; sitter->socket_to_babysitter = sitter->socket_to_main = _dbus_socket_get_invalid (); - sitter->argc = 0; - sitter->argv = NULL; - sitter->envp = NULL; - sitter->watches = _dbus_watch_list_new (); if (sitter->watches == NULL) { @@ -165,7 +185,7 @@ _dbus_babysitter_ref (DBusBabysitter *sitter) static void close_socket_to_babysitter (DBusBabysitter *sitter) { - _dbus_verbose ("Closing babysitter\n"); + _dbus_verbose ("Closing babysitter %p\n", sitter); if (sitter->sitter_watch != NULL) { @@ -191,7 +211,6 @@ close_socket_to_babysitter (DBusBabysitter *sitter) void _dbus_babysitter_unref (DBusBabysitter *sitter) { - int i; dbus_int32_t old_refcount; PING(); @@ -203,6 +222,7 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) if (old_refcount == 1) { + _dbus_win_cmutex_lock (sitter->mutex); close_socket_to_babysitter (sitter); if (sitter->socket_to_main.sock != INVALID_SOCKET) @@ -211,29 +231,6 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) sitter->socket_to_main.sock = INVALID_SOCKET; } - PING(); - if (sitter->argv != NULL) - { - for (i = 0; i < sitter->argc; i++) - if (sitter->argv[i] != NULL) - { - dbus_free (sitter->argv[i]); - sitter->argv[i] = NULL; - } - dbus_free (sitter->argv); - sitter->argv = NULL; - } - - if (sitter->envp != NULL) - { - char **e = sitter->envp; - - while (*e) - dbus_free (*e++); - dbus_free (sitter->envp); - sitter->envp = NULL; - } - if (sitter->child_handle != NULL) { CloseHandle (sitter->child_handle); @@ -250,13 +247,6 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) if (sitter->watches) _dbus_watch_list_free (sitter->watches); - if (sitter->start_sync_event != NULL) - { - PING(); - CloseHandle (sitter->start_sync_event); - sitter->start_sync_event = NULL; - } - if (sitter->thread_handle) { CloseHandle (sitter->thread_handle); @@ -265,6 +255,9 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) dbus_free (sitter->log_name); + _dbus_win_cmutex_unlock (sitter->mutex); + _dbus_win_cmutex_free (sitter->mutex); + dbus_free (sitter); } } @@ -272,12 +265,16 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) void _dbus_babysitter_kill_child (DBusBabysitter *sitter) { + HANDLE handle; PING(); - if (sitter->child_handle == NULL) + _dbus_win_cmutex_lock (sitter->mutex); + handle = sitter->child_handle; + _dbus_win_cmutex_unlock (sitter->mutex); + if (handle == NULL) return; /* child is already dead, or we're so hosed we'll never recover */ PING(); - TerminateProcess (sitter->child_handle, 12345); + TerminateProcess (handle, 12345); } /** @@ -288,8 +285,12 @@ _dbus_babysitter_kill_child (DBusBabysitter *sitter) dbus_bool_t _dbus_babysitter_get_child_exited (DBusBabysitter *sitter) { + dbus_bool_t state; PING(); - return (sitter->child_handle == NULL); + _dbus_win_cmutex_lock (sitter->mutex); + state = (sitter->child_handle == NULL); + _dbus_win_cmutex_unlock (sitter->mutex); + return state; } /** @@ -311,11 +312,16 @@ _dbus_babysitter_get_child_exit_status (DBusBabysitter *sitter, if (!_dbus_babysitter_get_child_exited (sitter)) _dbus_assert_not_reached ("Child has not exited"); + _dbus_win_cmutex_lock (sitter->mutex); if (!sitter->have_child_status || sitter->child_status == STILL_ACTIVE) - return FALSE; + { + _dbus_win_cmutex_unlock (sitter->mutex); + return FALSE; + } *status = sitter->child_status; + _dbus_win_cmutex_unlock (sitter->mutex); return TRUE; } @@ -337,6 +343,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, return; PING(); + _dbus_win_cmutex_lock (sitter->mutex); if (sitter->have_spawn_errno) { char *emsg = _dbus_win_error_string (sitter->spawn_errno); @@ -360,6 +367,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, sitter->log_name); } PING(); + _dbus_win_cmutex_unlock (sitter->mutex); } dbus_bool_t @@ -370,13 +378,17 @@ _dbus_babysitter_set_watch_functions (DBusBabysitter *sitter, void *data, DBusFreeFunction free_data_function) { + dbus_bool_t result; PING(); - return _dbus_watch_list_set_functions (sitter->watches, - add_function, - remove_function, - toggled_function, - data, - free_data_function); + _dbus_win_cmutex_lock (sitter->mutex); + result = _dbus_watch_list_set_functions (sitter->watches, + add_function, + remove_function, + toggled_function, + data, + free_data_function); + _dbus_win_cmutex_unlock (sitter->mutex); + return result; } static dbus_bool_t @@ -396,9 +408,11 @@ handle_watch (DBusWatch *watch, * struct. */ + _dbus_win_cmutex_lock (sitter->mutex); PING(); close_socket_to_babysitter (sitter); PING(); + _dbus_win_cmutex_unlock (sitter->mutex); if (_dbus_babysitter_get_child_exited (sitter) && sitter->finished_cb != NULL) @@ -583,57 +597,36 @@ spawn_program (char* name, char** argv, char** envp) return pi.hProcess; } - +/* This function is started in a different thread and waits until spawned child exit. */ static DWORD __stdcall babysitter (void *parameter) { int ret = 0; DBusBabysitter *sitter = (DBusBabysitter *) parameter; - HANDLE handle; + DWORD status; + SOCKET sock; PING(); - _dbus_verbose ("babysitter: spawning %s\n", sitter->log_name); + _dbus_verbose ("babysitter: waiting for spawn end of '%s'\n", sitter->log_name); - PING(); - handle = spawn_program (sitter->log_name, sitter->argv, sitter->envp); + // wait until process finished + WaitForSingleObject (sitter->child_handle, INFINITE); PING(); - if (handle != INVALID_HANDLE_VALUE) - { - sitter->child_handle = handle; - } - else - { - sitter->child_handle = NULL; - sitter->have_spawn_errno = TRUE; - sitter->spawn_errno = GetLastError(); - } - - PING(); - SetEvent (sitter->start_sync_event); + ret = GetExitCodeProcess (sitter->child_handle, &status); - if (sitter->child_handle != NULL) + _dbus_win_cmutex_lock (sitter->mutex); + if (ret) { - DWORD status; - - PING(); - // wait until process finished - WaitForSingleObject (sitter->child_handle, INFINITE); - - PING(); - ret = GetExitCodeProcess (sitter->child_handle, &status); - if (ret) - { - sitter->child_status = status; - sitter->have_child_status = TRUE; - } - - CloseHandle (sitter->child_handle); - sitter->child_handle = NULL; + sitter->child_status = status; + sitter->have_child_status = TRUE; } - PING(); - send (sitter->socket_to_main.sock, " ", 1, 0); + CloseHandle (sitter->child_handle); + sitter->child_handle = NULL; + sock = sitter->socket_to_main.sock; + _dbus_win_cmutex_unlock (sitter->mutex); + send (sock, " ", 1, 0); _dbus_babysitter_unref (sitter); @@ -651,6 +644,9 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, { DBusBabysitter *sitter; DWORD sitter_thread_id; + HANDLE handle; + int argc; + char **my_argv = NULL; _DBUS_ASSERT_ERROR_IS_CLEAR (error); _dbus_assert (argv[0] != NULL); @@ -686,7 +682,9 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, if (!_dbus_socketpair (&sitter->socket_to_babysitter, &sitter->socket_to_main, FALSE, error)) - goto out0; + { + goto out0; + } sitter->sitter_watch = _dbus_watch_new (sitter->socket_to_babysitter, DBUS_WATCH_READABLE, @@ -711,17 +709,42 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, goto out0; } - sitter->argc = protect_argv (argv, &sitter->argv); - if (sitter->argc == -1) + argc = protect_argv (argv, &my_argv); + if (argc == -1) { _DBUS_SET_OOM (error); goto out0; } - sitter->envp = envp; + + _dbus_verbose ("babysitter: spawn child '%s'\n", my_argv[0]); PING(); + handle = spawn_program (sitter->log_name, my_argv, envp); + + if (my_argv != NULL) + { + dbus_free_string_array (my_argv); + } + + PING(); + if (handle != INVALID_HANDLE_VALUE) + { + sitter->child_handle = handle; + } + else + { + sitter->child_handle = NULL; + sitter->have_spawn_errno = TRUE; + sitter->spawn_errno = GetLastError(); + dbus_set_error_const (error, DBUS_ERROR_SPAWN_EXEC_FAILED, + "Failed to spawn child"); + goto out0; + } + + PING(); + sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter, - _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); + _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); if (sitter->thread_handle == NULL) { @@ -732,9 +755,6 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, } PING(); - WaitForSingleObject (sitter->start_sync_event, INFINITE); - - PING(); if (sitter_p != NULL) *sitter_p = sitter; else @@ -747,7 +767,6 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, out0: _dbus_babysitter_unref (sitter); - return FALSE; } @@ -756,6 +775,7 @@ _dbus_babysitter_set_result_function (DBusBabysitter *sitter, DBusBabysitterFinishedFunc finished, void *user_data) { + PING(); sitter->finished_cb = finished; sitter->finished_data = user_data; } @@ -765,6 +785,7 @@ _dbus_babysitter_set_result_function (DBusBabysitter *sitter, void _dbus_babysitter_block_for_child_exit (DBusBabysitter *sitter) { + PING(); /* The thread terminates after the child does. We want to wait for the thread, * not just the child, to avoid data races and ensure that it has freed all * its memory. */ -- 1.8.4.5