After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 737677 - gmain: Make GSourceCallback thread-safe
gmain: Make GSourceCallback thread-safe
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: mainloop
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 778097 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-09-30 17:57 UTC by Philip Withnall
Modified: 2017-11-28 14:11 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
gmain: Make GSourceCallback thread-safe (2.16 KB, patch)
2014-09-30 17:57 UTC, Philip Withnall 		committed Details | Review
gmain: Mark some ref_count variables as volatile (927 bytes, patch)
2014-09-30 17:57 UTC, Philip Withnall 		committed Details | Review

Description Philip Withnall 2014-09-30 17:57:47 UTC 
Patches attached. This is quite an easy race condition to trigger. I think changing the refcounting of GSourceCallback is the correct solution — certainly more correct than changing the locking behaviour of GMainContext. And making the refcounting atomic is a low risk change.
Comment 1 Philip Withnall 2014-09-30 17:57:50 UTC 
Created attachment 287475 [details] [review]
gmain: Make GSourceCallback thread-safe

Otherwise there is a race in finalising the GSourceCallback if one
thread is finishing off a g_main_dispatch() while another thread is
destroying the GSource which owns the GSourceCallback.

A helgrind log:

==21707== Possible data race during write of size 4 at 0x54EACB0 by
thread #12
==21707== Locks held: none
==21707==    at 0x4ECC174: g_source_callback_unref (gmain.c:1528)
==21707==    by 0x4ECD953: g_main_dispatch (gmain.c:3081)
==21707==    by 0x4ECE667: g_main_context_dispatch (gmain.c:3673)
==21707==    by 0x4ECE859: g_main_context_iterate (gmain.c:3744)
==21707==    by 0x4ECEC7F: g_main_loop_run (gmain.c:3938)
==21707==    by 0x41C197: some_thread (some-code.c:224)
==21707==
==21707== This conflicts with a previous write of size 4 by thread #5
==21707== Locks held: 1, at address 0x54CF320
==21707==    at 0x4ECC174: g_source_callback_unref (gmain.c:1528)
==21707==    by 0x4ECB86F: g_source_destroy_internal (gmain.c:1178)
==21707==    by 0x4ECB9D4: g_source_destroy (gmain.c:1227)
==21707==    by 0x41CF09: some_other_thread (some-other-code.c:410)
Comment 2 Philip Withnall 2014-09-30 17:57:53 UTC 
Created attachment 287476 [details] [review]
gmain: Mark some ref_count variables as volatile

To make it more obvious they should exclusively be accessed with atomic
functions.
Comment 3 Philip Withnall 2017-02-03 00:19:24 UTC 
*** Bug 778097 has been marked as a duplicate of this bug. ***
Comment 4 Emmanuele Bassi (:ebassi) 2017-11-28 12:50:57 UTC 
Review of attachment 287475 [details] [review]:

Looks good
Comment 5 Emmanuele Bassi (:ebassi) 2017-11-28 12:52:07 UTC 
Review of attachment 287476 [details] [review]:

Yep.

It'd be good, at some point, to revisit the grefcount type patch, as it would make it even more explicit.
Comment 6 Philip Withnall 2017-11-28 14:10:54 UTC 
Attachment 287475 [details] pushed as d73f8ee - gmain: Make GSourceCallback thread-safe
Attachment 287476 [details] pushed as 9297a59 - gmain: Mark some ref_count variables as volatile