GNOME Bugzilla – Bug 778049
race in gsource detected by TSan
Last modified: 2017-04-21 23:07:32 UTC
Created attachment 344749 [details] ThreadSanitizer: data race glib-2-50/glib/gmain.c:3073 in g_source_is_destroyed In this case reported by the ThreadSanitizer, the source->flags are read in g_source_is_destroyed() without holding the context lock.
Created attachment 344929 [details] [review] gmain: Fix locking around GSource.flags field After a GSource is attached to a GMainContext, it can be accessed from any thread (for example, the GMainContext may be running in a different thread to the one which created the GSource). Currently, there are various bits of the code which access (but not update GSource.flags) unlocked; they might therefore see an inconsistent value for the flags. Fix that by protecting access to the flags with the attached GMainContext’s lock, if such a context is attached. Otherwise, access remains unlocked.
Here’s a strawman patch which extends the current locking regime (lock GSource.context if it’s non-NULL; otherwise don’t lock). Does that fix the issue for you, Fabrice? The alternative would be to use the fact that the G_HOOK_FLAG_ACTIVE flag is fairly independent of the other fields in the struct – in that once it’s set, clients should not look at the other fields any more – so it could be set and accessed atomically. A lock might not be necessary for it, because accesses to it don’t need synchronising with respect to other accesses to the GSource fields. That’s getting harder to reason about though.
Allison and I have taken a look at this, and it seems like it is a GCancellable bug instead of a bug in GSource: GCancellable should not be calling g_source_is_destroyed() in its dispatch handler, because a) it’s pointless because the cancellable could be cancelled immediately after the check and b) setting the ready time on a destroyed GSource causes no harm, so the check is redundant. Patch will appear as soon as I’ve tested it.
Review of attachment 344929 [details] [review]: Not the correct fix: reading GSource.flags without a lock is harmless, as all the flags are independent, and we are never going to read intermediate state from a locked transaction in another thread (because there is never any intermediate state for GSource.flags). What we *could* do is to a) add documentation to GSource marking which fields need to be accessed with the GSource.context lock and b) update the documentation for g_source_is_destroyed() to say that calling it from a thread other than the one owned by the GMainContext the GSource is attached to is pointless, since the value could be outdated as soon as it’s returned.
Created attachment 344978 [details] [review] gio: Drop redundant g_source_is_destroyed() calls These calls cause race warnings from tsan, but are not a thread safety problem, because we can only ever observe single bit changes: all modifications to the GSource.flags field are done with a lock held; all reads are of independent fields, so no intermediate state can ever be observed. This assumes that a non-atomic read will consistently give us an old value or a new value. In any case, these g_source_is_destroyed() calls can happen from any thread, and the state could be changed from another thread immediately after the call returns; so the checks are pointless. In addition, calling g_source_set_ready_time() or g_source_destroy() on a destroyed source is not a problem.
Created attachment 344979 [details] [review] gmain: Add some threading comments for GSource members These are for internal use.
Created attachment 344980 [details] [review] gmain: Document threading properties of g_source_is_destroyed()
Review of attachment 344978 [details] [review]: Did we also want to add some docs on GSource that stuff like this is OK?
Review of attachment 344979 [details] [review]: ::: glib/gmain.c @@ +348,3 @@ +/* All members of GSource must be accessed with the GSource.context’s lock + * held, if GSource.context is set. */ But like, this isn't true...
Review of attachment 344980 [details] [review]: Love it. A++! Would review again!
Attachment 344978 [details] pushed as 4091b2d - gio: Drop redundant g_source_is_destroyed() calls Attachment 344980 [details] pushed as c408256 - gmain: Document threading properties of g_source_is_destroyed()
(In reply to Allison Lortie (desrt) from comment #8) > Review of attachment 344978 [details] [review] [review]: > > Did we also want to add some docs on GSource that stuff like this is OK? Fixed for g_source_set_ready_time() as: 32aae79 gmain: Document that set_ready_time() is safe on destroyed GSources g_source_destroy() already documented that it’s safe to call twice.
I think this fix uncovered https://bugzilla.gnome.org/show_bug.cgi?id=781601