Merge branch 'dbus-1.4'

6 files changed, 76 insertions, 33 deletions

diff --git a/NEWS b/NEWS
index bcb4358e..1678fede 100644
--- a/NEWS
+++ b/NEWS
@@ -16,9 +16,10 @@ Other changes:
   or dbus_connection_try_register_fallback fails, not ...ADDRESS_IN_USE,
   and simplify object-path registration (fd.o #38874, Jiří Klimeš)
 
-• Consistently use atomic operations on the refcounts of DBusPendingCall
-  and DBusServer, as was done for DBusConnection in 1.4.12 (fd.o #38005,
-  Simon McVittie)
+• Consistently use atomic operations on the refcounts of DBusPendingCall,
+  DBusServer, DBusMessageFilter and DBusObjectTree, as was done for
+  DBusConnection in 1.4.12, and make the use of atomic operations
+  more thorough for DBusConnection (fd.o #38005, Simon McVittie)
 
 • Fix a file descriptor leak when connecting to a TCP socket (fd.o #37258,
   Simon McVittie)
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index bf927ba2..1417b733 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -342,8 +342,14 @@ static dbus_bool_t        _dbus_connection_peek_for_reply_unlocked           (DB
 static DBusMessageFilter *
 _dbus_message_filter_ref (DBusMessageFilter *filter)
 {
-  _dbus_assert (filter->refcount.value > 0);
+#ifdef DBUS_DISABLE_ASSERT
   _dbus_atomic_inc (&filter->refcount);
+#else
+  dbus_int32_t old_value;
+
+  old_value = _dbus_atomic_inc (&filter->refcount);
+  _dbus_assert (old_value > 0);
+#endif
 
   return filter;
 }
@@ -351,9 +357,12 @@ _dbus_message_filter_ref (DBusMessageFilter *filter)
 static void
 _dbus_message_filter_unref (DBusMessageFilter *filter)
 {
-  _dbus_assert (filter->refcount.value > 0);
+  dbus_int32_t old_value;
+
+  old_value = _dbus_atomic_dec (&filter->refcount);
+  _dbus_assert (old_value > 0);
 
-  if (_dbus_atomic_dec (&filter->refcount) == 1)
+  if (old_value == 1)
     {
       if (filter->free_user_data_function)
         (* filter->free_user_data_function) (filter->user_data);
@@ -1310,8 +1319,9 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
   
   if (_dbus_modify_sigpipe)
     _dbus_disable_sigpipe ();
-  
-  connection->refcount.value = 1;
+
+  /* initialized to 0: use atomic op to avoid mixing atomic and non-atomic */
+  _dbus_atomic_inc (&connection->refcount);
   connection->transport = transport;
   connection->watches = watch_list;
   connection->timeouts = timeout_list;
@@ -2108,23 +2118,15 @@ _dbus_connection_send_and_unlock (DBusConnection *connection,
 void
 _dbus_connection_close_if_only_one_ref (DBusConnection *connection)
 {
-  dbus_int32_t tmp_refcount;
+  dbus_int32_t refcount;
 
   CONNECTION_LOCK (connection);
 
-  /* We increment and then decrement the refcount, because there is no
-   * _dbus_atomic_get (mirroring the fact that there's no InterlockedGet
-   * on Windows). */
-  _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);
+  refcount = _dbus_atomic_get (&connection->refcount);
+  /* The caller should have at least one ref */
+  _dbus_assert (refcount >= 1);
 
-  if (tmp_refcount == 2)
+  if (refcount == 1)
     _dbus_connection_close_possibly_shared_and_unlock (connection);
   else
     CONNECTION_UNLOCK (connection);
@@ -2655,9 +2657,9 @@ _dbus_connection_last_unref (DBusConnection *connection)
   DBusList *link;
 
   _dbus_verbose ("Finalizing connection %p\n", connection);
-  
-  _dbus_assert (connection->refcount.value == 0);
-  
+
+  _dbus_assert (_dbus_atomic_get (&connection->refcount) == 0);
+
   /* You have to disconnect the connection before unref:ing it. Otherwise
    * you won't get the disconnected message.
    */
@@ -5420,8 +5422,8 @@ dbus_connection_add_filter (DBusConnection            *connection,
   if (filter == NULL)
     return FALSE;
 
-  filter->refcount.value = 1;
-  
+  _dbus_atomic_inc (&filter->refcount);
+
   CONNECTION_LOCK (connection);
 
   if (!_dbus_list_append (&connection->filter_list,
diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
index 3bb98c51..d9091714 100644
--- a/dbus/dbus-message.c
+++ b/dbus/dbus-message.c
@@ -1527,14 +1527,20 @@ dbus_message_copy (const DBusMessage *message)
 DBusMessage *
 dbus_message_ref (DBusMessage *message)
 {
+#ifndef DBUS_DISABLE_ASSERT
   dbus_int32_t old_refcount;
+#endif
 
   _dbus_return_val_if_fail (message != NULL, NULL);
   _dbus_return_val_if_fail (message->generation == _dbus_current_generation, NULL);
   _dbus_return_val_if_fail (!message->in_cache, NULL);
-  
+
+#ifdef DBUS_DISABLE_ASSERT
+  _dbus_atomic_inc (&message->refcount);
+#else
   old_refcount = _dbus_atomic_inc (&message->refcount);
   _dbus_assert (old_refcount >= 1);
+#endif
 
   return message;
 }
@@ -1557,7 +1563,7 @@ dbus_message_unref (DBusMessage *message)
 
   old_refcount = _dbus_atomic_dec (&message->refcount);
 
-  _dbus_assert (old_refcount >= 0);
+  _dbus_assert (old_refcount >= 1);
 
   if (old_refcount == 1)
     {
diff --git a/dbus/dbus-object-tree.c b/dbus/dbus-object-tree.c
index 989e5d8b..172c9d95 100644
--- a/dbus/dbus-object-tree.c
+++ b/dbus/dbus-object-tree.c
@@ -954,7 +954,7 @@ allocate_subtree_object (const char *name)
 
   len = strlen (name);
 
-  subtree = dbus_malloc (MAX (front_padding + (len + 1), sizeof (DBusObjectSubtree)));
+  subtree = dbus_malloc0 (MAX (front_padding + (len + 1), sizeof (DBusObjectSubtree)));
 
   if (subtree == NULL)
     return NULL;
@@ -991,7 +991,7 @@ _dbus_object_subtree_new (const char                  *name,
     }
 
   subtree->user_data = user_data;
-  subtree->refcount.value = 1;
+  _dbus_atomic_inc (&subtree->refcount);
   subtree->subtrees = NULL;
   subtree->n_subtrees = 0;
   subtree->max_subtrees = 0;
@@ -1006,8 +1006,14 @@ _dbus_object_subtree_new (const char                  *name,
 static DBusObjectSubtree *
 _dbus_object_subtree_ref (DBusObjectSubtree *subtree)
 {
-  _dbus_assert (subtree->refcount.value > 0);
+#ifdef DBUS_DISABLE_ASSERT
   _dbus_atomic_inc (&subtree->refcount);
+#else
+  dbus_int32_t old_value;
+
+  old_value = _dbus_atomic_inc (&subtree->refcount);
+  _dbus_assert (old_value > 0);
+#endif
 
   return subtree;
 }
@@ -1015,9 +1021,12 @@ _dbus_object_subtree_ref (DBusObjectSubtree *subtree)
 static void
 _dbus_object_subtree_unref (DBusObjectSubtree *subtree)
 {
-  _dbus_assert (subtree->refcount.value > 0);
+  dbus_int32_t old_value;
+
+  old_value = _dbus_atomic_dec (&subtree->refcount);
+  _dbus_assert (old_value > 0);
 
-  if (_dbus_atomic_dec (&subtree->refcount) == 1)
+  if (old_value == 1)
     {
       _dbus_assert (subtree->unregister_function == NULL);
       _dbus_assert (subtree->message_function == NULL);
diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c
index bab516de..18f69dc3 100644
--- a/dbus/dbus-sysdeps.c
+++ b/dbus/dbus-sysdeps.c
@@ -1067,6 +1067,30 @@ _dbus_strerror_from_errno (void)
   return _dbus_strerror (errno);
 }
 
+/**
+ * Atomically get the value of an integer. It may change at any time
+ * thereafter, so this is mostly only useful for assertions.
+ *
+ * This function temporarily increases the atomic integer, so only
+ * use it in contexts where that would be OK (such as refcounts).
+ *
+ * @param atomic pointer to the integer to increment
+ * @returns the value at this moment
+ */
+dbus_int32_t
+_dbus_atomic_get (DBusAtomic *atomic)
+{
+  dbus_int32_t old_value;
+
+  /* On Windows we use InterlockedIncrement and InterlockedDecrement,
+   * and there is no InterlockedGet, so we have to change the value.
+   * Increasing it is less likely to have bad side-effects (for instance,
+   * it's OK for refcounts). */
+  old_value = _dbus_atomic_inc (atomic);
+  _dbus_atomic_dec (atomic);
+  return old_value;
+}
+
 /** @} end of sysdeps */
 
 /* tests in dbus-sysdeps-util.c */
diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h
index 501bc7be..f66efaad 100644
--- a/dbus/dbus-sysdeps.h
+++ b/dbus/dbus-sysdeps.h
@@ -242,6 +242,7 @@ struct DBusAtomic
 
 dbus_int32_t _dbus_atomic_inc (DBusAtomic *atomic);
 dbus_int32_t _dbus_atomic_dec (DBusAtomic *atomic);
+dbus_int32_t _dbus_atomic_get (DBusAtomic *atomic);
 
 
 /* AIX uses different values for poll */