Bug 89297

Created attachment 114648 [details] [review]
A patch that uses mutex to protect DBusCounter data

It seems using a mutex is better than using atomic, because it protects whole object instead of protecting only reference count.

Created attachment 114668 [details]
test case

(In reply to Adrian Szyndela from comment #0)
> Created attachment 113787 [details]
> A patch for making DBusCounter refcount atomic.
> 
> With a simple test program I can generate a crash in libdbus.
> The program:
> 1. creates a connection;
> 2. creates a few threads that share the connection;
> 3. then, threads perform method calls over the connection.
I tried to verify that the patch is working with the appended test case (written in python) on linux, because I do not have an ARM based system.
The test case freezes with or without the patch. Not sure, if it matches this bug.

Created attachment 114726 [details]
test program in C

I attach a test program in C that I used.

(In reply to Adrian Szyndela from comment #3)
> Created attachment 114726 [details]
> test program in C
> 
> I attach a test program in C that I used.

I did run the test app on linux without any reproducable problem on linux. How did you see the bug condition ? Do you run the app, wait for a visible error condition (please describe how do you detect the error case) and then attached a debugger or something else ?

First, it appeared during massive automatic tests (lots of devices for many hours). It was not really reproducible in controlled environment in reasonable time. All I had was a core dump. Well, lots of core dumps.

To generate the bug with the attached program I ran it under gdb in non-stop mode (https://sourceware.org/gdb/onlinedocs/gdb/Non_002dStop-Mode.html). I put breakpoints in _dbus_counter_unref() and _dbus_counter_ref(). Then, by continuing selected threads manually I was able to get the program to the state, where one thread was refing a counter, and another thread was unrefing it, as in bug description (there are copied&pasted values from actual gdb session).

So, maybe there is a way to modify the test program to "see" the bug without gdb in reasonable time, but I was already pretty sure of its existence at this point and didn't feel the need to push it further.

@simon: any hints how to proceed ?

Comment on attachment 114648 [details] [review]
A patch that uses mutex to protect DBusCounter data

Review of attachment 114648 [details] [review]:
-----------------------------------------------------------------

The overall problem here is that DBusCounter is sort of vaguely linked to a DBusConnection, but is not actually guaranteed to be protected by that connection's mutex; and a DBusMessage can carry a reference to the DBusCounter, resulting in freeing that DBusMessage having an effect on the DBusCounter.

So the weird thing that you're doing, which other libdbus users have apparently not done, seems to be:

* process a DBusConnection in one or more threads
* get a DBusMessage from that DBusConnection
* keep a reference to the message
* free the DBusMessage later, when another thread happens to be processing the DBusConnection

This is a facet of the general problem that libdbus is not very well-designed for multi-threaded use: it tries to support every possible use-case (the most-general, lowest-performance model) and as a result it supports them all poorly. A more opinionated design (for instance "each DBusConnection belongs to one thread, all other uses are invalid") would not have had this problem.

Potential fixes:

* this
* make the refcount atomic instead
* re-document libdbus as "was never actually thread safe"

The last one is unfortunately not really feasible, as much as I would like it to be.

Making the refcount atomic would involve less messing about with mutexes, so it is superficially attractive. However, the potential problem with that approach is that it does not protect the notify function. Is there a valid situation in which one thread tries to notify while another thread is changing the notify function?

The call to _dbus_counter_set_notify() in _dbus_transport_init_base() is on a newly allocated counter and nobody can be holding a ref to it yet, so that's safe.

The call in _dbus_transport_finalize_base() is, unfortunately, probably not protected by any specific lock.

The call in _dbus_transport_set_max_message_size() is under the DBusConnection's lock, but that's no good, because _dbus_counter_notify() isn't.

So unfortunately making the reference count atomic would not work either, leaving this as the only valid solution without more extensive design changes.

::: dbus/dbus-resources.c
@@ +99,5 @@
> +  _dbus_rmutex_new_at_location (&counter->mutex);
> +  if (counter->mutex == NULL)
> +  {
> +    dbus_free (counter);
> +	counter = NULL;

(Minor: something isn't right with the indentation here. Please indent new code with spaces, not "hard tabs"; if there are hard tabs in the source already, they should always be treated as advancing the cursor to the next multiple of 8 spaces, like ts=8 in vim.)

@@ +142,4 @@
>    _dbus_assert (counter->refcount > 0);
>  
>    counter->refcount -= 1;
> +  last_ref = counter->refcount == 0;

minor: "last_ref = (counter->refcount == 0);" please, to clarify precedence

@@ +213,5 @@
>    if (counter->notify_pending)
>      {
>        counter->notify_pending = FALSE;
> +	  notify_function = counter->notify_function;
> +	  notify_data = counter->notify_data;

(Minor: something isn't right with the indentation here, as above.)

Created attachment 115058 [details] [review]
A patch that uses mutex to protect DBusCounter data with corrected indentation and clarified precedence

I'll give Thiago a chance to respond since he seemed to have strong opinions on this, but if there's no veto I'll test and probably apply that patch. Thanks!

(In reply to Simon McVittie from comment #7)

> So the weird thing that you're doing, which other libdbus users have
> apparently not done, seems to be:
> 
> * process a DBusConnection in one or more threads
> * get a DBusMessage from that DBusConnection
> * keep a reference to the message
> * free the DBusMessage later, when another thread happens to be processing
> the DBusConnection

would be test-relay the best base for creating a related test case or something else ?

(In reply to Ralf Habacker from comment #10)
> would be test-relay the best base for creating a related test case or
> something else ?

The structure of test-loopback, where the program just "talks to itself", would probably be sufficient: test-relay is a bit more complicated (4 connections instead of 2) which I don't think is actually necessary here.

I think this would be best as a separate test executable: it will probably need to be fairly abusive (lots of threads, lots of iterations, arbitrary delays) in order to reproduce the bug.

Fixed in git for 1.8.18 and 1.9.16. If someone wants to add a test-case that would be great, but I'm not going to wait for it.

(In reply to Simon McVittie from comment #12)
> Fixed in git for 1.8.18 and 1.9.16. If someone wants to add a test-case that
> would be great, but I'm not going to wait for it.

I tried several variants of the appended test case running countless hours, but was not able to reproduce this problem on a linux x64_86 system. The only catch I got has been reported with bug 90316, which may or may not be related.