From 722a820f6d9ca70e10e4a990eb42d7cfd24fda71 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Thu, 14 Jul 2016 19:41:24 +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 | 220 ++++++++++++++++++++++++-------------------------- 1 file changed, 106 insertions(+), 114 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 5cb3044..479f41d 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -60,30 +60,25 @@ */ struct DBusBabysitter { - DBusAtomic refcount; - - HANDLE start_sync_event; - - char *log_name; + DBusCMutex *mutex; - int argc; - char **argv; - char **envp; - - HANDLE thread_handle; + /* used by both threads and guarded by @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 +105,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 = CreateMutex (NULL, FALSE, NULL); + 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 +156,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 +182,6 @@ close_socket_to_babysitter (DBusBabysitter *sitter) void _dbus_babysitter_unref (DBusBabysitter *sitter) { - int i; dbus_int32_t old_refcount; PING(); @@ -203,6 +193,7 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) if (old_refcount == 1) { + WaitForSingleObject (sitter->mutex, INFINITE); close_socket_to_babysitter (sitter); if (sitter->socket_to_main.sock != INVALID_SOCKET) @@ -211,29 +202,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 +218,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 +226,9 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) dbus_free (sitter->log_name); + ReleaseMutex (sitter->mutex); + CloseHandle (sitter->mutex); + dbus_free (sitter); } } @@ -273,11 +237,16 @@ void _dbus_babysitter_kill_child (DBusBabysitter *sitter) { PING(); + WaitForSingleObject (sitter->mutex, INFINITE); + if (sitter->child_handle == NULL) - return; /* child is already dead, or we're so hosed we'll never recover */ + goto out; PING(); TerminateProcess (sitter->child_handle, 12345); + +out: + ReleaseMutex (sitter->mutex); } /** @@ -288,8 +257,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); + WaitForSingleObject (sitter->mutex, INFINITE); + state = (sitter->child_handle == NULL); + ReleaseMutex (sitter->mutex); + return state; } /** @@ -308,14 +281,19 @@ dbus_bool_t _dbus_babysitter_get_child_exit_status (DBusBabysitter *sitter, int *status) { + WaitForSingleObject (sitter->mutex, INFINITE); if (!_dbus_babysitter_get_child_exited (sitter)) _dbus_assert_not_reached ("Child has not exited"); if (!sitter->have_child_status || sitter->child_status == STILL_ACTIVE) - return FALSE; + { + ReleaseMutex (sitter->mutex); + return FALSE; + } *status = sitter->child_status; + ReleaseMutex (sitter->mutex); return TRUE; } @@ -337,6 +315,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, return; PING(); + WaitForSingleObject (sitter->mutex, INFINITE); if (sitter->have_spawn_errno) { char *emsg = _dbus_win_error_string (sitter->spawn_errno); @@ -360,6 +339,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, sitter->log_name); } PING(); + ReleaseMutex (sitter->mutex); } dbus_bool_t @@ -370,13 +350,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); + WaitForSingleObject (sitter->mutex, INFINITE); + result = _dbus_watch_list_set_functions (sitter->watches, + add_function, + remove_function, + toggled_function, + data, + free_data_function); + ReleaseMutex (sitter->mutex); + return result; } static dbus_bool_t @@ -396,9 +380,11 @@ handle_watch (DBusWatch *watch, * struct. */ + WaitForSingleObject (sitter->mutex, INFINITE); PING(); close_socket_to_babysitter (sitter); PING(); + ReleaseMutex (sitter->mutex); if (_dbus_babysitter_get_child_exited (sitter) && sitter->finished_cb != NULL) @@ -583,58 +569,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) + WaitForSingleObject (sitter->mutex, INFINITE); + 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; + send (sock, " ", 1, 0); + ReleaseMutex (sitter->mutex); _dbus_babysitter_unref (sitter); return ret ? 0 : 1; @@ -651,6 +615,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 +653,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 +680,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 +726,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 +738,6 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, out0: _dbus_babysitter_unref (sitter); - return FALSE; } @@ -756,6 +746,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 +756,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. */ -- 2.6.6