GNOME Bugzilla – Bug 712760
Clang static analysis fixes
Last modified: 2016-08-07 07:22:39 UTC
Various small fixes for bugs found by using scan-build on GTK+ (thanks to Murry for blogging about it: http://www.murrayc.com/permalink/2013/11/15/jhbuild-and-clangs-scan-build/). Most of these are on obscure code paths, but some may be hit fairly regularly.
*** Bug 712759 has been marked as a duplicate of this bug. ***
*** Bug 712758 has been marked as a duplicate of this bug. ***
Created attachment 260350 [details] [review] gdkwindow: Fix potential NULL pointer dereference gdk_window_ensure_native() can end up with a NULL parent pointer, which it passes to find_native_parent_above()…but that expects a non-NULL parent. Found with scan-build.
Created attachment 260351 [details] [review] gdkwindow: Remove an unused assignment Found with scan-build.
Created attachment 260352 [details] [review] gdkwindow: Fix potential uses of uninitialised variables Found with scan-build.
Created attachment 260353 [details] [review] gdkwindow: Fix potential NULL pointer dereferences in event code The event code could potentially dereference pointer_info if the invariant that ENTER_NOTIFY and LEAVE_NOTIFY events are only emitted on devices which have pointers is violated elsewhere. Found with scan-build.
Created attachment 260354 [details] [review] gdkwindow: Fix a potentially lost return value from g_slist_append() It was a dead assignment which potentially changed the head pointer of the list, but didn’t store it back correctly. Found with scan-build.
Created attachment 260355 [details] [review] gtkiconviewaccessible: Add a missing return value assignment The return pointer of get_pixbuf_box() was never actually written to. Found with scan-build.
Created attachment 260356 [details] [review] gtktreeviewaccessible: Fix potential uses of uninitialised variables Found with scan-build.
Created attachment 260357 [details] [review] gtktreeviewaccessible: Fix a potential division by zero Found with scan-build.
Created attachment 260358 [details] [review] gtktreeviewaccessible: Fix a potential NULL pointer dereference Found by scan-build.
Created attachment 260359 [details] [review] gtk: Fix potential memcpy() from NULL pointer In both cases the copied size would have been 0 in the NULL pointer case, but let’s keep the static analyser happy and conditionalise the memcpy() call. This is necessary because the memcpy() specification doesn’t state that it’s happy with a NULL source pointer even if the size is 0. Found by scan-build.
Created attachment 260360 [details] [review] gtkcssselector: Fix potential use of an uninitialised variable On the first loop iteration, max_selector is uninitialised and so the gtk_css_selector_compare_one() call is comparing against uninitialised memory. Found with scan-build.
Created attachment 260361 [details] [review] gtkentry: Fix potential use of uninitialised variables Found with scan-build.
Created attachment 260362 [details] [review] gtkentry: Remove dead assignments Found by scan-build.
Created attachment 260363 [details] [review] gtkentry: Add an assertion to help out static analysis This helps scan-build avoid some false positive potential NULL pointer dereference warnings.
Created attachment 260364 [details] [review] gtkicontheme: Fix potential use of uninitialised variables Found by scan-build.
Created attachment 260366 [details] [review] gtkmenu: Fix potential use of uninitialised variables The child_height out variable is only valid if compute_child_offset() returns TRUE. Found by scan-build.
Created attachment 260367 [details] [review] gtknotebook: Fix potential use of uninitialised variables Found by scan-build.
Created attachment 260368 [details] [review] gtknotebook: Fix a potential NULL pointer dereference Found by scan-build.
Created attachment 260369 [details] [review] gtknotebook: Remove dead variable assignments Found by scan-build.
Created attachment 260370 [details] [review] gtkpapersize: Reformat #if preprocessor commands This eliminates some false positive warnings from scan-build, which was not interpreting the #ifs and hence warning about unbalanced #endifs.
Created attachment 260371 [details] [review] gtktextview: Fix a definite use of an uninitialised variable This seems to have been a typo in the original code, and allowed access to virtual_cursor_y when it was uninitialised. Found by scan-build.
Created attachment 260372 [details] [review] gtkthemingengine: Fix a definite use of an uninitialised variable At this point, segments[1] is always uninitialised, and is used to initialise itself. Looking at the code in the branch above, this appears to have been a typo from segments[0], as segments[1] seems to typically be 2 * segments[0]. Found by scan-build.
Created attachment 260373 [details] [review] gtkthemingengine: Eliminate a dead assignment This is technically a dead assignment, but is nice to retain for clarity. Moving it to the variable definition shuts scan-build up.
Created attachment 260374 [details] [review] gtktreeselection: Eliminate a dead assignment This is technically a dead assignment, but is nice to retain for clarity. Moving it to the variable definition shuts scan-build up.
Created attachment 260375 [details] [review] gtktreeselection: Fix potential NULL pointer dereferences _gtk_rbtree_first() can potentially return NULL if the RB tree is empty, which would result in NULL pointer dereferences in the GtkTreeSelection code. Gracefully handle them. Found by scan-build.
Created attachment 260376 [details] [review] gtktreeselection: Fix a potential use of uninitialised variables Found by scan-build.
Created attachment 260377 [details] [review] testsuite: Fix potential strcmp() against NULL Use g_strcmp0() instead. Found by scan-build.
Review of attachment 260351 [details] [review]: ::: gdk/gdkwindow.c @@ +9341,3 @@ if (scale == 0) scale = gdk_window_get_scale_factor (window); +#endif I don't like to add ifdefs for this. It is a temporary condition - the cairo with this API will become more common over time.
Review of attachment 260352 [details] [review]: ok
Review of attachment 260350 [details] [review]: ::: gdk/gdkwindow.c @@ +1102,3 @@ + g_return_val_if_fail (parent != NULL, NULL); + g_return_val_if_fail (child != NULL, NULL); I don't like g_return check in internal static functions. Also, this is not correct, the code is hanlding child == NULL and is even explicitly recursing with child == NULL.
Review of attachment 260354 [details] [review]: ::: gdk/gdkwindow.c @@ +3284,3 @@ if (! tmp->next && has_ancestor_in_list) { + update_windows = g_slist_append (tmp, window); No, this is wrong. If tmp is non-NULL (and it can't be here), then g_slist_append(tmp, window) will not modify the list head, so there is no need to store it. And in particular, overwriting the actual list head with tmp is going to lose stuff - remember, tmp is the last list element, so if you overwrite update_windows with it, your list will only have 2 members afterwards.
Review of attachment 260357 [details] [review]: sure
Review of attachment 260377 [details] [review]: sure
commit 9d7a442b8b76fc01c51864549b34ee9b85931f21 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Nov 20 17:39:21 2013 +0000 testsuite: Fix potential strcmp() against NULL Use g_strcmp0() instead. Found by scan-build. https://bugzilla.gnome.org/show_bug.cgi?id=712760 testsuite/gtk/object.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) commit ada766025f1dc32e494e83b806ad625d0756ab1d Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Nov 20 17:25:48 2013 +0000 gtktreeviewaccessible: Fix a potential division by zero Found with scan-build. https://bugzilla.gnome.org/show_bug.cgi?id=712760 gtk/a11y/gtktreeviewaccessible.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) commit 86b617094067f90236d67b099a68f7b8f54667ce Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Nov 20 17:20:33 2013 +0000 gdkwindow: Fix potential uses of uninitialised variables Found with scan-build. https://bugzilla.gnome.org/show_bug.cgi?id=712760 gdk/gdkwindow.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Created attachment 261392 [details] [review] gdkwindow: Fix potential NULL pointer dereference gdk_window_ensure_native() can end up with a NULL parent pointer, which it passes to find_native_parent_above()…but that expects a non-NULL parent. Found with scan-build.
Created attachment 261393 [details] [review] gdkwindow: Remove an unused assignment scale is only used if HAVE_CAIRO_SURFACE_SET_DEVICE_SCALE is defined. Found with scan-build.
Created attachment 261394 [details] [review] gdkwindow: Fix potential NULL pointer dereferences in event code The event code could potentially dereference pointer_info if the invariant that ENTER_NOTIFY and LEAVE_NOTIFY events are only emitted on devices which have pointers is violated elsewhere. Found with scan-build.
Created attachment 261395 [details] [review] gdkwindow: Fix a potentially lost return value from g_slist_append() It was a dead assignment to a local variable. The g_slist_append() call is guaranteed to operate part-way through the list, so there’s no need to store back to the head of the list. Since it seems to be impossible to shut the unused result warning up by casting, and assigning to a local variable produces a scan-build warning about a dead assignment, expanding g_slist_append() in-place to allocate and chain the new list element. Found with scan-build.
Created attachment 261396 [details] [review] gtkiconviewaccessible: Add a missing return value assignment The return pointer of get_pixbuf_box() was never actually written to. Found with scan-build.
Created attachment 261397 [details] [review] gtktreeviewaccessible: Fix potential uses of uninitialised variables Found with scan-build.
Created attachment 261398 [details] [review] gtktreeviewaccessible: Fix a potential NULL pointer dereference Found by scan-build.
Created attachment 261399 [details] [review] gtk: Fix potential memcpy() from NULL pointer In both cases the copied size would have been 0 in the NULL pointer case, but let’s keep the static analyser happy and conditionalise the memcpy() call. This is necessary because the memcpy() specification doesn’t state that it’s happy with a NULL source pointer even if the size is 0. Found by scan-build.
Created attachment 261400 [details] [review] gtkcssselector: Fix potential use of an uninitialised variable On the first loop iteration, max_selector is uninitialised and so the gtk_css_selector_compare_one() call is comparing against uninitialised memory. Found with scan-build.
Created attachment 261401 [details] [review] gtkentry: Fix potential use of uninitialised variables Found with scan-build.
Created attachment 261402 [details] [review] gtkentry: Remove dead assignments Found by scan-build.
Created attachment 261403 [details] [review] gtkentry: Add an assertion to help out static analysis This helps scan-build avoid some false positive potential NULL pointer dereference warnings.
Created attachment 261404 [details] [review] gtkicontheme: Fix potential use of uninitialised variables Found by scan-build.
Created attachment 261405 [details] [review] gtkmenu: Fix potential use of uninitialised variables The child_height out variable is only valid if compute_child_offset() returns TRUE. Found by scan-build.
Created attachment 261406 [details] [review] gtknotebook: Fix potential use of uninitialised variables Found by scan-build.
Created attachment 261407 [details] [review] gtknotebook: Fix a potential NULL pointer dereference Found by scan-build.
Created attachment 261408 [details] [review] gtknotebook: Remove dead variable assignments Found by scan-build.
Created attachment 261409 [details] [review] gtkpapersize: Reformat #if preprocessor commands This eliminates some false positive warnings from scan-build, which was not interpreting the #ifs and hence warning about unbalanced #endifs.
Created attachment 261410 [details] [review] gtktextview: Fix a definite use of an uninitialised variable This seems to have been a typo in the original code, and allowed access to virtual_cursor_y when it was uninitialised. Found by scan-build.
Created attachment 261411 [details] [review] gtkthemingengine: Fix a definite use of an uninitialised variable At this point, segments[1] is always uninitialised, and is used to initialise itself. Looking at the code in the branch above, this appears to have been a typo from segments[0], as segments[1] seems to typically be 2 * segments[0]. Found by scan-build.
Created attachment 261412 [details] [review] gtkthemingengine: Eliminate a dead assignment This is technically a dead assignment, but is nice to retain for clarity. Moving it to the variable definition shuts scan-build up.
Created attachment 261413 [details] [review] gtktreeselection: Eliminate a dead assignment This is technically a dead assignment, but is nice to retain for clarity. Moving it to the variable definition shuts scan-build up.
Created attachment 261414 [details] [review] gtktreeselection: Fix potential NULL pointer dereferences _gtk_rbtree_first() can potentially return NULL if the RB tree is empty, which would result in NULL pointer dereferences in the GtkTreeSelection code. Gracefully handle them. Found by scan-build.
Created attachment 261415 [details] [review] gtktreeselection: Fix a potential use of uninitialised variables Found by scan-build.
Created attachment 262865 [details] [review] gtkwidget: Fix a potential g_object_ref(NULL) call If gtk_widget_set_visual() is called with a NULL visual, it will attempt to reference it, which will cause a critical error. Found by scan-build.
Created attachment 262866 [details] [review] gtkprintcontext: Fix several potential g_object_[un]ref(NULL) calls The page_setup of a GtkPrintContext or GtkPrintUnixDialog is nullable, so all reference count changes to it have to be guarded against NULL values. Found by scan-build.
Created attachment 262867 [details] [review] gtkprintunixdialog: Remove a redundant (GFile != NULL) check g_file_new_for_uri() is guaranteed to return a non-NULL value, so this check was redundant, and was confusing the static analyser into returning a false positive, where it thought the file could be NULL.
Created attachment 262868 [details] [review] gtkprintunixdialog: Fix a potential g_object_unref(NULL) call The code above checks whether (printer == NULL), so we’d better do so here as well. Found by scan-build.
Created attachment 263077 [details] [review] gtkclipboard: Fix a potential g_signal_handler_disconnect(NULL) call Found by scan-build.
Review of attachment 261393 [details] [review]: ::: gdk/gdkwindow.c @@ -1551,2 @@ was_mapped = GDK_WINDOW_IS_MAPPED (window); - show = FALSE; This hunk is unrelated to the commit message and should probably be a separate commit. @@ +9359,3 @@ + if (scale == 0) + scale = gdk_window_get_scale_factor (window); + This looks ok.
Review of attachment 261394 [details] [review]: The commit message is a bit misleading. Dereferencing pointer_info is nothing bad - you probably wanted to say 'dereference a NULL pointer_info'
Review of attachment 261395 [details] [review]: Nope. The code is fine as is. Not working around stupidity in the scanner like this. False positives are a fact of life when working with static analysis.
Review of attachment 261396 [details] [review]: Fixed differently
Review of attachment 261397 [details] [review]: looks ok.
Review of attachment 261398 [details] [review]: I am pretty sure that cell_info->node can never be NULL.
Review of attachment 261399 [details] [review]: I don't care about the happiness of static analyzers, sorry.
Review of attachment 261400 [details] [review]: If you look at the code populating the hash table, you will find that all the values are > 0, so in the first loop iteration, value > max_count is guaranteed to be true.
Review of attachment 261401 [details] [review]: ::: gtk/gtkentry.c @@ +3520,3 @@ + if (height) + *height = 0; + No. This is broken. If the static analyzer gets upset here, we should rather call class->get_text_area_size unconditionally. @@ +3607,3 @@ + *width = 0; + if (height) + *height = 0; Same here. These vfuncs cannot be NULL @@ +10143,3 @@ gdouble old_fraction; gint x, y, width, height; + gint old_x = 0, old_y = 0, old_width = 0, old_height = 0; This seems harmless enough.
Review of attachment 261402 [details] [review]: Looks ok otherwise ::: gtk/gtkentry.c @@ +3766,2 @@ width += 2 * priv->focus_width; height += 2 * priv->focus_width; I prefer to instead pass x, y into the gtk_render_focus call.
Review of attachment 261403 [details] [review]: ::: gtk/gtkentry.c @@ +7177,3 @@ + g_assert (icon_info != NULL); + That is not clear at all to me. I suggest to instead add a if (icon_info == NULL) return; at the very top (and remove the icon_info check two lines up as well).
Review of attachment 261404 [details] [review]: Here, I'd rather check the return value of those icon_info_scale_point calls and return FALSE if either of them does.
Review of attachment 261405 [details] [review]: ok
Review of attachment 261411 [details] [review]: looks right
Review of attachment 261412 [details] [review]: ok
Review of attachment 261409 [details] [review]: ok
Review of attachment 261413 [details] [review]: ok
Review of attachment 262865 [details] [review]: ::: gtk/gtkwidget.c @@ +10857,1 @@ g_object_unref); I prefer to keep the ref right next to the unref here. Can we make this visual ? g_object_ref (visual) : NULL instead ?
Review of attachment 262868 [details] [review]: ::: gtk/gtkprintunixdialog.c @@ +937,3 @@ g_object_set (cell, "sensitive", TRUE, NULL); + g_clear_object (&printer); Just a simple if (printer), please
Review of attachment 262867 [details] [review]: ok
Attachment 261397 [details] pushed as 480a005 - gtktreeviewaccessible: Fix potential uses of uninitialised variables Attachment 261405 [details] pushed as a265d8f - gtkmenu: Fix potential use of uninitialised variables Attachment 261409 [details] pushed as 99a162c - gtkpapersize: Reformat #if preprocessor commands Attachment 261411 [details] pushed as 5c5390f - gtkthemingengine: Fix a definite use of an uninitialised variable Attachment 261412 [details] pushed as 1adf0be - gtkthemingengine: Eliminate a dead assignment Attachment 261413 [details] pushed as 3801503 - gtktreeselection: Eliminate a dead assignment Attachment 262867 [details] pushed as b37f8b8 - gtkprintunixdialog: Remove a redundant (GFile != NULL) check
Comment on attachment 261393 [details] [review] gdkwindow: Remove an unused assignment (In reply to comment #66) > Review of attachment 261393 [details] [review]: > > ::: gdk/gdkwindow.c > @@ -1551,2 @@ > was_mapped = GDK_WINDOW_IS_MAPPED (window); > - show = FALSE; > > This hunk is unrelated to the commit message and should probably be a separate > commit. > > @@ +9359,3 @@ > + if (scale == 0) > + scale = gdk_window_get_scale_factor (window); > + > > This looks ok. Split up and pushed. Attachment 261393 [details] pushed as c73bdb2 - gdkwindow: Remove an unused assignment
(In reply to comment #68) > Review of attachment 261395 [details] [review]: > > Nope. The code is fine as is. Not working around stupidity in the scanner like > this. False positives are a fact of life when working with static analysis. The code contains a dead assignment (which will probably be optimised out) and is not particularly self-explanatory. At a first reading, it can easily look like the author accidentally assigned the result to the wrong variable (local instead of heap). Let’s leave it alone though.
(In reply to comment #72) > Review of attachment 261398 [details] [review]: > > I am pretty sure that cell_info->node can never be NULL. You’re right. The scanner was coming up with a false positive because the GTK_RBNODE_FLAG_* macros check whether the node is NULL. The scanner assumes that explicit NULL checks imply a variable is nullable. Patch dropped.
(In reply to comment #74) > Review of attachment 261400 [details] [review]: > > If you look at the code populating the hash table, you will find that all the > values are > 0, so in the first loop iteration, value > max_count is guaranteed > to be true. That’s not especially obvious. Would you accept a patch to document this with a comment?
Created attachment 263278 [details] [review] gdkwindow: Fix potential NULL pointer dereference gdk_window_ensure_native() can end up with a NULL parent pointer, which it passes to find_native_parent_above()…but that expects a non-NULL parent. Found with scan-build.
Created attachment 263279 [details] [review] gdkwindow: Fix potential NULL pointer dereferences in event code The event code could potentially dereference pointer_info if the invariant that ENTER_NOTIFY and LEAVE_NOTIFY events are only emitted on devices which have pointers is violated elsewhere. Found with scan-build.
Created attachment 263280 [details] [review] gtkentry: Fix potential use of uninitialised variables Found with scan-build.
Created attachment 263281 [details] [review] gtkentry: Call get_*_size() vfuncs unconditionally These vfuncs cannot be NULL: implementations are provided by GtkEntry, and subclasses should not set them to NULL. Instead of conditionalising the calls to the vfuncs, assert that they’re set and call them unconditionally. This prevents the possibility of a subclass setting the vfunc to NULL and then a gtk_entry_get_*_size() call returning undefined values in its out variables.
Created attachment 263282 [details] [review] gtkentry: Remove dead assignments Found by scan-build.
Created attachment 263283 [details] [review] gtkentry: Fix dead assignments The assignments to x and y were dead on this code path; fix that by passing them to gtk_render_focus(). This is almost functionally equivalent to the previous code (where x and y were hard-coded to 0 in the function call), as the visible-focus code path can only be taken if the focus code path has already been taken, giving an increment and a decrement of the x and y values. If the spin button code path has been taken, the x value may differ from before.
Created attachment 263284 [details] [review] gtkentry: Return early from gtk_entry_clear() if no icon info exists This helps scan-build avoid some false positive potential NULL pointer dereference warnings.
Created attachment 263285 [details] [review] gtkicontheme: Fix potential use of uninitialised variables If either icon_info_scale_point() call returns FALSE, the scaled_* variables may be uninitialised. Found by scan-build.
Created attachment 263286 [details] [review] gtknotebook: Fix potential use of uninitialised variables Found by scan-build.
Created attachment 263287 [details] [review] gtknotebook: Fix a potential NULL pointer dereference Found by scan-build.
Created attachment 263288 [details] [review] gtknotebook: Remove dead variable assignments Found by scan-build.
Created attachment 263289 [details] [review] gtktextview: Fix a definite use of an uninitialised variable This seems to have been a typo in the original code, and allowed access to virtual_cursor_y when it was uninitialised. Found by scan-build.
Created attachment 263290 [details] [review] gtktreeselection: Fix potential NULL pointer dereferences _gtk_rbtree_first() can potentially return NULL if the RB tree is empty, which would result in NULL pointer dereferences in the GtkTreeSelection code. Gracefully handle them. Found by scan-build.
Created attachment 263291 [details] [review] gtktreeselection: Fix a potential use of uninitialised variables Found by scan-build.
Created attachment 263292 [details] [review] gtkwidget: Fix a potential g_object_ref(NULL) call If gtk_widget_set_visual() is called with a NULL visual, it will attempt to reference it, which will cause a critical error. Found by scan-build.
Created attachment 263293 [details] [review] gtkprintcontext: Fix several potential g_object_[un]ref(NULL) calls The page_setup of a GtkPrintContext or GtkPrintUnixDialog is nullable, so all reference count changes to it have to be guarded against NULL values. Found by scan-build.
Created attachment 263294 [details] [review] gtkprintunixdialog: Fix a potential g_object_unref(NULL) call The code above checks whether (printer == NULL), so we’d better do so here as well. Found by scan-build.
Created attachment 263295 [details] [review] gtkclipboard: Fix a potential g_signal_handler_disconnect(NULL) call Found by scan-build.
Review of attachment 263278 [details] [review]: ok
Review of attachment 263279 [details] [review]: It looks to me like we dereference pointer_info much earlier in that function though. Since you talk about invariants being violated, maybe an assert would be in order ?
Review of attachment 263280 [details] [review]: Really a false positive. The compiler doesn't know that the drawability of the widget is not going to change between the two calls. But sure, lets help it out.
Review of attachment 263281 [details] [review]: not sure the assertion is really useful here. More useful would be documentation for subclassing that spells out which vfuncs are required
Review of attachment 263282 [details] [review]: sure
Review of attachment 263283 [details] [review]: render_focus is not called here anymore
Review of attachment 263284 [details] [review]: ok
Review of attachment 263285 [details] [review]: ::: gtk/gtkicontheme.c @@ +4953,3 @@ + { + return FALSE; + } Drop the {} here. I would probably write this as two ifs just to make it less unwieldy
Review of attachment 263286 [details] [review]: ::: gtk/gtknotebook.c @@ +1768,1 @@ switch (tab_pos) I'd prefer one of two alternatives: 1) default: g_assert_not_reached (); 2) rewrite the switch as if-else @@ +6373,3 @@ GtkWidget *widget = GTK_WIDGET (notebook); GtkNotebookPrivate *priv = notebook->priv; + GtkAllocation child_allocation = { 0, }, label_allocation; Same here. @@ +8083,3 @@ + if (fill) + *fill = FALSE; + I don't like this much. I'd rather turn the silent return into a g_return_if_fail
Review of attachment 263287 [details] [review]: there's more places where cur_page should probably be checked
Review of attachment 263288 [details] [review]: ok
Review of attachment 263289 [details] [review]: ok
Review of attachment 263290 [details] [review]: ok. would be nice to have some tests around selection in empty trees, perhaps
Review of attachment 263291 [details] [review]: ok
Review of attachment 263292 [details] [review]: I just fixed this
Review of attachment 263293 [details] [review]: ok
Review of attachment 263294 [details] [review]: ::: gtk/gtkprintunixdialog.c @@ +941,1 @@ } Could go with clear_object instead, as in the previous patch
Review of attachment 263295 [details] [review]: ok
Attachment 263278 [details] pushed as 586240d - gdkwindow: Fix potential NULL pointer dereference Attachment 263280 [details] pushed as 0282714 - gtkentry: Fix potential use of uninitialised variables Attachment 263281 [details] pushed as 7479133 - gtkentry: Call get_*_size() vfuncs unconditionally Attachment 263282 [details] pushed as 19222a6 - gtkentry: Remove dead assignments Attachment 263284 [details] pushed as 61c46d9 - gtkentry: Return early from gtk_entry_clear() if no icon info exists Attachment 263287 [details] pushed as c7f5f10 - gtknotebook: Fix a potential NULL pointer dereference Attachment 263288 [details] pushed as 51971d5 - gtknotebook: Remove dead variable assignments Attachment 263289 [details] pushed as 4fc6880 - gtktextview: Fix a definite use of an uninitialised variable Attachment 263290 [details] pushed as 968780d - gtktreeselection: Fix potential NULL pointer dereferences Attachment 263291 [details] pushed as 8e3b499 - gtktreeselection: Fix a potential use of uninitialised variables Attachment 263293 [details] pushed as af36220 - gtkprintcontext: Fix several potential g_object_[un]ref(NULL) calls Attachment 263295 [details] pushed as f8eac08 - gtkclipboard: Fix a potential g_signal_handler_disconnect(NULL) call
Review of attachment 263283 [details] [review]: (In reply to Matthias Clasen from comment #115) > Review of attachment 263283 [details] [review] [review]: > > render_focus is not called here anymore => rejected, since the problem doesn’t exist any more.
Review of attachment 263285 [details] [review]: gtk_icon_info_get_embedded_rect() has been deprecated and its implementation removed, so this patch is no longer relevant.
(In reply to Matthias Clasen from comment #111) > Review of attachment 263279 [details] [review] [review]: > > It looks to me like we dereference pointer_info much earlier in that > function though. > Since you talk about invariants being violated, maybe an assert would be in > order ? We only dereference it on the path where it’s originally assigned, so it should never be NULL there, since @device is non-NULL. The invariant I was suggesting is that pointer_info is non-NULL for (event->type == GDK_ENTER_NOTIFY || event->type == GDK_LEAVE_NOTIFY), but that’s an invariant which would have to apply to the whole of the GDK code base. The other invariants I’ve proposed have applied to a single class or function, so are a lot easier to apply (and maintain). If you want to go with enforcing that invariant across all code, I can amend the patch; my original idea was that simply checking (pointer_info != NULL) would improve safety in this code without needing to worry about the behaviour of the rest of GDK. Updated patches coming for the other two.
Created attachment 298884 [details] [review] gtknotebook: Fix potential use of uninitialised variables Found by scan-build.
Created attachment 298885 [details] [review] gtkprintunixdialog: Fix a potential g_object_unref(NULL) call The code above checks whether (printer == NULL), so we’d better do so here as well. Found by scan-build.
Created attachment 298890 [details] [review] gtknotebook: Fix potential use of uninitialised variables Found by scan-build.
Review of attachment 298885 [details] [review]: sure
Review of attachment 298890 [details] [review]: sure, looks fine
Thanks for all your reviews. That leaves attachment #263279 [details] (comment #111 and comment #131), and the follow up you suggested in comment #119, comment #122 and comment #113. Attachment 298885 [details] pushed as ac4da77 - gtkprintunixdialog: Fix a potential g_object_unref(NULL) call Attachment 298890 [details] pushed as c352093 - gtknotebook: Fix potential use of uninitialised variables
Created attachment 298953 [details] [review] gtkentry: Document vfuncs in GtkEntryClass Clarify that the signal handler vfuncs can be NULL, nothing else can, and that they all have default implementations.
Created attachment 298954 [details] [review] gtktreeselection: Fix an abort on selecting an invalid range gtk_tree_selection_real_modify_range() has a g_return_if_fail() if the start or end paths passed to it do not correspond to real tree nodes. However, GtkTreePaths inherently do not have to be valid, so it should be acceptable to call gtk_tree_selection_select_range() with non-existent paths. Replace the g_return_if_fail() by a silent return, and add a unit test.
Created attachment 298955 [details] [review] gtknotebook: Add more non-NULL checks for cur_page These were not spotted by scan-build, but from some brief mental reasoning could potentially be problem areas.
Review of attachment 298953 [details] [review]: ok
Review of attachment 298954 [details] [review]: thanks for adding tests
Review of attachment 298955 [details] [review]: ok
One patch left: attachment #263279 [details] (comment #111 and comment #131). Attachment 298953 [details] pushed as 8001343 - gtkentry: Document vfuncs in GtkEntryClass Attachment 298954 [details] pushed as 52858f7 - gtktreeselection: Fix an abort on selecting an invalid range Attachment 298955 [details] pushed as d65ccf9 - gtknotebook: Add more non-NULL checks for cur_page
Pushed attachment #263279 [details] without changes, since the other blocks in that function check pointer_info inline rather than with an assertion already. Attachment 263279 [details] pushed as 7b40fdb - gdkwindow: Fix potential NULL pointer dereferences in event code