Bug 40072 - dbus_threads_init() should be made reentrant
Summary: dbus_threads_init() should be made reentrant
Status: RESOLVED DUPLICATE of bug 54972
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium trivial
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords: NEEDINFO, patch
Depends on:
Blocks:
 
Reported: 2011-08-14 03:48 UTC by Mirsal Ennaime
Modified: 2013-09-13 13:39 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Make dbus_threads_init() reentrant (3.67 KB, patch)
2011-08-14 03:48 UTC, Mirsal Ennaime
Details | Splinter Review

Description Mirsal Ennaime 2011-08-14 03:48:52 UTC
Created attachment 50194 [details] [review]
Make dbus_threads_init() reentrant

dbus_threads_init() manipulates static data in a way which is not reentrant.
It should be made reentrant by manipulating a copy on the stack instead.
Comment 1 Simon McVittie 2011-08-18 08:20:42 UTC
(In reply to comment #0)
> dbus_threads_init() manipulates static data in a way which is not reentrant.
> It should be made reentrant by manipulating a copy on the stack instead.

When would this be a problem? There are two uses for re-entrancy (that I'm aware of): functions that can (directly or indirectly) call themselves, and functions that can be called in multiple threads at the same time.

The only calls out to user-supplied code that dbus_threads_init makes are to the implementation of _dbus_mutex_new; if the implementation of "give me a new mutex" tries to re-initialize threads, then something is very wrong.

dbus_threads_init() is documented as "Note that this function must be called BEFORE the second thread is started", so if you have more than one thread at the first call to this function, your application is wrong.

The documentation also says "dbus_threads_init() may be called more than once. The first one wins and subsequent calls are ignored". This is the reason for the early-return if thread_functions.mask != 0, which it looks as though your patch breaks.

> +  thread_functions = local_functions;

If you're aiming for this to be an atomic replacement, you should be aware that assignment of larger-than-pointer-sized aggregates compiles into a series of several memory accesses (or sometimes even a call to memcpy).

dbus_threads_init() can't use mutexes, because until dbus_threads_init() finishes, there are no mutexes. It can't use atomic operations either, because our implementation of atomic operations falls back to using locks on platforms where there are no atomic operations.

(In a hypothetical D-Bus 2.0 I'd remove the ability to use user-specified threading functions, though - in practice dbus_threads_init_default() should do the right thing on all platforms, and if it doesn't, we should fix it so it does.)
Comment 2 Simon McVittie 2011-08-18 08:21:58 UTC
(In reply to comment #1)
> This is the reason for
> the early-return if thread_functions.mask != 0, which it looks as though your
> patch breaks

Sorry, that's not actually true, but the rest stands.
Comment 3 Simon McVittie 2012-02-17 05:33:47 UTC
I've spent about a day and a half trying to make thread initialization safe to call from more than one thread at the same time (so that it can be done automatically), and it doesn't currently look feasible to reconcile this with the requirement that libdbus copes gracefully with out-of-memory errors. You're welcome to prove me wrong, of course.
Comment 4 Simon McVittie 2013-09-13 13:39:34 UTC
(In reply to comment #3)
> I've spent about a day and a half trying to make thread initialization safe
> to call from more than one thread at the same time (so that it can be done
> automatically), and it doesn't currently look feasible to reconcile this
> with the requirement that libdbus copes gracefully with out-of-memory
> errors. You're welcome to prove me wrong, of course.

Some months later, I succeeded: this was fixed as part of Bug #54972. Fixed in 1.7.4.

*** This bug has been marked as a duplicate of bug 54972 ***


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.