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 692034 - Install an invalidation notifier for GClosure in g_source_set_closure()
Install an invalidation notifier for GClosure in g_source_set_closure()
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: mainloop
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-01-18 20:24 UTC by Giovanni Campagna
Modified: 2017-11-28 15:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Install an invalidation notifier for GClosure in g_source_set_closure() (1.53 KB, patch)
2013-01-18 20:24 UTC, Giovanni Campagna
committed Details | Review
gmain: Unref GSourceCallbackFuncs _before_ finalising GSource (1.86 KB, patch)
2014-09-25 09:22 UTC, Philip Withnall
committed Details | Review

Description Giovanni Campagna 2013-01-18 20:24:21 UTC
The point of g_source_set_closure() is getting memory management right,
including handling closures disappearing from the outside (for example
because a runtime they refer to is being shutdown). This means that
sources with an associated closure should remove themselves from the
main loop and free memory when the closure is invalidated.
Comment 1 Giovanni Campagna 2013-01-18 20:24:24 UTC
Created attachment 233796 [details] [review]
Install an invalidation notifier for GClosure in g_source_set_closure()
Comment 2 Colin Walters 2013-01-20 15:18:54 UTC
Review of attachment 233796 [details] [review]:

Ah, makes sense to me.
Comment 3 Giovanni Campagna 2013-01-20 15:24:14 UTC
Attachment 233796 [details] pushed as 1ce415b - Install an invalidation notifier for GClosure in g_source_set_closure()
Comment 4 Allison Karlitskaya (desrt) 2013-02-20 10:42:01 UTC
See bug 694253 for a possible regression.
Comment 5 Philip Withnall 2014-09-25 09:04:34 UTC
This introduces a bug with GSources with closures (e.g. any GSource which has had g_source_set_dummy_callback() called on it, which happens quite a bit for GCancellableSources), whereby dropping the last reference on the GSource will invalidate the closure, calling g_source_destroy() on the already-freed GSource and making valgrind unhappy (and also potentially causing a crash, though it’s unlikely for the GSource’s memory to have been re-used in the space between calling g_free() and g_source_destroy() in g_source_unref_internal()).

I don’t really know the best way to fix this:
 • The g_source_destroy() is needed in the closure invalidate notifier in case the closure is invalidated before the GSource is finalised.
 • The closure cannot hold a reference to the GSource or we’d get a reference loop.
 • The GSource cannot call the GSourceCallbackFuncs.unref function (closure unref) before freeing itself when finalising, or it would end up being called with the GMainContext lock held.

Perhaps the third option is workable by temporarily releasing the GMainContext lock while calling the unref callback, and doing so earlier in g_source_unref_internal()?

Here’s a valgrind trace, using GLib master (42d0dc363e3597d36136051d53e25c09ebff253f) with the patch from bug #737338 applied, and libnice master (07a46eded142f35952a56198c4d5f2a7e8edd3a8):

==4961== Invalid read of size 8
==4961==    at 0x54A4693: g_source_destroy (gmain.c:1224)
==4961==    by 0x50286FD: closure_invalidated (gsourceclosure.c:228)
==4961==    by 0x50094DD: closure_invoke_notifiers (gclosure.c:256)
==4961==    by 0x500A775: g_closure_invalidate (gclosure.c:562)
==4961==    by 0x500A804: g_closure_unref (gclosure.c:584)
==4961==    by 0x54A58E1: g_source_unref_internal (gmain.c:2053)
==4961==    by 0x54A5861: g_source_unref_internal (gmain.c:2036)
==4961==    by 0x54A5945: g_source_unref (gmain.c:2073)
==4961==    by 0x407F55: test_pollable_properties (test-build-io-stream.c:174)
==4961==    by 0x409E1E: main (test-build-io-stream.c:450)
==4961==  Address 0x5a3aa30 is 32 bytes inside a block of size 112 free'd
==4961==    at 0x4A074C4: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4961==    by 0x54AEC47: g_free (gmem.c:190)
==4961==    by 0x54A589E: g_source_unref_internal (gmain.c:2042)
==4961==    by 0x54A5861: g_source_unref_internal (gmain.c:2036)
==4961==    by 0x54A5945: g_source_unref (gmain.c:2073)
==4961==    by 0x407F55: test_pollable_properties (test-build-io-stream.c:174)
==4961==    by 0x409E1E: main (test-build-io-stream.c:450)
==4961== 
==4961== Invalid read of size 4
==4961==    at 0x54A46C0: g_source_destroy (gmain.c:1229)
==4961==    by 0x50286FD: closure_invalidated (gsourceclosure.c:228)
==4961==    by 0x50094DD: closure_invoke_notifiers (gclosure.c:256)
==4961==    by 0x500A775: g_closure_invalidate (gclosure.c:562)
==4961==    by 0x500A804: g_closure_unref (gclosure.c:584)
==4961==    by 0x54A58E1: g_source_unref_internal (gmain.c:2053)
==4961==    by 0x54A5861: g_source_unref_internal (gmain.c:2036)
==4961==    by 0x54A5945: g_source_unref (gmain.c:2073)
==4961==    by 0x407F55: test_pollable_properties (test-build-io-stream.c:174)
==4961==    by 0x409E1E: main (test-build-io-stream.c:450)
==4961==  Address 0x5a3aa3c is 44 bytes inside a block of size 112 free'd
==4961==    at 0x4A074C4: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4961==    by 0x54AEC47: g_free (gmem.c:190)
==4961==    by 0x54A589E: g_source_unref_internal (gmain.c:2042)
==4961==    by 0x54A5861: g_source_unref_internal (gmain.c:2036)
==4961==    by 0x54A5945: g_source_unref (gmain.c:2073)
==4961==    by 0x407F55: test_pollable_properties (test-build-io-stream.c:174)
==4961==    by 0x409E1E: main (test-build-io-stream.c:450)
==4961== 
==4961== Invalid write of size 4
==4961==    at 0x54A46CC: g_source_destroy (gmain.c:1229)
==4961==    by 0x50286FD: closure_invalidated (gsourceclosure.c:228)
==4961==    by 0x50094DD: closure_invoke_notifiers (gclosure.c:256)
==4961==    by 0x500A775: g_closure_invalidate (gclosure.c:562)
==4961==    by 0x500A804: g_closure_unref (gclosure.c:584)
==4961==    by 0x54A58E1: g_source_unref_internal (gmain.c:2053)
==4961==    by 0x54A5861: g_source_unref_internal (gmain.c:2036)
==4961==    by 0x54A5945: g_source_unref (gmain.c:2073)
==4961==    by 0x407F55: test_pollable_properties (test-build-io-stream.c:174)
==4961==    by 0x409E1E: main (test-build-io-stream.c:450)
==4961==  Address 0x5a3aa3c is 44 bytes inside a block of size 112 free'd
==4961==    at 0x4A074C4: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4961==    by 0x54AEC47: g_free (gmem.c:190)
==4961==    by 0x54A589E: g_source_unref_internal (gmain.c:2042)
==4961==    by 0x54A5861: g_source_unref_internal (gmain.c:2036)
==4961==    by 0x54A5945: g_source_unref (gmain.c:2073)
==4961==    by 0x407F55: test_pollable_properties (test-build-io-stream.c:174)
==4961==    by 0x409E1E: main (test-build-io-stream.c:450)
Comment 6 Philip Withnall 2014-09-25 09:22:56 UTC
Created attachment 287056 [details] [review]
gmain: Unref GSourceCallbackFuncs _before_ finalising GSource

Rather than unreffing them _after_ finalising the GSource and freeing
its struct. This fixes the case where the GSourceCallbackFuncs data
contains a pointer to the GSource, and the unref() function operates on
that pointer, e.g. by calling g_source_destroy(). This happens when
using g_source_set_dummy_callback() on a GSource, as the generated
GClosure needs to destroy the GSource when it is invalidated, which
could happen (at latest) when the GSourceCallbackFuncs.unref() function
is called during finalisation of the GSource.

By moving the GSourceCallbackFuncs.unref() invocation higher up in
g_source_unref_internal(), it becomes re-entrancy-safe for GSource
methods.
Comment 7 Philip Withnall 2014-09-25 09:23:53 UTC
(In reply to comment #6)
> Created an attachment (id=287056) [details] [review]
> gmain: Unref GSourceCallbackFuncs _before_ finalising GSource

So here’s an attempt at a patch to fix it. It passes all the GLib unit tests, and fixes the problem in libnice without any obvious negative effects…but I’m wary of touching the GSource code and have probably broken something. Expert review needed!
Comment 8 Emmanuele Bassi (:ebassi) 2017-11-28 12:54:37 UTC
Review of attachment 287056 [details] [review]:

By and large, this sounds logical to me: if the source owns the callbacks, then the source must exist while we destroy the callbacks.

I'm not an expert of GSource, though.
Comment 9 Philip Withnall 2017-11-28 15:02:59 UTC
Thanks for the review. I had to fix some merge conflicts, and added a refcount inc/dec around the callback_funcs.unref() call to mirror the finalize() call above.

Attachment 287056 [details] pushed as 90dd9ff - gmain: Unref GSourceCallbackFuncs _before_ finalising GSource