GNOME Bugzilla – Bug 710367
Crash in g_settings_backend_dispatch_signal()
Last modified: 2018-02-02 13:53:07 UTC
This crash has been reported a few times and has ~400 occurrences in the Fedora crash database. It happens with glib2-2.36.3-3.fc19.x86_64 and dconf-0.16.0-2.fc19.x86_64 (at least). https://bugzilla.redhat.com/show_bug.cgi?id=873412 Core was generated by `evolution'. Program terminated with signal 11, Segmentation fault.
+ Trace 232634
Thread 6 (Thread 0x7f436c0d4a00 (LWP 5322))
Thread 1 (Thread 0x7f435f3ed700 (LWP 5333))
I have a strong suspicion that this is a once-too-many-unref of a GSettings object...
Do you mean this is probably a bug in Evolution? All reports I could find occurred there indeed (this is not a proof but...).
*** Bug 711375 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > I have a strong suspicion that this is a once-too-many-unref of a GSettings > object... Thanks for the suggestion. I did walk-through evolution-data-server and evolution code and checked how GSettings objects returned from g_settings_new() are unreffed, and as far as i can tell, all are unreffed only once. I even found three places where they were not freed at all. I saw few other bug reports recently, which were about memory corruption, thus maybe this is a consequence of it?
Confirming this. It has nothing to do with a GSettings object having a connected signal handler at dispose time _or_ with one too many unref. It's a plain stupid race. A signal comes in just as someone calls unref() on the last reference on a GSettings in the main thread. The dispose process starts in the main thread and gets to the point where it has started to call weak notifies. The GSettingsBackend lock is acquired in the thread which received the signal. It increases the refcount on the GSettings object, but it is too late. We release the lock. Back in the main thread, the lock is acquired and despite the fact that we increased the reference count, we're already running the weak notify handlers. We remove ourselves from the list. Back in the signal thread, we access the list, assuming that we will still be alive (since we took a ref, right?). Crash.
Created attachment 271563 [details] [review] gsettingsbackend: a minor simplification Change the order of the arguments on the (internal) keys_changed callback in GSettingsListenerVTable. This means that all functions in the table now fit the following signature: void (* f) (GObject *target, GSettingsBackend *backend, const gchar *name_or_path, gpointer origin_tag, const gchar * const *names); allowing the possibility of arguments ignored at the end. This allows us to simplify our dispatch-to-thread code in GSettingsBackend, making it a bit less generic. So far, this should be a straight refactor.
Created attachment 271564 [details] [review] GSettingsBackend: fix a nasty race condition In the event that a GSettings object is being destroyed just as a change signal is being delivered, the destroying thread will race with the dconf worker thread for acquiring the lock on the GSettingsBackend. If the signalling thread gets there first then the destroying thread will block on the lock. The signalling thread adds a reference to the GSettings object that is being destroyed and releases the lock. The idea is that this should prevent the GSettings object from being destroyed and thus maintain its entry in the list. Unfortunately, the weak reference notify function is already running and as soon as we release the lock, the list entry is removed. The signalling thread crashes. This bug is indicative of a serious problem encountered in many situations where GObject instances are touched from multiple threads. Ideally, we will move to a place where g_object_ref() is not called at all on the GSettings object from the dconf worker thread and instead, a dispatch will be done without holding a reference (similar to how GAppInfoMonitor presently works). This would also prevent the unfortunate case of someone dropping what they assume to be the last reference on a GSettings object, only to have an already-pending signal delivered once they return to the mainloop, crashing their program. Making this change for GSettings (with multiple instances per thread, the possibility of multiple backends and each instance being interested in different events) is going to be extremely non-trivial, so it's not a change that makes sense at this point in the cycle. For now, we can do a relatively small and isolated tweak so that we never access the list except under a lock. We still perform the bad pattern of acquiring a ref in a foreign thread which means that we still risk delivering a signal to a GSettings object that the user has assumed is dead (unless they explicitly disconnect their signal handler). This is a problem that we already had, however.
Attachment 271563 [details] pushed as 698970f - gsettingsbackend: a minor simplification Attachment 271564 [details] pushed as 3f119b2 - GSettingsBackend: fix a nasty race condition Pushing on the basis of positive testing in Ubuntu (where this is already being carried as a vendor patch).
*** Bug 727641 has been marked as a duplicate of this bug. ***
*** Bug 703098 has been marked as a duplicate of this bug. ***