17 files changed, 196 insertions, 8 deletions
diff --git a/dbus/dbus-bus.c b/dbus/dbus-bus.c
index 1de9ca47..35fc135e 100644
--- a/dbus/dbus-bus.c
+++ b/dbus/dbus-bus.c
@@ -83,17 +83,18 @@ typedef struct
 } BusData;
 
 /** The slot we have reserved to store BusData.
+ * Protected by _DBUS_LOCK_connection_slots.
  */
 static dbus_int32_t bus_data_slot = -1;
 
 /** Number of bus types */
 #define N_BUS_TYPES 3
 
+/* Protected by _DBUS_LOCK_bus, except during shutdown, which can't safely
+ * be done in a threaded application anyway. */
 static DBusConnection *bus_connections[N_BUS_TYPES];
 static char *bus_connection_addresses[N_BUS_TYPES] = { NULL, NULL, NULL };
-
 static DBusBusType activation_bus_type = DBUS_BUS_STARTER;
-
 static dbus_bool_t initialized = FALSE;
 
 static void
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 0b6a0c6d..159dbe1b 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -245,9 +245,9 @@ struct DBusPreallocatedSend
 };
 
 #if HAVE_DECL_MSG_NOSIGNAL
-static dbus_bool_t _dbus_modify_sigpipe = FALSE;
+static DBusAtomic _dbus_modify_sigpipe = { FALSE };
 #else
-static dbus_bool_t _dbus_modify_sigpipe = TRUE;
+static DBusAtomic _dbus_modify_sigpipe = { TRUE };
 #endif
 
 /**
@@ -1328,7 +1328,7 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
   if (objects == NULL)
     goto error;
   
-  if (_dbus_modify_sigpipe)
+  if (_dbus_atomic_get (&_dbus_modify_sigpipe) != 0)
     _dbus_disable_sigpipe ();
 
   /* initialized to 0: use atomic op to avoid mixing atomic and non-atomic */
@@ -6115,8 +6115,11 @@ dbus_connection_get_data (DBusConnection   *connection,
  */
 void
 dbus_connection_set_change_sigpipe (dbus_bool_t will_modify_sigpipe)
-{  
-  _dbus_modify_sigpipe = will_modify_sigpipe != FALSE;
+{
+  if (will_modify_sigpipe)
+    _dbus_atomic_set_nonzero (&_dbus_modify_sigpipe);
+  else
+    _dbus_atomic_set_zero (&_dbus_modify_sigpipe);
 }
 
 /**
diff --git a/dbus/dbus-dataslot.c b/dbus/dbus-dataslot.c
index 18e20f60..a4503a29 100644
--- a/dbus/dbus-dataslot.c
+++ b/dbus/dbus-dataslot.c
@@ -353,6 +353,7 @@ _dbus_data_slot_list_free (DBusDataSlotList *list)
 #include "dbus-test.h"
 #include <stdio.h>
 
+/* Test-only, does not need to be thread-safe */
 static int free_counter;
 
 static void
diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c
index 7420f203..7eb85501 100644
--- a/dbus/dbus-internals.c
+++ b/dbus/dbus-internals.c
@@ -184,7 +184,14 @@
  */
 const char *_dbus_no_memory_message = "Not enough memory";
 
+/* Not necessarily thread-safe, but if writes don't propagate between
+ * threads, the worst that will happen is that we duplicate work in more than
+ * one thread. */
 static dbus_bool_t warn_initted = FALSE;
+
+/* Not necessarily thread-safe, but if writes don't propagate between
+ * threads, the worst that will happen is that warnings get their default
+ * fatal/non-fatal nature. */
 static dbus_bool_t fatal_warnings = FALSE;
 static dbus_bool_t fatal_warnings_on_check_failed = TRUE;
 
diff --git a/dbus/dbus-memory.c b/dbus/dbus-memory.c
index 98869d57..5468561a 100644
--- a/dbus/dbus-memory.c
+++ b/dbus/dbus-memory.c
@@ -99,6 +99,7 @@
  */
 
 #ifdef DBUS_ENABLE_EMBEDDED_TESTS
+/* Test-only, does not need to be thread-safe */
 static dbus_bool_t debug_initialized = FALSE;
 static int fail_nth = -1;
 static size_t fail_size = 0;
diff --git a/dbus/dbus-pending-call.c b/dbus/dbus-pending-call.c
index d057b49b..84fdcb58 100644
--- a/dbus/dbus-pending-call.c
+++ b/dbus/dbus-pending-call.c
@@ -101,6 +101,7 @@ _dbus_pending_call_trace_ref (DBusPendingCall *pending_call,
 #endif
 }
 
+/* protected by _DBUS_LOCK_pending_call_slots */
 static dbus_int32_t notify_user_data_slot = -1;
 
 /**
diff --git a/dbus/dbus-spawn-unix.c b/dbus/dbus-spawn-unix.c
index 742c3a7a..5550ffac 100644
--- a/dbus/dbus-spawn-unix.c
+++ b/dbus/dbus-spawn-unix.c
@@ -1153,6 +1153,8 @@ check_babysit_events (pid_t grandchild_pid,
     }
 }
 
+/* Only used in a single-threaded child process, does not need to be
+ * thread-safe */
 static int babysit_sigchld_pipe = -1;
 
 static void
diff --git a/dbus/dbus-sysdeps-thread-win.c b/dbus/dbus-sysdeps-thread-win.c
index db389825..3b600fa9 100644
--- a/dbus/dbus-sysdeps-thread-win.c
+++ b/dbus/dbus-sysdeps-thread-win.c
@@ -32,6 +32,7 @@
 
 #include <windows.h>
 
+/* Protected by DllMain lock, effectively */
 static dbus_bool_t global_init_done = FALSE;
 static CRITICAL_SECTION init_lock;
 
@@ -54,7 +55,7 @@ struct DBusCondVar {
 
 static DWORD dbus_cond_event_tls = TLS_OUT_OF_INDEXES;
 
-
+/* Protected by DllMain lock, effectively */
 static HMODULE dbus_dll_hmodule;
 
 void *
diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
index 1eb54bd4..c91c05ef 100644
--- a/dbus/dbus-sysdeps-unix.c
+++ b/dbus/dbus-sysdeps-unix.c
@@ -3072,6 +3072,42 @@ _dbus_atomic_get (DBusAtomic *atomic)
 }
 
 /**
+ * Atomically set the value of an integer to 0.
+ *
+ * @param atomic pointer to the integer to set
+ */
+void
+_dbus_atomic_set_zero (DBusAtomic *atomic)
+{
+#if DBUS_USE_SYNC
+  /* Atomic version of "*atomic &= 0; return *atomic" */
+  __sync_and_and_fetch (&atomic->value, 0);
+#else
+  pthread_mutex_lock (&atomic_mutex);
+  atomic->value = 0;
+  pthread_mutex_unlock (&atomic_mutex);
+#endif
+}
+
+/**
+ * Atomically set the value of an integer to something nonzero.
+ *
+ * @param atomic pointer to the integer to set
+ */
+void
+_dbus_atomic_set_nonzero (DBusAtomic *atomic)
+{
+#if DBUS_USE_SYNC
+  /* Atomic version of "*atomic |= 1; return *atomic" */
+  __sync_or_and_fetch (&atomic->value, 1);
+#else
+  pthread_mutex_lock (&atomic_mutex);
+  atomic->value = 1;
+  pthread_mutex_unlock (&atomic_mutex);
+#endif
+}
+
+/**
  * Wrapper for poll().
  *
  * @param fds the file descriptors to poll
diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c
index ee8b9678..c5a9d016 100644
--- a/dbus/dbus-sysdeps-win.c
+++ b/dbus/dbus-sysdeps-win.c
@@ -103,6 +103,10 @@ typedef MIB_TCPROW_OWNER_PID _MIB_TCPROW_EX;
 typedef MIB_TCPTABLE_OWNER_PID MIB_TCPTABLE_EX;
 typedef PMIB_TCPTABLE_OWNER_PID PMIB_TCPTABLE_EX;
 typedef DWORD (WINAPI *ProcAllocateAndGetTcpExtTableFromStack)(PMIB_TCPTABLE_EX*,BOOL,HANDLE,DWORD,DWORD);
+
+/* Not protected by a lock, but if we miss a write, all that
+ * happens is that the lazy initialization will happen in two threads
+ * concurrently - it results in the same value either way so that's OK */
 static ProcAllocateAndGetTcpExtTableFromStack lpfnAllocateAndGetTcpExTableFromStack = NULL;
 
 /**
@@ -3295,6 +3299,28 @@ _dbus_atomic_get (DBusAtomic *atomic)
 }
 
 /**
+ * Atomically set the value of an integer to 0.
+ *
+ * @param atomic pointer to the integer to set
+ */
+void
+_dbus_atomic_set_zero (DBusAtomic *atomic)
+{
+  InterlockedExchange (&atomic->value, 0);
+}
+
+/**
+ * Atomically set the value of an integer to something nonzero.
+ *
+ * @param atomic pointer to the integer to set
+ */
+void
+_dbus_atomic_set_nonzero (DBusAtomic *atomic)
+{
+  InterlockedExchange (&atomic->value, 1);
+}
+
+/**
  * Called when the bus daemon is signaled to reload its configuration; any
  * caches should be nuked. Of course any caches that need explicit reload
  * are probably broken, but c'est la vie.
diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h
index 7c420426..60091026 100644
--- a/dbus/dbus-sysdeps.h
+++ b/dbus/dbus-sysdeps.h
@@ -335,6 +335,10 @@ DBUS_PRIVATE_EXPORT
 dbus_int32_t _dbus_atomic_dec (DBusAtomic *atomic);
 DBUS_PRIVATE_EXPORT
 dbus_int32_t _dbus_atomic_get (DBusAtomic *atomic);
+DBUS_PRIVATE_EXPORT
+void         _dbus_atomic_set_zero    (DBusAtomic *atomic);
+DBUS_PRIVATE_EXPORT
+void         _dbus_atomic_set_nonzero (DBusAtomic *atomic);
 
 #ifdef DBUS_WIN
 
diff --git a/dbus/dbus-test-tap.h b/dbus/dbus-test-tap.h
index ea116921..423cf48e 100644
--- a/dbus/dbus-test-tap.h
+++ b/dbus/dbus-test-tap.h
@@ -59,6 +59,8 @@ void _dbus_test_check_memleaks (const char *test_name);
 DBUS_PRIVATE_EXPORT
 int _dbus_test_done_testing (void);
 
+#define _dbus_test_check(a) if (!(a)) _dbus_test_not_ok ("%s:%d - '%s' failed\n", __FILE__, __LINE__, #a)
+
 #endif
 
 #endif
diff --git a/dbus/dbus-threads.c b/dbus/dbus-threads.c
index 3ebe9e70..8d7c03cd 100644
--- a/dbus/dbus-threads.c
+++ b/dbus/dbus-threads.c
@@ -26,6 +26,7 @@
 #include "dbus-threads-internal.h"
 #include "dbus-list.h"
 
+/* Protected by _dbus_threads_lock_platform_specific() */
 static int thread_init_generation = 0;
 
 /**
@@ -283,6 +284,7 @@ _dbus_condvar_wake_one (DBusCondVar *cond)
   _dbus_platform_condvar_wake_one (cond);
 }
 
+/* Protected by _dbus_threads_lock_platform_specific() */
 static DBusRMutex *global_locks[_DBUS_N_GLOBAL_LOCKS] = { NULL };
 
 static void
diff --git a/dbus/dbus-userdb.c b/dbus/dbus-userdb.c
index 66682748..d3225371 100644
--- a/dbus/dbus-userdb.c
+++ b/dbus/dbus-userdb.c
@@ -222,6 +222,7 @@ _dbus_user_database_lookup (DBusUserDatabase *db,
     }
 }
 
+/* Protected by _DBUS_LOCK_system_users */
 static dbus_bool_t database_locked = FALSE;
 static DBusUserDatabase *system_db = NULL;
 static DBusString process_username;
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 08e7a8b0..d0cc8d4d 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -39,6 +39,10 @@ set(test-shell_SOURCES
     shell-test.c
 )
 
+set(test-atomic_SOURCES
+    internals/atomic.c
+)
+
 set(test-spawn_SOURCES
     spawn-test.c
 )
@@ -92,6 +96,7 @@ if(WIN32)
 endif()
 
 if(DBUS_ENABLE_EMBEDDED_TESTS)
+    add_test_executable(test-atomic ${test-atomic_SOURCES} dbus-testutils)
     add_test_executable(test-hash internals/hash.c dbus-testutils)
     set_target_properties(test-hash PROPERTIES COMPILE_FLAGS ${DBUS_INTERNAL_CLIENT_DEFINITIONS})
 
diff --git a/test/Makefile.am b/test/Makefile.am
index 28be5cdd..9566f900 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -85,6 +85,7 @@ TEST_BINARIES               += test-spawn
 endif
 
 uninstallable_test_programs += \
+	test-atomic \
 	test-bus \
 	test-bus-dispatch \
 	test-bus-dispatch-sha1 \
@@ -106,6 +107,9 @@ endif DBUS_UNIX
 noinst_PROGRAMS += $(uninstallable_test_programs)
 TESTS += $(uninstallable_test_programs)
 
+test_atomic_SOURCES = internals/atomic.c
+test_atomic_LDADD = libdbus-testutils.la
+
 else !DBUS_ENABLE_EMBEDDED_TESTS
 
 TEST_BINARIES=
diff --git a/test/internals/atomic.c b/test/internals/atomic.c
new file mode 100644
index 00000000..d595dc17
--- /dev/null
+++ b/test/internals/atomic.c
@@ -0,0 +1,91 @@
+/* Regression test for atomic ops
+ *
+ * Author: Simon McVittie <simon.mcvittie@collabora.co.uk>
+ * Copyright © 2013 Collabora Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation files
+ * (the "Software"), to deal in the Software without restriction,
+ * including without limitation the rights to use, copy, modify, merge,
+ * publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <config.h>
+
+#include <dbus/dbus.h>
+#include <dbus/dbus-internals.h>
+#include <dbus/dbus-sysdeps.h>
+#include <dbus/dbus-test-tap.h>
+#include <test/test-utils.h>
+
+static dbus_bool_t
+atomic_test_inc_dec (const char *test_data_dir _DBUS_GNUC_UNUSED)
+{
+  DBusAtomic a = { 0 };
+  DBusAtomic b = { 123 };
+  dbus_int32_t old;
+
+  _dbus_test_check (_dbus_atomic_get (&a) == 0);
+  _dbus_test_check (_dbus_atomic_get (&b) == 123);
+
+  _dbus_test_check (a.value == 0);
+  old = _dbus_atomic_dec (&a);
+  _dbus_test_check (old == 0);
+  _dbus_test_check (a.value == -1);
+
+  _dbus_test_check (b.value == 123);
+  old = _dbus_atomic_inc (&b);
+  _dbus_test_check (old == 123);
+  _dbus_test_check (b.value == 124);
+  return TRUE;
+}
+
+static dbus_bool_t
+atomic_test_zero (const char *test_data_dir _DBUS_GNUC_UNUSED)
+{
+  DBusAtomic a = { 0 };
+  DBusAtomic b = { 123 };
+
+  _dbus_atomic_set_nonzero (&a);
+  /* careful: this is not necessarily 1 */
+  _dbus_test_check (a.value != 0);
+
+  _dbus_atomic_set_nonzero (&b);
+  _dbus_test_check (b.value != 0);
+
+  _dbus_atomic_set_zero (&a);
+  _dbus_test_check (a.value == 0);
+
+  _dbus_atomic_set_zero (&b);
+  _dbus_test_check (b.value == 0);
+  return TRUE;
+}
+
+static DBusTestCase tests[] =
+{
+  { "atomic_inc/dec", atomic_test_inc_dec },
+  { "atomic_set_[non]zero", atomic_test_zero },
+  { NULL }
+};
+
+int
+main (int argc, char **argv)
+{
+  return _dbus_test_main (argc, argv, _DBUS_N_ELEMENTS (tests), tests,
+                          DBUS_TEST_FLAGS_NONE,
+                          NULL, NULL);
+}