thread-safety of counters

Adrian Szyndela <[email protected]> Mon, 23 Feb 2015 12:23:39 +0100

Newsgroups	gmane.comp.freedesktop.dbus
Message-ID	<[email protected]>

Hi,
Let's consider a simple test program that:
1. creates a D-Bus connection;
2. creates a few threads that share the connection;
3. then, threads perform method calls over the connection.
A sample program is attached.

My question is: is dbus designed for such usage?

If answer is no, then I have another question: is there any place that
describes what is thread-safe in dbus and what is not?
If answer is yes, then I think there is a bug. I am able to trick the
sample program into a crash only by executing instructions in specific
order on ARM arch.

Precisely:
I used gdb in non-stop mode to handle threads independently.
I caught two threads in two points:
Thread 1. in _dbus_counter_ref() while adding a counter to an incoming
message (dbus/dbus-transport.c:1161, _dbus_transport_queue_messages());
Thread 2. in _dbus_counter_unref() on the same counter, while unrefing a
reply for another method call.

There are two messages involved, but both point to the same counter
(connection->transport->live_messages).

Then, by careful stepping through assembler of ref/unref functions in
both threads I was able to generate refcount corruption. This resulted
in freeing the object prematurely, and eventually led to a crash.

As a conclusion, I suggest changing DBusCounter's refcount from int to
DBusAtomic, as in DBusMessage. A patch with this done is attached.

This might be related to bug reports:
https://bugs.launchpad.net/ubuntu/+source/colord/+bug/958267
https://bugzilla.redhat.com/show_bug.cgi?id=1177488
https://bugzilla.redhat.com/show_bug.cgi?id=1157141

as we have the same call stack trace when crashed. However, these are
generated on x86_64, and I am not sure if the described scenario is
possible on this arch.

Regards,
Adrian

_______________________________________________
dbus mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dbus

tt.c (text/x-csrc, 983 B)

#include <pthread.h>
#include <dbus/dbus.h>

  DBusConnection *connection;

void *fun(void*a)
{
	DBusError error;
	dbus_error_init (&error);
	while (1)
	{
		DBusMessageIter iter;
		DBusMessage* message = dbus_message_new_method_call (NULL,
				"/org/freedesktop/DBus",
				"org.freedesktop.DBus",
				"ListNames");
		dbus_message_set_auto_start (message, TRUE);
		dbus_message_set_destination (message, "org.freedesktop.DBus");
		dbus_message_iter_init_append (message, &iter);

		DBusMessage *reply = dbus_connection_send_with_reply_and_block (connection,
				message, -1,
				&error);

		dbus_message_unref(reply);
		dbus_message_unref(message);
	}
	return 0;
}

int main(int argc, char **argv)
{
  DBusError error;
  dbus_error_init (&error);
  connection = dbus_bus_get (DBUS_BUS_SYSTEM, &error);

  int i;
  for (i = 0; i < 10; i++)
  {
	  pthread_t thr;
	  pthread_create(&thr, NULL, fun, NULL);
  }

  for (;;) sleep(1000);

  dbus_connection_unref(connection);
  return 0;
}

counter-thread-safety.patch (text/x-patch, 1.3 KB)

diff --git a/dbus/dbus-resources.c b/dbus/dbus-resources.c
index 80fb55b..991bb58 100644
--- a/dbus/dbus-resources.c
+++ b/dbus/dbus-resources.c
@@ -53,7 +53,7 @@
  */
 struct DBusCounter
 {
-  int refcount;  /**< reference count */
+  DBusAtomic refcount;  /**< reference count */
 
   long size_value;       /**< current size counter value */
   long unix_fd_value;    /**< current unix fd counter value */
@@ -93,7 +93,7 @@ _dbus_counter_new (void)
   if (counter == NULL)
     return NULL;
 
-  counter->refcount = 1;
+  _dbus_atomic_inc (&counter->refcount);
 
   return counter;
 }
@@ -107,9 +107,9 @@ _dbus_counter_new (void)
 DBusCounter *
 _dbus_counter_ref (DBusCounter *counter)
 {
-  _dbus_assert (counter->refcount > 0);
+  _dbus_assert (_dbus_atomic_get (&counter->refcount) > 0);
   
-  counter->refcount += 1;
+  _dbus_atomic_inc (&counter->refcount);
 
   return counter;
 }
@@ -123,11 +123,13 @@ _dbus_counter_ref (DBusCounter *counter)
 void
 _dbus_counter_unref (DBusCounter *counter)
 {
-  _dbus_assert (counter->refcount > 0);
+ dbus_int32_t old_refcount;
 
-  counter->refcount -= 1;
+  old_refcount = _dbus_atomic_dec (&counter->refcount);
 
-  if (counter->refcount == 0)
+  _dbus_assert (old_refcount >= 1);
+
+  if (old_refcount == 1)
     {
       
       dbus_free (counter);