From 8d50f1f43df66b6f743972471e73c87f722eb875 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Tue, 17 May 2016 16:39:02 +0200 Subject: [PATCH] Use mutex to protect access to DBusBabysitter struct members on Windows. We are using the dbus provided mutex wrapper instead of native funtions to avoid confusion of WaitForSingleObject() calls used for events too. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=95191 --- dbus/dbus-spawn-win.c | 84 ++++++++++++++++++++++++++++++++++---------- dbus/dbus-threads-internal.h | 4 +++ 2 files changed, 70 insertions(+), 18 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index f9a4207..03dc351 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -61,6 +61,7 @@ struct DBusBabysitter { DBusAtomic refcount; + DBusCMutex *mutex; HANDLE start_sync_event; @@ -110,6 +111,8 @@ _dbus_babysitter_new (void) if (sitter == NULL) return NULL; + _dbus_cmutex_new_at_location (&sitter->mutex); + old_refcount = _dbus_atomic_inc (&sitter->refcount); _dbus_babysitter_trace_ref (sitter, old_refcount, old_refcount+1, __FUNCTION__); @@ -203,6 +206,7 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) if (old_refcount == 1) { + _dbus_cmutex_lock (sitter->mutex); close_socket_to_babysitter (sitter); if (sitter->socket_to_main.sock != INVALID_SOCKET) @@ -264,7 +268,8 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) } dbus_free (sitter->log_name); - + _dbus_cmutex_unlock (sitter->mutex); + _dbus_cmutex_free_at_location (&sitter->mutex); dbus_free (sitter); } } @@ -272,12 +277,16 @@ _dbus_babysitter_unref (DBusBabysitter *sitter) void _dbus_babysitter_kill_child (DBusBabysitter *sitter) { + HANDLE handle; PING(); - if (sitter->child_handle == NULL) + _dbus_cmutex_lock (sitter->mutex); + handle = sitter->child_handle; + _dbus_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 +297,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_cmutex_lock (sitter->mutex); + state = sitter->child_handle == NULL; + _dbus_cmutex_unlock (sitter->mutex); + return state; } /** @@ -311,11 +324,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_cmutex_lock (sitter->mutex); if (!sitter->have_child_status || sitter->child_status == STILL_ACTIVE) - return FALSE; + { + _dbus_cmutex_unlock (sitter->mutex); + return FALSE; + } *status = sitter->child_status; + _dbus_cmutex_unlock (sitter->mutex); return TRUE; } @@ -337,6 +355,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, return; PING(); + _dbus_cmutex_lock (sitter->mutex); if (sitter->have_spawn_errno) { char *emsg = _dbus_win_error_string (sitter->spawn_errno); @@ -360,6 +379,7 @@ _dbus_babysitter_set_child_exit_error (DBusBabysitter *sitter, sitter->log_name); } PING(); + _dbus_cmutex_unlock (sitter->mutex); } dbus_bool_t @@ -370,13 +390,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_cmutex_lock (sitter->mutex); + result = _dbus_watch_list_set_functions (sitter->watches, + add_function, + remove_function, + toggled_function, + data, + free_data_function); + _dbus_cmutex_unlock (sitter->mutex); + return result; } static dbus_bool_t @@ -396,6 +420,7 @@ handle_watch (DBusWatch *watch, * struct. */ + _dbus_cmutex_lock (sitter->mutex); PING(); close_socket_to_babysitter (sitter); PING(); @@ -406,6 +431,7 @@ handle_watch (DBusWatch *watch, sitter->finished_cb (sitter, sitter->finished_data); sitter->finished_cb = NULL; } + _dbus_cmutex_unlock (sitter->mutex); return TRUE; } @@ -591,6 +617,7 @@ babysitter (void *parameter) DBusBabysitter *sitter = (DBusBabysitter *) parameter; HANDLE handle; + _dbus_cmutex_lock (sitter->mutex); PING(); _dbus_verbose ("babysitter: spawning %s\n", sitter->log_name); @@ -611,17 +638,19 @@ babysitter (void *parameter) PING(); SetEvent (sitter->start_sync_event); + _dbus_cmutex_unlock (sitter->mutex); - if (sitter->child_handle != NULL) + if (handle != INVALID_HANDLE_VALUE) { DWORD status; PING(); // wait until process finished - WaitForSingleObject (sitter->child_handle, INFINITE); + WaitForSingleObject (handle, INFINITE); PING(); - ret = GetExitCodeProcess (sitter->child_handle, &status); + ret = GetExitCodeProcess (handle, &status); + _dbus_cmutex_lock (sitter->mutex); if (ret) { sitter->child_status = status; @@ -630,10 +659,13 @@ babysitter (void *parameter) CloseHandle (sitter->child_handle); sitter->child_handle = NULL; + _dbus_cmutex_unlock (sitter->mutex); } PING(); + _dbus_cmutex_lock (sitter->mutex); send (sitter->socket_to_main.sock, " ", 1, 0); + _dbus_cmutex_unlock (sitter->mutex); _dbus_babysitter_unref (sitter); @@ -651,6 +683,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, { DBusBabysitter *sitter; DWORD sitter_thread_id; + HANDLE event; _DBUS_ASSERT_ERROR_IS_CLEAR (error); _dbus_assert (argv[0] != NULL); @@ -666,10 +699,12 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, return FALSE; } + _dbus_cmutex_lock (sitter->mutex); sitter->log_name = _dbus_strdup (log_name); if (sitter->log_name == NULL && log_name != NULL) { _DBUS_SET_OOM (error); + _dbus_cmutex_unlock (sitter->mutex); goto out0; } @@ -679,6 +714,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, if (sitter->log_name == NULL) { _DBUS_SET_OOM (error); + _dbus_cmutex_unlock (sitter->mutex); goto out0; } @@ -686,7 +722,10 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, if (!_dbus_socketpair (&sitter->socket_to_babysitter, &sitter->socket_to_main, FALSE, error)) - goto out0; + { + _dbus_cmutex_unlock (sitter->mutex); + goto out0; + } sitter->sitter_watch = _dbus_watch_new (sitter->socket_to_babysitter, DBUS_WATCH_READABLE, @@ -695,6 +734,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, if (sitter->sitter_watch == NULL) { _DBUS_SET_OOM (error); + _dbus_cmutex_unlock (sitter->mutex); goto out0; } @@ -708,6 +748,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, sitter->sitter_watch = NULL; _DBUS_SET_OOM (error); + _dbus_cmutex_unlock (sitter->mutex); goto out0; } @@ -715,10 +756,12 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, if (sitter->argc == -1) { _DBUS_SET_OOM (error); + _dbus_cmutex_unlock (sitter->mutex); goto out0; } sitter->envp = envp; + _dbus_cmutex_unlock (sitter->mutex); PING(); sitter->thread_handle = (HANDLE) CreateThread (NULL, 0, babysitter, _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); @@ -732,7 +775,10 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, } PING(); - WaitForSingleObject (sitter->start_sync_event, INFINITE); + _dbus_cmutex_lock (sitter->mutex); + event = sitter->start_sync_event; + _dbus_cmutex_unlock (sitter->mutex); + WaitForSingleObject (event, INFINITE); PING(); if (sitter_p != NULL) @@ -756,8 +802,11 @@ _dbus_babysitter_set_result_function (DBusBabysitter *sitter, DBusBabysitterFinishedFunc finished, void *user_data) { + PING(); + _dbus_cmutex_lock (sitter->mutex); sitter->finished_cb = finished; sitter->finished_data = user_data; + _dbus_cmutex_unlock (sitter->mutex); } #ifdef DBUS_ENABLE_EMBEDDED_TESTS @@ -786,11 +835,10 @@ get_test_exec (const char *exe, return _dbus_string_get_data (scratch_space); } -#define LIVE_CHILDREN(sitter) ((sitter)->child_handle != NULL) - static 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. */ diff --git a/dbus/dbus-threads-internal.h b/dbus/dbus-threads-internal.h index 076acd2..29edad5 100644 --- a/dbus/dbus-threads-internal.h +++ b/dbus/dbus-threads-internal.h @@ -55,9 +55,13 @@ void _dbus_rmutex_unlock (DBusRMutex *mutex); void _dbus_rmutex_new_at_location (DBusRMutex **location_p); void _dbus_rmutex_free_at_location (DBusRMutex **location_p); +DBUS_PRIVATE_EXPORT void _dbus_cmutex_lock (DBusCMutex *mutex); +DBUS_PRIVATE_EXPORT void _dbus_cmutex_unlock (DBusCMutex *mutex); +DBUS_PRIVATE_EXPORT void _dbus_cmutex_new_at_location (DBusCMutex **location_p); +DBUS_PRIVATE_EXPORT void _dbus_cmutex_free_at_location (DBusCMutex **location_p); DBusCondVar* _dbus_condvar_new (void); -- 1.8.4.5