GNOME Bugzilla – Bug 692034
Install an invalidation notifier for GClosure in g_source_set_closure()
Last modified: 2017-11-28 15:03:04 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.
Created attachment 233796 [details] [review] Install an invalidation notifier for GClosure in g_source_set_closure()
Review of attachment 233796 [details] [review]: Ah, makes sense to me.
Attachment 233796 [details] pushed as 1ce415b - Install an invalidation notifier for GClosure in g_source_set_closure()
See bug 694253 for a possible regression.
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)
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.
(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!
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.
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