From 424db582c4c2d9e4f86b7c1c68b1d7c3c95f3bf3 Mon Sep 17 00:00:00 2001
From: Simon McVittie <simon.mcvittie@collabora.co.uk>
Date: Tue, 7 Jun 2011 13:48:05 +0100
Subject: [PATCH] Consistently use atomic operations for all access to
 DBusConnection refcount

Trying to mix atomic operations with locked non-atomic operations is
broken: the atomic ops aren't necessarily atomic with respect to the
locked non-atomic ops, and the non-atomic ops aren't protected by the
lock because the atomic ops can change the refcount behind their back.

In theory we could use the connection lock if atomic ops aren't supported
(making a per-connection lock cheaper than the global lock used to
implement atomic ops) *and* our mutexes are recursive (making it safe
against deadlocks)... but life's too short.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=38005
---
 dbus/dbus-connection.c |   82 +++++++++--------------------------------------
 1 files changed, 16 insertions(+), 66 deletions(-)

diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index d0aca25..e81ed76 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -1399,13 +1399,8 @@ _dbus_connection_ref_unlocked (DBusConnection *connection)
   _dbus_assert (connection->generation == _dbus_current_generation);
 
   HAVE_LOCK_CHECK (connection);
-  
-#ifdef DBUS_HAVE_ATOMIC_INT
+
   _dbus_atomic_inc (&connection->refcount);
-#else
-  _dbus_assert (connection->refcount.value > 0);
-  connection->refcount.value += 1;
-#endif
 
   return connection;
 }
@@ -1425,22 +1420,8 @@ _dbus_connection_unref_unlocked (DBusConnection *connection)
   
   _dbus_assert (connection != NULL);
 
-  /* The connection lock is better than the global
-   * lock in the atomic increment fallback
-   */
-  
-#ifdef DBUS_HAVE_ATOMIC_INT
   last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
-#else
-  _dbus_assert (connection->refcount.value > 0);
 
-  connection->refcount.value -= 1;
-  last_unref = (connection->refcount.value == 0);  
-#if 0
-  printf ("unref_unlocked() connection %p count = %d\n", connection, connection->refcount.value);
-#endif
-#endif
-  
   if (last_unref)
     _dbus_connection_last_unref (connection);
 }
@@ -2126,11 +2107,21 @@ _dbus_connection_send_and_unlock (DBusConnection *connection,
 void
 _dbus_connection_close_if_only_one_ref (DBusConnection *connection)
 {
+  dbus_int32_t tmp_refcount;
+
   CONNECTION_LOCK (connection);
-  
-  _dbus_assert (connection->refcount.value > 0);
 
-  if (connection->refcount.value == 1)
+  _dbus_atomic_inc (&connection->refcount);
+
+  tmp_refcount = _dbus_atomic_dec (&connection->refcount);
+
+  /* The caller should have one ref, and this function temporarily took
+   * one more, which is reflected in this count even though we already
+   * released it (relying on the caller's ref) due to _dbus_atomic_dec
+   * semantics */
+  _dbus_assert (tmp_refcount >= 2);
+
+  if (tmp_refcount == 2)
     _dbus_connection_close_possibly_shared_and_unlock (connection);
   else
     CONNECTION_UNLOCK (connection);
@@ -2632,25 +2623,8 @@ dbus_connection_ref (DBusConnection *connection)
 {
   _dbus_return_val_if_fail (connection != NULL, NULL);
   _dbus_return_val_if_fail (connection->generation == _dbus_current_generation, NULL);
-  
-  /* The connection lock is better than the global
-   * lock in the atomic increment fallback
-   *
-   * (FIXME but for now we always use the atomic version,
-   * to avoid taking the connection lock, due to
-   * the mess with set_timeout_functions()/set_watch_functions()
-   * calling out to the app without dropping locks)
-   */
-  
-#if 1
-  _dbus_atomic_inc (&connection->refcount);
-#else
-  CONNECTION_LOCK (connection);
-  _dbus_assert (connection->refcount.value > 0);
 
-  connection->refcount.value += 1;
-  CONNECTION_UNLOCK (connection);
-#endif
+  _dbus_atomic_inc (&connection->refcount);
 
   return connection;
 }
@@ -2787,33 +2761,9 @@ dbus_connection_unref (DBusConnection *connection)
 
   _dbus_return_if_fail (connection != NULL);
   _dbus_return_if_fail (connection->generation == _dbus_current_generation);
-  
-  /* The connection lock is better than the global
-   * lock in the atomic increment fallback
-   *
-   * (FIXME but for now we always use the atomic version,
-   * to avoid taking the connection lock, due to
-   * the mess with set_timeout_functions()/set_watch_functions()
-   * calling out to the app without dropping locks)
-   */
-  
-#if 1
-  last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
-#else
-  CONNECTION_LOCK (connection);
-  
-  _dbus_assert (connection->refcount.value > 0);
 
-  connection->refcount.value -= 1;
-  last_unref = (connection->refcount.value == 0);
+  last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
 
-#if 0
-  printf ("unref() connection %p count = %d\n", connection, connection->refcount.value);
-#endif
-  
-  CONNECTION_UNLOCK (connection);
-#endif
-  
   if (last_unref)
     {
 #ifndef DBUS_DISABLE_CHECKS
-- 
1.7.5.3