GSettingsBackend: fix a nasty race condition
authorRyan Lortie <desrt@desrt.ca>
Wed, 12 Mar 2014 01:41:36 +0000 (21:41 -0400)
committerRyan Lortie <desrt@desrt.ca>
Fri, 14 Mar 2014 13:46:39 +0000 (09:46 -0400)
commit3f119b2fd408fb2e0f4eea07ed9618d2faa749ee
treeeabae27e42ac8368234cc14d9204e33e380613fe
parent698970f1f70f1821df5ac83ffa6d797fa4579881
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.

https://bugzilla.gnome.org/show_bug.cgi?id=710367
gio/gsettingsbackend.c