GNOME Bugzilla – Bug 781715
Crash under atk_gobject_accessible_dispose()
Last modified: 2017-05-25 06:08:58 UTC
Created attachment 350394 [details] [review] proposed patch The atk_gobject_accessible_dispose() can access already freed memory, effectively causing a crash. One such backtrace is here: Program received signal SIGSEGV, Segmentation fault. 0x00007f3e4792bf74 in g_type_check_instance_is_a (type_instance=type_instance@entry=0x54b76c0, iface_type=<optimized out>) at gtype.c:4012 4012 check = node && node->is_instantiatable && iface && type_node_conforms_to_U (node, iface, TRUE, FALSE); Missing separate debuginfos, use: debuginfo-install evolution-3.22.6-8.el7.x86_64
+ Trace 237388
Review of attachment 350394 [details] [review]: The patch looks good to me, although having an atk_gobject_accessible_dispose and an atk_gobject_accessible_real_dispose is somewhat confusing. In fact, the problem is that atk_gobject_accessible_dispose is somewhat confusing, as seems to suggest that is the Gobject dispose of the object. So a good bonus would be rename the existing dispose. If you don't find a better name, feel free to commit the patch as it is. Thanks.
Review of attachment 350394 [details] [review]: ::: atk-2.22.0/atk/atkgobjectaccessible.c.test @@ +177,3 @@ + g_object_weak_unref (obj, + (GWeakNotify) atk_gobject_accessible_dispose, + object); IIUC this way we'll never atk_object_notify_state_change() and we still might have a g_object_unref() being called on free'd memory. Note that above in atk_gobject_accessible_for_object() a different weak ref is added to the main gobject and its notify handler is g_object_unref() with the accessible instance as the data argument. If the atk_gobject_accessible_dispose (really bad name) handler runs first we do the state change notification and unref the accessible which means that the other weak ref handler (g_object_unref) will be called on a free'd accessible. If g_object_unref handler runs first we'll hit this real_dispose vfunc and won't call the notification.
(In reply to Rui Matos from comment #2) > Review of attachment 350394 [details] [review] [review]: > > ::: atk-2.22.0/atk/atkgobjectaccessible.c.test > @@ +177,3 @@ > + g_object_weak_unref (obj, > + (GWeakNotify) atk_gobject_accessible_dispose, > + object); > > IIUC this way we'll never atk_object_notify_state_change() and we still > might have a g_object_unref() being called on free'd memory. > > Note that above in atk_gobject_accessible_for_object() a different weak ref > is added to the main gobject and its notify handler is g_object_unref() with > the accessible instance as the data argument. > > If the atk_gobject_accessible_dispose (really bad name) handler runs first > we do the state change notification and unref the accessible which means > that the other weak ref handler (g_object_unref) will be called on a free'd > accessible. > If g_object_unref handler runs first we'll hit this real_dispose vfunc and > won't call the notification. Yes, you are right. I got confused by the name mess, and didn't realize that in this case we would miss the notification. Will change the state of the patch.
Review of attachment 350394 [details] [review]: As pointed by Rui Matos(thanks), as it is, the object will not notify that the object is defunct if g_object_unref handler runs first.
It means to basically mimic what atk_gobject_accessible_dispose() does, regarding the DEFUNC part, right? With respect of the rename, what about atk_gobject_accessible_object_gone_cb?
Created attachment 351442 [details] [review] proposed patch ][ Added changes as suggested above.
Review of attachment 351442 [details] [review]: it would be nice if you could provide a git formatted patch ::: atk-2.22.0/atk/atkgobjectaccessible.c.use-after-free @@ +170,2 @@ static void +atk_gobject_accessible_dispose (GObject *object) the variable naming in this file is all over the place which confuses things a lot. please call this one atk_obj which is the same variable name used in the _initialize() function for the same instance. @@ +174,3 @@ + + if (obj) { + g_object_set_qdata (object, quark_accessible_object, NULL); I think this should be obj because it's the main gobject which has the atk_obj set as qdata
(In reply to Rui Matos from comment #7) > the variable naming in this file is all over the place which confuses things > a lot. Maybe, but this bug is not about cleaning the file from something unconventional, I hope. > please call this one atk_obj which is the same variable name used in the > _initialize() function for the same instance. I do not think so. There are: atk_gobject_accessible_get_type() atk_gobject_accessible_class_init() and if you even used G_DEFINE_TYPE() macro, then there would also be: atk_gobject_accessible_init() all of these being related to GObject class, not to AtkObject class. The atk_real_gobject_accessible_initialize() is related to AtkObjectClass, not to GObjectClass. That's why I made it: atk_gobject_accessible_dispose() because it follows the pattern. This pattern is not used everywhere, neither I use it always, but I also do not see any issue with it. You can always rename it before committing (see below). > @@ +174,3 @@ > + > + if (obj) { > + g_object_set_qdata (object, quark_accessible_object, NULL); > > I think this should be obj because it's the main gobject which has the > atk_obj set as qdata You are right. The usage of quark_accessible_object in atk_gobject_accessible_for_object() confused me (that function looks like a getter, but behaves like "create on demand". It can eventually use zero quark_accessible_object, which is not correct too, but it's out of this bug report as well.
(In reply to Milan Crha from comment #8) > (In reply to Rui Matos from comment #7) > > the variable naming in this file is all over the place which confuses things > > a lot. > > Maybe, but this bug is not about cleaning the file from something > unconventional, I hope. It isn't. I'm just asking that the new code uses good variable names. > > please call this one atk_obj which is the same variable name used in the > > _initialize() function for the same instance. > > I do not think so. There are: > atk_gobject_accessible_get_type() > atk_gobject_accessible_class_init() > and if you even used G_DEFINE_TYPE() macro, then there would also be: > atk_gobject_accessible_init() > all of these being related to GObject class, not to AtkObject class. The > atk_real_gobject_accessible_initialize() is related to AtkObjectClass, not > to GObjectClass. That's why I made it: > atk_gobject_accessible_dispose() > because it follows the pattern. This pattern is not used everywhere, neither > I use it always, but I also do not see any issue with it. The problem is that in this file we have 2 types of instances: AtkGObjectAccessible and arbitrary GObjects. IMO calling the variable that holds an AtkGObjectAccessible instance 'object' and the variable that holds a GObject instance 'obj' is needlessly confusing. > You can always > rename it before committing (see below). I'm not going to push this btw since I'm not the maintainer, I'm just reviewing to help moving this forward. > > @@ +174,3 @@ > > + > > + if (obj) { > > + g_object_set_qdata (object, quark_accessible_object, NULL); > > > > I think this should be obj because it's the main gobject which has the > > atk_obj set as qdata > > You are right. The usage of quark_accessible_object in > atk_gobject_accessible_for_object() confused me (that function looks like a > getter, but behaves like "create on demand". > It can eventually use zero > quark_accessible_object, which is not correct too, but it's out of this bug > report as well. I don't think that can happen because the quark is created in class_init()
Created attachment 352371 [details] [review] proposed patch ]I[ I'm sorry, but I do not generate the patch from a git checkout, thus git-formatted patch is not an option here. (In reply to Rui Matos from comment #9) > It isn't. I'm just asking that the new code uses good variable names. Ouch, I'm sorry, I misunderstood your plea, I thought you want to rename the function, not the variable. My fault. > I don't think that can happen because the quark is created in class_init() Yes, that's one more confusing thing here. I think it's there, because the class_init() is called only within the first call to _get_type(), which may not happen before the first instance of the object is created, which can be in this function. I can be wrong, but being consistent doesn't hurt, thus I added a code to cover that here as well. This patch should make it, I hope, as it contains all three things discussed above.
Review of attachment 352371 [details] [review]: looks good, but I'd prefer Alejandro to give it a look too ::: atk-2.22.0/atk/atkgobjectaccessible.c.use-after-free @@ +89,3 @@ /* See if we have a cached accessible for this object */ + accessible = quark_accessible_object ? g_object_get_qdata (obj, quark_accessible_object) : NULL; good catch, I missed this case
Review of attachment 352371 [details] [review]: LGTM too. Thanks for the patch. And thanks Rui Matos for the detailed review
Thanks. Unfortunately, I do not have a git checkout of atk, thus if you could, please commit for me. Thanks in advance.
(In reply to Milan Crha from comment #13) > Thanks. Unfortunately, I do not have a git checkout of atk, thus if you > could, please commit for me. Thanks in advance. Done: https://git.gnome.org/browse/atk/commit/?id=b9a89abf12379c2b8497cbbd73eb65f2b498d6dc Thanks for the patch. Closing bug.