GNOME Bugzilla – Bug 771279
Segfault due to gtk_event_controller_get_widget() returning NULL but GtkGesture not checking
Last modified: 2017-10-28 02:13:56 UTC
See the full stack trace here: https://bugs.debian.org/837419 This is in 3.21.5. dest_widget passed into gtk_widget_translate_coordinates() is NULL. That variable is set in _update_widget_coordinates() at gtk/gtkgesture.c:481 via the return value of gtk_event_controller_get_widget(). There's no null check on that return value but there clearly should be because the function contains this line: g_return_val_if_fail (GTK_IS_EVENT_CONTROLLER (controller), 0); The documentation for gtk_event_controller_get_widget() also makes no mention of possibly returning NULL, so that needs to be fixed too: https://developer.gnome.org/gtk3/stable/GtkEventController.html#gtk-event-controller-get-widget Regards, Scott.
(In reply to smlx from comment #0) > There's no null check on that return value but there clearly should be > because the function contains this line: > > g_return_val_if_fail (GTK_IS_EVENT_CONTROLLER (controller), 0); As a general concept, I'm not sure that follows. If an assertion fails, it's probably best to think of everything that happens after is undefined, and quit while you're ahead. The return of a dummy value is just so that _something_ is returned, thus avoiding making a failing program any worse than it already is. That said, here specifically, the setter specifically checks for, and allows, a null Widget being set - so that, and the fact it can be NULL in the wild, both indicate the return value is intended to be nullable. If so, then of course, what you said stands to reason. Presumably we should give up and exit early if it returns NULL there.
Created attachment 358386 [details] [review] EventController: get_widget() returns nullable ptr (In reply to Daniel Boles from comment #1) > That said, here specifically, the setter specifically checks for, and > allows, a null Widget being set - so that, and the fact it can be NULL in > the wild, both indicate the return value is intended to be nullable. If so, > then of course, what you said stands to reason. Presumably we should give up > and exit early if it returns NULL there. gtkeventcontroller.c always NULL-checks priv->widget before using it, so I guess that answers that: the result is unambiguously nullable. However, the question next is whether callers should just exit silently if the widget is NULL, or assert that they shouldn't end up in that situation. That's a question for someone familiar with gestures.
Hmm, I don't think making the parameter nullable is entirely right... A GtkEventController widget can only be NULL in three situations: 1) If the controller was constructed without a widget. This is basically a programming error, as an event controller requires a widget to be functional and meaningful (eg. all GtkGesture coordinates are guaranteed to be in the given widget's coordinate space) 2) After dispose(), on the way to being destroyed. 3) After the given widget loses its last reference. The controller should be destroyed too in consequence, but there may be extra references somewhere else. With no widget, the controller is effectively inert, all you can do with it is destroying it. As per the backtrace in the Debian bug report, the gesture that seems to be getting the NULL widget is one of the ones attached to the GtkWindow for WM management purposes, which pretty much discards #1. However #2 and #3 all have the same issue. For code flow to reach that point we must have successfully figured out a GtkWindow from the event, which means priv->widget ought not to be NULL. And I think it's very unlikely that the window is being destroyed during the processing of this event because no user code has been hit yet. This leaves us with a scarier #4, memory corruption. First things first, Scott, are you still able to reproduce with a recent gtk+ 3.22.x?
(In reply to Carlos Garnacho from comment #3) > Hmm, I don't think making the parameter nullable is entirely right... A > GtkEventController widget can only be NULL in three situations: > > 1) If the controller was constructed without a widget. This is basically a > programming error, as an event controller requires a widget to be functional > and meaningful (eg. all GtkGesture coordinates are guaranteed to be in the > given widget's coordinate space) Shouldn't it then assert that it's not constructed with a NULL widget?
Created attachment 361845 [details] [review] Deference src_widget and dest_widget after we are sure they are valid widgets
This bug seems fixed in master but there are still people able to reproduce this with 3.22 as reported here: https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1724703
Review of attachment 361845 [details] [review]: Seems obviously correct I would try to fit the commit message into 50 characters and specify the module affected, e.g.: widget: Typecheck before deref in translate_coords
Created attachment 361849 [details] [review] widget: Typecheck before deref in translate_coords
Fixe(In reply to Daniel Boles from comment #7) > Review of attachment 361845 [details] [review] [review]: > > Seems obviously correct > > I would try to fit the commit message into 50 characters and specify the > module affected, e.g.: > > widget: Typecheck before deref in translate_coords Fixed. Thanks.
Attachment 361849 [details] pushed as 81d1aaa - widget: Typecheck before deref in translate_coords