Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
Widget toolkits are used to make the process of application GUI development easier, and GTK+ is one of them. It is this project that I picked for my first article about the PVS-Studio analyzer. I scanned the code of GTK+ with PVS-Studio for possible bugs and got quite a lot of messages about errors and suspicious fragments. Some of them are pretty critical. The total number of bugs is too big for an article, so I will talk only about some of them, which are the most typical.
GTK+ (abbrev. of GIMP ToolKit) is a cross-platform widget toolkit for creating graphical user interfaces. It is licensed under the terms of the LGPL, allowing both free and proprietary software to use it. It is one of the most popular toolkits for the Wayland and X11 windowing systems, along with Qt.
We scanned the toolkit's code with the PVS-Studio static analyzer, version 6.02, and studied the diagnostic messages.
For a start, let's discuss warnings that deal with forming logical expressions. Such issues aren't always bugs; they are simply extra checks that make conditions harder to read and understand. Expressions with such checks can be simplified to a large extent.
V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!mount' and 'mount'. gtkplacesview.c 708
static void
add_volume (....)
{
....
GMount *mount;
....
if (!mount ||
(mount && !g_mount_is_shadowed (mount)))
....
}This code contains an extra check of the 'mount' pointer and can be modified as follows:
if (!mount || !g_mount_is_shadowed (mount)))Another similar case:
V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'ret' and '!ret'. gtktreeview.c 13682
void
gtk_tree_view_get_cell_area (....)
{
....
gboolean ret = ...;
....
/* Get vertical coords */
if ((!ret && tree == NULL) || ret)
....
}One more superfluous check; this time, it is Boolean variable 'ret'. Let's simplify the code:
if (ret || tree == NULL)V590 Consider inspecting the 'str[0] == '\0' || str[0] != 'U'' expression. The expression is excessive or contains a misprint. gtkcomposetable.c 62
static gboolean
is_codepoint (const gchar *str)
{
int i;
/* 'U' is not code point but 'U00C0' is code point */
if (str[0] == '\0' || str[0] != 'U' || str[1] == '\0')
return FALSE;
for (i = 1; str[i] != '\0'; i++)
{
if (!g_ascii_isxdigit (str[i]))
return FALSE;
}
return TRUE;
}The str[0] == '\0' check is redundant since it is a special case of the str[0] != 'U' expression. We can simplify the code by removing the extra check:
if (str[0] != 'U' || str[1] == '\0')
return FALSE;All these issues are not really errors. The code will successfully execute; it's just that it contains some unnecessary checks, which will be executed too.
Software development industry relies heavily on code reuse. Indeed, why reinvent the wheel? A very common source of errors is the copy-paste technique, when code blocks are copied and then slightly edited. Programmers tend to skip such blocks, forgetting to fix them, and it results in bugs. One of PVS-Studio's strong points is the ability to detect such fragments.
Here are a few examples of errors caused by misuse of copy-paste:
V523 The 'then' statement is equivalent to the 'else' statement. gtkprogressbar.c 1232
static void
gtk_progress_bar_act_mode_enter (GtkProgressBar *pbar)
{
....
/* calculate start pos */
if (orientation == GTK_ORIENTATION_HORIZONTAL)
{
if (!inverted)
{
priv->activity_pos = 0.0;
priv->activity_dir = 0;
}
else
{
priv->activity_pos = 1.0;
priv->activity_dir = 1;
}
}
else
{
if (!inverted)
{
priv->activity_pos = 0.0;
priv->activity_dir = 0;
}
else
{
priv->activity_pos = 1.0;
priv->activity_dir = 1;
}
}
....
}The block of the 'if (orientation == GTK_ORIENTATION_HORIZONTAL)' statement and the corresponding else-block contain the same code. It may be either incomplete functionality or a bug.
V501 There are identical sub-expressions '(box->corner[GTK_CSS_TOP_RIGHT].horizontal)' to the left and to the right of the '>' operator. gtkcssshadowvalue.c 685
V501 There are identical sub-expressions '(box->corner[GTK_CSS_TOP_LEFT].horizontal)' to the left and to the right of the '>' operator. gtkcssshadowvalue.c 696
static void
draw_shadow_corner (....
GtkRoundedBox *box,
....)
{
....
overlapped = FALSE;
if (corner == GTK_CSS_TOP_LEFT ||
corner == GTK_CSS_BOTTOM_LEFT)
{
....
max_other = MAX(box->corner[GTK_CSS_TOP_RIGHT].horizontal,
box->corner[GTK_CSS_TOP_RIGHT].horizontal);
....
}
else
{
....
max_other = MAX(box->corner[GTK_CSS_TOP_LEFT].horizontal
box->corner[GTK_CSS_TOP_LEFT].horizontal);
....
}
....
}The MAX macro receives identical variables as its arguments. Perhaps the programmer forgot to replace 'GTK_CSS_TOP_RIGHT' and 'GTK_CSS_TOP_LEFT' with the appropriate constant values; or maybe the comparison should have involved quite a different variable.
V501 There are identical sub-expressions 'G_PARAM_EXPLICIT_NOTIFY' to the left and to the right of the '|' operator. gtkcalendar.c 400
static void
gtk_calendar_class_init (GtkCalendarClass *class)
{
....
g_object_class_install_property (gobject_class,
PROP_YEAR,
g_param_spec_int ("year",
P_("Year"),
P_("The selected year"),
0, G_MAXINT >> 9, 0,
GTK_PARAM_READWRITE|G_PARAM_EXPLICIT_NOTIFY|
G_PARAM_EXPLICIT_NOTIFY));
....
}In this code, either the 'G_PARAM_EXPLICIT_NOTIFY' constant was copied one extra time or the programmer forgot to replace it with another constant.
Another similar case, dealing with constant 'G_PARAM_DEPRECATED':
V501 There are identical sub-expressions 'G_PARAM_DEPRECATED' to the left and to the right of the '|' operator. gtkmenubar.c 275
static void
gtk_menu_bar_class_init (GtkMenuBarClass *class)
{
....
gtk_widget_class_install_style_property (widget_class,
g_param_spec_int ("internal-padding",
P_("Internal padding"),
P_("Amount of border space between ...."),
0, G_MAXINT, 0,
GTK_PARAM_READABLE |
G_PARAM_DEPRECATED|G_PARAM_DEPRECATED));
....
}Mistakes related to copy-paste can be often found lurking in long initialization lists. They are difficult to notice for a human, and that's where a static analyzer can help you out.
The example below contains a very long initialization list, so it's no surprise there's an error inside it:
V519 The 'impl_class->set_functions' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 5760, 5761. gdkwindow-x11.c 5761
static void
gdk_window_impl_x11_class_init (GdkWindowImplX11Class *klass)
{
....
GdkWindowImplClass *impl_class = GDK_WINDOW_IMPL_CLASS (klass);
....
impl_class->set_decorations = gdk_x11_window_set_decorations;
impl_class->get_decorations = gdk_x11_window_get_decorations;
impl_class->set_functions = gdk_x11_window_set_functions;
impl_class->set_functions = gdk_x11_window_set_functions;
....
}At first I thought that there is a missing 'get-set' pair, like in the previous case: 'set_functions' and 'get_functions'. But the 'GdkWindowImplClass' structure turned out to have no 'get_functions' field. Perhaps the programmer made an extra copy of the initializing line by mistake, or maybe they wanted to replace it with some other code but forgot all about it. Anyway, they need to make sure they initialize everything that ought to be initialized, and remove the extra statement if necessary.
The next warning is similar to the previous one:
V519 The 'impl_class->set_functions' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1613, 1614. gdkwindow-broadway.c 1614
static void
gdk_window_impl_broadway_class_init
(GdkWindowImplBroadwayClass *klass)
{
....
GdkWindowImplClass *impl_class = GDK_WINDOW_IMPL_CLASS (klass);
....
impl_class->set_functions =
gdk_broadway_window_set_functions;
impl_class->set_functions =
gdk_broadway_window_set_functions;
....
}Again, we deal with duplicated assignment 'impl_class->set_functions'. It may have migrated from the previous example.
Sometimes, similarly looking functions are copied in full and programmers forget to modify their bodies. Let's help these forgetful programmers and fix the bugs found:
V524 It is odd that the body of 'gtk_mirror_bin_get_preferred_height' function is fully equivalent to the body of 'gtk_mirror_bin_get_preferred_width' function. offscreen_window2.c 340
static void
gtk_mirror_bin_get_preferred_width (GtkWidget *widget,
gint *minimum,
gint *natural)
{
GtkRequisition requisition;
gtk_mirror_bin_size_request (widget, &requisition);
*minimum = *natural = requisition.width;
}
static void
gtk_mirror_bin_get_preferred_height (GtkWidget *widget,
gint *minimum,
gint *natural)
{
GtkRequisition requisition;
gtk_mirror_bin_size_request (widget, &requisition);
*minimum = *natural = requisition.width;
}In the gtk_mirror_bin_get_preferred_height function, 'requisition.height' must be probably used instead of 'requisition.width'. Then it should look like this:
*minimum = *natural = requisition.height;Maybe it was originally conceived exactly that way and there is no error, but this code does look strange.
Here is one more example where, I believe, the width and the length are also confused:
V524 It is odd that the body of 'gtk_hsv_get_preferred_height' function is fully equivalent to the body of 'gtk_hsv_get_preferred_width' function. gtkhsv.c 310
static void
gtk_hsv_get_preferred_width (GtkWidget *widget,
gint *minimum,
gint *natural)
{
GtkHSV *hsv = GTK_HSV (widget);
GtkHSVPrivate *priv = hsv->priv;
gint focus_width;
gint focus_pad;
gtk_widget_style_get (widget,
"focus-line-width", &focus_width,
"focus-padding", &focus_pad,
NULL);
*minimum = priv->size + 2 * (focus_width + focus_pad);
*natural = priv->size + 2 * (focus_width + focus_pad);
}
static void
gtk_hsv_get_preferred_height (GtkWidget *widget,
gint *minimum,
gint *natural)
{
GtkHSV *hsv = GTK_HSV (widget);
GtkHSVPrivate *priv = hsv->priv;
gint focus_width;
gint focus_pad;
gtk_widget_style_get (widget,
"focus-line-width", &focus_width,
"focus-padding", &focus_pad,
NULL);
*minimum = priv->size + 2 * (focus_width + focus_pad);
*natural = priv->size + 2 * (focus_width + focus_pad);
}Since both the height and the width are calculated in the same way, it's probably better to use one function instead of two. But if these expressions were meant to be different, one should remember to do the necessary edits.
In the next example, it's not quite clear which argument must be used instead of the copied argument 'component_name', but it's obviously strange to compare a string to itself:
V549 The first argument of 'strcmp' function is equal to the second argument. gtkrc.c 1400
GtkStyle *
gtk_rc_get_style_by_paths (....)
{
....
pos = gtk_widget_path_append_type (path, component_type);
if (component_name != NULL &&
strcmp (component_name, component_name) != 0) // <=
gtk_widget_path_iter_set_name (path, pos, component_name);
....
}Going on with warnings related to code copying:
V570 The 'tmp_info' variable is assigned to itself. gtkimcontextxim.c 442
static GtkXIMInfo *
get_im (....)
{
....
GtkXIMInfo *info;
....
info = NULL;
tmp_list = open_ims;
while (tmp_list)
{
....
else
{
tmp_info = tmp_info; // <=
break;
}
....
}
if (info == NULL)
{
....
}
....
}After examining this code, one draws a logical conclusion that the programmer actually wanted to assign a value to the 'info' variable: only then would the code after 'while' make sense. Let's try to fix it:
info = tmp_info;We're done with our discussion of errors related to code copying here. One conclusion to draw from all said above is that it's a very common pattern of errors, which may stay hidden for a long time. These errors are generally difficult to find as they don't catch your eye when glancing through the code.
The next category of possible errors deals with incorrect use of pointers. Careless pointer handling may result in crashes or undefined behavior.
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *data->groups[0] != '\0'. gtkrecentmanager.c 979
struct _GtkRecentData
{
....
gchar **groups;
....
};
gboolean
gtk_recent_manager_add_full (GtkRecentManager *manager,
const gchar *uri,
const GtkRecentData *data)
{
....
if (data->groups && data->groups[0] != '\0')
....
....
}What is found at address 'data->groups[0]' is 'gchar*', i.e. also a pointer, which can't be compared with '\0'. In this example, the 'data->groups[0]' pointer is in fact compared with a null pointer. If the programmer really needed to make sure the pointer is non-null, then the correct way to do it is the following:
if (data->groups && data->groups[0] != NULL)And if they wanted to test the character found at address 'data->groups[0]' for being a null terminator, then the pointer should have been dereferenced:
if (data->groups && *data->groups[0] != '\0')Here is another similar example, which also deals with an incorrect comparison:
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *priv->icon_list[0] == '\0'. gtkscalebutton.c 987
struct _GtkScaleButtonPrivate
{
....
gchar **icon_list;
....
};
struct _GtkScaleButton
{
....
GtkScaleButtonPrivate *priv;
};
static void
gtk_scale_button_update_icon (GtkScaleButton *button)
{
GtkScaleButtonPrivate *priv = button->priv;
....
if (!priv->icon_list || priv->icon_list[0] == '\0')
....
}One must be careful when using pointers in C/C++. If you are not sure that the pointer really points to any data, you must test it for null.
Accessing a memory block by a null pointer will lead to undefined behavior or a crash. The following diagnostic messages warn you when such dangerous access may occur.
V595 The 'completion' pointer was utilized before it was verified against nullptr. Check lines: 2231, 2239. gtkentrycompletion.c 2231
static gboolean
gtk_entry_completion_key_press (...., gpointer user_data)
{
....
GtkEntryCompletion *completion =
GTK_ENTRY_COMPLETION (user_data);
if (!completion->priv->popup_completion)
return FALSE;
....
if (completion && completion->priv->completion_timeout) // <=
{
....
}
....
}In the function body, the programmer tests the 'completion' pointer for null and then uses it:
if (completion && completion->priv->completion_timeout)This check indicates the programmer's assumption that the pointer may be null. However, earlier in the code, this pointer was accessed without such check:
if (!completion->priv->popup_completion)
return FALSE;If the pointer here appears to be null, we'll get undefined behavior. Either there is a missing check of the 'completion' pointer earlier in the code or the later check makes no sense and is not necessary.
There are over a dozen cases like that in the toolkit's code, so we'll discuss only one more example:
V595 The 'dispatch->backend' pointer was utilized before it was verified against nullptr. Check lines: 1570, 1580. gtkprintbackendcups.c 1570
static void
cups_dispatch_watch_finalize (GSource *source)
{
....
if (dispatch->backend->username != NULL)
username = dispatch->backend->username;
else
username = cupsUser ();
....
if (dispatch->backend)
dispatch->backend->authentication_lock = FALSE;
....
}The 'dispatch->backend' pointer is checked only after it has been accessed, so this code is potentially unsafe.
Below is a list of other similar issues. It doesn't include warnings on pointer checks inside macros. Although these macros may have troubles with using null pointers as well, it's also possible that the programmer simply took macros that suited them, together with the checks they didn't need.
V595 The 'impl->toplevel' pointer was utilized before it was verified against nullptr. Check lines: 514, 524. gdkwindow-x11.c 514
V595 The 'pointer_info' pointer was utilized before it was verified against nullptr. Check lines: 9610, 9638. gdkwindow.c 9610
V595 The 'elt' pointer was utilized before it was verified against nullptr. Check lines: 2218, 2225. gtktreemodelfilter.c 2218
V595 The 'tmp_list' pointer was utilized before it was verified against nullptr. Check lines: 5817, 5831. gtktreeview.c 5817
V595 The 'dispatch->data_poll' pointer was utilized before it was verified against nullptr. Check lines: 1470, 1474. gtkprintbackendcups.c 1470
Finally, we will discuss a group of diverse warnings concerning possible algorithmic errors or typos.
In the following example, the author forgot to write 'break' statements at the end of 'case' statements:
V519 The 'type' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 187, 189. testselection.c 189
void
selection_get (....
guint info,
....)
{
....
switch (info)
{
case COMPOUND_TEXT:
case TEXT:
type = seltypes[COMPOUND_TEXT];
case STRING:
type = seltypes[STRING];
}
....
}No matter which of the three values will be assigned to 'info', we will end up with the 'type = seltypes[STRING];' assignment. To avoid it, we need to add the 'break' statement:
switch (info)
{
case COMPOUND_TEXT:
case TEXT:
type = seltypes[COMPOUND_TEXT];
break;
case STRING:
type = seltypes[STRING];
break;
}The next fragment is very suspicious: one variable ('i') is used as a counter for both the outer and the inner loops:
V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 895, 936. gtkstyleproperties.c 936
void
gtk_style_properties_merge (....)
{
....
guint i;
....
for (i = 0; i < prop_to_merge->values->len; i++)
{
....
else if (_gtk_is_css_typed_value_of_type (data->value,
G_TYPE_PTR_ARRAY) && value->value != NULL)
{
....
for (i = 0; i < array_to_merge->len; i++)
g_ptr_array_add (array,
g_ptr_array_index (array_to_merge, i));
}
....
}
....
}I'm not sure what value the 'i' variable will be referring to after the inner loop has executed and how the outer loop will run afterward. To avoid this uncertainty, the inner loop should use a counter of its own, for example:
guint j;
for (j = 0; j < array_to_merge->len; j++)
g_ptr_array_add (array,
g_ptr_array_index (array_to_merge, j));Two more cases of potentially unsafe use of loop counters:
V557 Array overrun is possible. The value of 'i + 1' index could reach 21. gtkcssselector.c 1219
V557 Array overrun is possible. The value of 'i + 1' index could reach 21. gtkcssselector.c 1224
#define G_N_ELEMENTS(arr) (sizeof (arr) / sizeof ((arr)[0]))
static GtkCssSelector *
parse_selector_pseudo_class (....)
{
static const struct {
....
} pseudo_classes[] = {
{ "first-child", 0, 0, POSITION_FORWARD, 0, 1 },
....
{ "drop(active)", 0, GTK_STATE_FLAG_DROP_ACTIVE, }
};
guint i;
....
for (i = 0; i < G_N_ELEMENTS (pseudo_classes); i++)
{
....
{
if (pseudo_classes[i + 1].state_flag ==
pseudo_classes[i].state_flag)
_gtk_css_parser_error_full (parser,
GTK_CSS_PROVIDER_ERROR_DEPRECATED,
"The :%s pseudo-class is deprecated. Use :%s instead.",
pseudo_classes[i].name,
pseudo_classes[i + 1].name);
....
}
....
}
....
}The loop is based on the number of items in the 'pseudo_classes' array, and I hope it will never reach the last item. Otherwise, the 'pseudo_classes[i+1]' construct will result in indexing outside the array's bounds.
The next potential error looks like a typo:
V559 Suspicious assignment inside the condition expression of 'if' operator. gdkselection-x11.c 741
gboolean
gdk_x11_display_utf8_to_compound_text (....)
{
....
GError *error = NULL;
....
if (!(error->domain = G_CONVERT_ERROR &&
error->code == G_CONVERT_ERROR_ILLEGAL_SEQUENCE))
....
}Programmers often mistakenly use the assignment operator '=' instead of comparison operator '=='. The code will compile, and some compilers may generate a warning on this issue. But you don't turn off undesired warnings and don't make mistakes like that, do you? The code should probably look like this:
if (!(error->domain == G_CONVERT_ERROR &&
error->code == G_CONVERT_ERROR_ILLEGAL_SEQUENCE))In the next example, the analyzer warns about a conditional if-statement containing an expression that always evaluates to one and the same value:
V560 A part of conditional expression is always false: !auto_mnemonics. gtklabel.c 2693
static void
gtk_label_set_markup_internal (....)
{
....
gboolean enable_mnemonics = TRUE;
gboolean auto_mnemonics = TRUE;
g_object_get (gtk_widget_get_settings (GTK_WIDGET (label)),
"gtk-enable-mnemonics", &enable_mnemonics,
NULL);
if (!(enable_mnemonics && priv->mnemonics_visible &&
(!auto_mnemonics ||
(gtk_widget_is_sensitive (GTK_WIDGET (label)) &&
(!priv->mnemonic_widget ||
gtk_widget_is_sensitive (priv->mnemonic_widget))))))
....
}It doesn't look like an error at first sight. But if you look closely, you'll notice that variable 'enable_mnemonics' is created near the 'auto_mnemonics' variable and is then initialized to a value from the settings. Perhaps the value for 'auto_mnemonics' must have been retrieved in a similar way, too. And if not, then the check of the '!auto_mnemonics' condition should be deleted, I guess.
One more warning concerning the 'auto_mnemonics' variable:
V560 A part of conditional expression is always false: !auto_mnemonics. gtklabel.c 2923
static void
gtk_label_set_pattern_internal (....)
{
....
gboolean enable_mnemonics = TRUE;
gboolean auto_mnemonics = TRUE;
....
g_object_get (gtk_widget_get_settings (GTK_WIDGET (label)),
"gtk-enable-mnemonics", &enable_mnemonics,
NULL);
if (enable_mnemonics && priv->mnemonics_visible && pattern &&
(!auto_mnemonics ||
(gtk_widget_is_sensitive (GTK_WIDGET (label)) &&
(!priv->mnemonic_widget ||
gtk_widget_is_sensitive (priv->mnemonic_widget)))))
....
}And here's a warning about a dangerous comparison of an unsigned variable of type 'guint' with a signed constant:
V605 Consider verifying the expression. An unsigned value is compared to the number -3. gtktextview.c 9162
V605 Consider verifying the expression. An unsigned value is compared to the number -1. gtktextview.c 9163
struct GtkTargetPair {
GdkAtom target;
guint flags;
guint info;
};
typedef enum
{
GTK_TEXT_BUFFER_TARGET_INFO_BUFFER_CONTENTS = - 1,
GTK_TEXT_BUFFER_TARGET_INFO_RICH_TEXT = - 2,
GTK_TEXT_BUFFER_TARGET_INFO_TEXT = - 3
} GtkTextBufferTargetInfo;
static void
gtk_text_view_target_list_notify (....)
{
....
if (pair->info >= GTK_TEXT_BUFFER_TARGET_INFO_TEXT &&
pair->info <= GTK_TEXT_BUFFER_TARGET_INFO_BUFFER_CONTENTS)
....
}Compilers also output warnings on comparisons like that, but they are often undeservedly turned off by programmers. In this example, a signed type will be implicitly cast to unsigned. It's not that bad if the programmer provided for that possibility when writing the condition, but even then this code is far from good. You should at least use an explicit conversion to show you understand what's happening. If 'pair->info' can be assigned values only from the 'GtkTextBufferTargetInfo' enumeration, then why not make the info variable of the same type? And if it may be assigned other values as well, this approach is unsafe altogether.
The last warning we'll discuss deals with overlapping ranges in 'if...elseif' conditions:
V695 Range intersections are possible within conditional expressions. Example: if (A < 5) { .... } else if (A < 2) { .... }. Check lines: 580, 587. broadway-server.c 587
static void
parse_input (BroadwayInput *input)
{
....
while (input->buffer->len > 2)
{
....
if (payload_len > 125)
{
....
}
else if (payload_len > 126)
{
....
}
....
}
}As seen from the code, any value of 'payload_len' that is larger than 125 will result in executing the 'if (payload_len > 125)' branch while the 'else if (payload_len > 126)' branch is a special case of that original check. Therefore, the code in the 'elseif' condition will never execute. The developers need to examine and fix it.
Analysis of the GTK+ toolkit's code shows that it contains both ordinary typos and more interesting errors, which need to be fixed. Static analyzers are very good at eliminating such errors at earlier development stages; they help save developers' time, which can be spent on developing new functionality instead of debugging and manual bug hunting. Remember, you can try the PVS-Studio static analyzer for free by downloading it here.
0
0
0
0