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.
(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.)
(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.
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.
(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 ***