From a5c51278add34ea57e194c778efb478502578e8f Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Fri, 13 May 2016 00:56:42 +0200 Subject: [PATCH 1/2] Eliminates a race condition accessing DBusBabysitter instance at startup of babysitter() on Windows. Ensure that the babysitter thread already owns its one reference to the babysitter when it starts up, and eliminates the race condition. This patch requires that DBusBabysitter refcounting is thread-safe and is based on an analysis and proposal of Simon Mc Vittie. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=95191 Reviewed-by: Simon McVittie --- dbus/dbus-spawn-win.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index 804aa426..fa290638 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -586,8 +586,6 @@ babysitter (void *parameter) DBusBabysitter *sitter = (DBusBabysitter *) parameter; PING(); - _dbus_babysitter_ref (sitter); - if (sitter->child_setup) { PING(); @@ -728,7 +726,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter **sitter_p, PING(); sitter_thread = (HANDLE) CreateThread (NULL, 0, babysitter, - sitter, 0, &sitter_thread_id); + _dbus_babysitter_ref (sitter), 0, &sitter_thread_id); if (sitter_thread == 0) { From baeea825a4a84858f7df2755853e0b7d2d2c91e7 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Tue, 10 May 2016 16:53:57 +0200 Subject: [PATCH 2/2] On Windows make access to member 'refcount' of struct DBusBabysitter thread safe. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=95191 Reviewed-by: Simon McVittie --- dbus/dbus-internals.h | 1 + dbus/dbus-spawn-win.c | 35 ++++++++++++++++++++++++++++------- dbus/dbus-sysdeps.h | 3 +++ 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index df8e9643..5e6efd81 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -124,6 +124,7 @@ static void _dbus_verbose(const char * x,...) {;} # define _dbus_is_verbose() FALSE #endif /* !DBUS_ENABLE_VERBOSE_MODE */ +DBUS_PRIVATE_EXPORT void _dbus_trace_ref (const char *obj_name, void *obj, int old_refcount, diff --git a/dbus/dbus-spawn-win.c b/dbus/dbus-spawn-win.c index fa290638..cbd26f18 100644 --- a/dbus/dbus-spawn-win.c +++ b/dbus/dbus-spawn-win.c @@ -60,7 +60,7 @@ */ struct DBusBabysitter { - int refcount; + DBusAtomic refcount; HANDLE start_sync_event; #ifdef DBUS_ENABLE_EMBEDDED_TESTS @@ -91,16 +91,33 @@ struct DBusBabysitter int child_status; }; +static void +_dbus_babysitter_trace_ref (DBusBabysitter *sitter, + int old_refcount, + int new_refcount, + const char *why) +{ +#ifdef DBUS_ENABLE_VERBOSE_MODE + static int enabled = -1; + + _dbus_trace_ref ("DBusBabysitter", sitter, old_refcount, new_refcount, why, + "DBUS_BABYSITTER_TRACE", &enabled); +#endif +} + static DBusBabysitter* _dbus_babysitter_new (void) { DBusBabysitter *sitter; + dbus_int32_t old_refcount; sitter = dbus_new0 (DBusBabysitter, 1); if (sitter == NULL) return NULL; - sitter->refcount = 1; + 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) @@ -148,11 +165,13 @@ _dbus_babysitter_new (void) DBusBabysitter * _dbus_babysitter_ref (DBusBabysitter *sitter) { + dbus_int32_t old_refcount; PING(); _dbus_assert (sitter != NULL); - _dbus_assert (sitter->refcount > 0); - sitter->refcount += 1; + old_refcount = _dbus_atomic_inc (&sitter->refcount); + _dbus_assert (old_refcount > 0); + _dbus_babysitter_trace_ref (sitter, old_refcount, old_refcount+1, __FUNCTION__); return sitter; } @@ -187,14 +206,16 @@ void _dbus_babysitter_unref (DBusBabysitter *sitter) { int i; + dbus_int32_t old_refcount; PING(); _dbus_assert (sitter != NULL); - _dbus_assert (sitter->refcount > 0); - sitter->refcount -= 1; + old_refcount = _dbus_atomic_dec (&sitter->refcount); + _dbus_assert (old_refcount > 0); + _dbus_babysitter_trace_ref (sitter, old_refcount, old_refcount-1, __FUNCTION__); - if (sitter->refcount == 0) + if (old_refcount == 1) { close_socket_to_babysitter (sitter); diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index 2f493b09..2d152b8c 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -295,8 +295,11 @@ struct DBusAtomic # undef DBUS_HAVE_ATOMIC_INT #endif +DBUS_PRIVATE_EXPORT dbus_int32_t _dbus_atomic_inc (DBusAtomic *atomic); +DBUS_PRIVATE_EXPORT dbus_int32_t _dbus_atomic_dec (DBusAtomic *atomic); +DBUS_PRIVATE_EXPORT dbus_int32_t _dbus_atomic_get (DBusAtomic *atomic); #ifdef DBUS_WIN