GNOME Bugzilla – Bug 679509
use after free in g_dbus_action_group_describe_all_done()
Last modified: 2012-09-30 16:27:25 UTC
Created attachment 218173 [details] [review] GDBusActionGroup: fix use after free in g_dbus_action_group_describe_all_done() see patch
Can you describe when this issue is hit?
Created attachment 218848 [details] [review] testcase ==31959== Invalid read of size 8 ==31959== at 0x5637DDB: g_dbus_action_group_describe_all_done (gdbusactiongroup.c:249) ==31959== by 0x5607E76: g_simple_async_result_complete (gsimpleasyncresult.c:767) ==31959== by 0x565667D: g_dbus_connection_call_done (gdbusconnection.c:5283) ==31959== by 0x5607E76: g_simple_async_result_complete (gsimpleasyncresult.c:767) ==31959== by 0x5607F78: complete_in_idle_cb (gsimpleasyncresult.c:779) ==31959== by 0x4C6E6E4: g_main_context_dispatch (gmain.c:2539) ==31959== by 0x4C6EA17: g_main_context_iterate.isra.23 (gmain.c:3146) ==31959== by 0x4C6EE11: g_main_loop_run (gmain.c:3340) ==31959== by 0x403141: test_bug679509 (actions.c:839) ==31959== by 0x4C8FF54: g_test_run_suite_internal (gtestutils.c:1663) ==31959== by 0x4C900D5: g_test_run_suite_internal (gtestutils.c:1727) ==31959== by 0x4C900D5: g_test_run_suite_internal (gtestutils.c:1727) ==31959== by 0x4C9043A: g_test_run_suite (gtestutils.c:1772) ==31959== by 0x402FAF: main (actions.c:861) ==31959== Address 0x5b58b58 is 56 bytes inside a block of size 72 free'd ==31959== at 0x4A079AE: free (vg_replace_malloc.c:427) ==31959== by 0x4C743CE: g_free (gmem.c:252) ==31959== by 0x4C88D0E: g_slice_free1 (gslice.c:1111) ==31959== by 0x51788B4: g_type_free_instance (gtype.c:1937) ==31959== by 0x403127: test_bug679509 (actions.c:836) ==31959== by 0x4C8FF54: g_test_run_suite_internal (gtestutils.c:1663) ==31959== by 0x4C900D5: g_test_run_suite_internal (gtestutils.c:1727) ==31959== by 0x4C900D5: g_test_run_suite_internal (gtestutils.c:1727) ==31959== by 0x4C9043A: g_test_run_suite (gtestutils.c:1772) ==31959== by 0x402FAF: main (actions.c:861)
Looks pretty legit, thanks. I wonder if it would be easier to fix this by just taking an extra ref on the proxy while the call was in progress, though...
Created attachment 218911 [details] [review] GDBusActionGroup: hold ref until async init done Taking extra ref is a solution, too.
Review of attachment 218911 [details] [review]: Definitely much easier this way. Looks great. Thanks for the patch.
Could you commit?
The following fixes have been pushed: 151b198 GDBusActionGroup: hold ref until async init done b2d848e gio/tests/actions: test for bug679509
Created attachment 220448 [details] [review] GDBusActionGroup: hold ref until async init done to avoid use-after-free if GDBusActionGroup was finalized
Created attachment 220449 [details] [review] gio/tests/actions: test for bug679509
*** Bug 678698 has been marked as a duplicate of this bug. ***
Unfortunately commited patch fixes only a half of the problem. Another half: zombie GDBusActionGroup can emit signal. Btw, my first patch fixes this completely.
Created attachment 225425 [details] [review] improved testcase crashes with GLib-GObject-CRITICAL **: g_object_ref: assertion `G_IS_OBJECT (object)' failed
You can not assume that an object will stop emitting signals just because you drop your ref on it. Others may also be holding refs. You must explicitly disconnect signals. In this sense, your testcase is buggy...
Review of attachment 225425 [details] [review]: This testcase engages in undefined behaviour (ie: connecting a signal with a user_data that becomes invalid before the signal is disconnected [which is never]).