Created attachment 115540 [details] [review] A patch that extends lock's range in live_messages_notify(). live_messages_notify() is called with the following stack trace: #0 _dbus_transport_ref (transport=transport@entry=0x114d0) #1 live_messages_notify (counter=<optimized out>, user_data=0x114d0) #2 free_counter (element=0x117d8, data=0xb5b02eb0) #3 dbus_message_cache_or_finalize (message=0xb5b02eb0) #4 dbus_message_unref (message=0xb5b02eb0) #5 external_function() None of the above functions locks the connection with _dbus_connection_lock(), while live_messages_notify() refs and unrefs the transport. Another thread may send something at the same time using same transport: #0 _dbus_transport_unref (transport=0x114d0) #1 _dbus_connection_do_iteration_unlocked (connection=0x11b28, ...) #2 _dbus_connection_send_preallocated_unlocked_no_update (connection=0x11b28, ...) #3 0xb62e5992 in _dbus_connection_send_unlocked_no_update (connection=0x11b28, message=0xb5b030e0, ...) #4 0xb62e6092 in dbus_connection_send_with_reply (connection=0x11b28, message=0xb5b030e0, ...) #5 0xb62e6244 in dbus_connection_send_with_reply_and_block (connection=0x11b28, message=0xb5b030e0, ...) #6 another_external_function() This results in the following state: Id Target Id Frame * 2 Thread 0xb54ff460 "tt" _dbus_transport_unref (transport=0x114d0) at dbus-transport.c:504 1 Thread 0xb56ff460 "tt" _dbus_transport_ref (transport=0x114d0) at dbus-transport.c:486 Then, with non-atomic refcount, it sometimes crashes. A simple solution for this is to protect _dbus_transport_ref() and _dbus_transport_unref() with connection lock. Such lock is already used in live_messages_notify(), but only to protect call to (*transport->vtable->live_messages_changed). Nothing more really exists in live_messages_notify(), so I think lock range can be extended to protect also ref and unref calls. This is what the attached patch does. Test scenario is almost the same as in bug 89297. However, call to live_messages_notify() has to be triggered somehow (e.g. counter->size_value has to reach counter->notify_size_guard_value).
Comment on attachment 115540 [details] [review] A patch that extends lock's range in live_messages_notify(). Review of attachment 115540 [details] [review]: ----------------------------------------------------------------- This makes sense. Not your fault, but the circular dependency between the transport and the connection seems rather crazy: I don't know why we have these as separate layers if we aren't going to layer them properly. But the people who could answer that are no longer involved with D-Bus :-(
(In reply to Simon McVittie from comment #1) > Review of attachment 115540 [details] [review] [review]: > ----------------------------------------------------------------- > > This makes sense. Is this patch intended for master only or also for dbus-1.8 ?
Fixed in git for 1.8.18 and 1.9.16.
(In reply to Ralf Habacker from comment #2) > Is this patch intended for master only or also for dbus-1.8 ? The surrounding code is the same either way, and it fixes a real bug (lack of thread-safety) in production code, so it seems worthwhile for 1.8 too.
*** Bug 90316 has been marked as a duplicate of this bug. ***
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.