Our website uses cookies to enhance your browsing experience.
You may have already read a recent article about the first PVS-Studio run and filtration of warnings. We used the GTK 4 project as an example. It's about time we worked with the received report in more detail. Our regular readers may have already guessed that this article will be a description of errors found in the code.
Rarely do I tend to stuff many errors in an article. It was the case with a recent post "Espressif IoT Development Framework: 71 Shots in the Foot". This time I'll limit myself to 21 mistakes in honor of 2021:). Besides, I should note high quality of the GTK 4 project, as well as small density of errors.
The project is popular, well tested. As far as I know, it is already being tested by tools such as Clang static analysis tool, Coverity, AddressSanitizer, UndefinedBehavior Sanitizer. Basically, everything is fine with the code quality. Therefore, even a dozen of found errors is a great piece of work.
Check out the article "GTK: The First Analyzer Run in Figures" by Svyatoslav Razmyslov covering the process of checking GTK 4 and filtering report messages. After the first filtration, the report showed 581 warnings of the first and second levels of certainty. I have not considered the ones of the third level. So, we've got 581 warnings and only twenty-one errors added to the article: is that enough? That's ok.
Like I said, in my opinion, the project code is of high quality. Most warnings come from failed macros and the use of conditional compilation directives. In some cases, I can't even say for sure if it's a false positive that the analyzer issued. PVS-Studio seems to issue reasonable warnings, but they still are not doing any good. Look at the example - you'll get what I'm saying.
Imagine you see the following code:
bool var;
var = true;
if (!var && foo)I guess you'll agree that the code looks suspicious. Why write like that? Maybe the author forgot to change the value of the var variable somewhere? The code smells wrong. No wonder, the analyzer didn't like it either. The PVS-Studio static analyzer issues a warning "A part of conditional expression is always false:! var". A reasonable warning? Yes.
As always, there are nuances in practice. Here is an identical code fragment from GTK 4, but there is nothing suspicious or dangerous in it:
gboolean debug_enabled;
#ifdef G_ENABLE_DEBUG
debug_enabled = TRUE;
#else
debug_enabled = FALSE;
#endif
....
if (!debug_enabled && !keys[i].always_enabled)The analyzer still issues a warning: V560 [CWE-570] A part of conditional expression is always false:! debug_enabled. gdk.c 281
But as you can see, this warning is pointless. That's the way it is, the real world. Useless warnings often appear this way.
Fortunately, there's nothing terrible about it. As the previous article goes, there are many ways to get rid of these needless positives: change the code, suppress individual warnings, mark macros, use the base for warnings mass suppression, and so on.
As I see, quite a lot of errors found are due to inattention. In other words, typos are the reason for these errors. Let's start right with them.
Fragment N1 Pretty typo in a loop
void
gsk_vulkan_image_upload_regions (GskVulkanImage *self,
GskVulkanUploader *uploader,
guint num_regions,
GskImageRegion *regions)
{
....
for (int i = 0; i < num_regions; i++)
{
m = mem + offset;
if (regions[i].stride == regions[i].width * 4)
{
memcpy (m, regions[i].data, regions[i].stride * regions[i].height);
}
else
{
for (gsize r = 0; r < regions[i].height; i++) // <=
memcpy (m + r * regions[i].width * 4,
regions[i].data + r * regions[i].stride, regions[i].width * 4);
}
....
}
....
}PVS-Studio warning: V533 [CWE-691] It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. gskvulkanimage.c 721
Note that in a nested loop, it is not the variable r that is incremented, but i. No need to comment. It's a golden classic!
Fragment N2 Loop that does not execute
In the previous case, the function could start running a loop with an uncontrolled number of iterations. It would end when the memcpy function wrote something in a very wrong place. As a result we would get a Segmentation fault.
As for this case, on the contrary, the second loop will not execute at all:
GtkCssValue *
_gtk_css_border_value_parse (GtkCssParser *parser,
GtkCssNumberParseFlags flags,
gboolean allow_auto,
gboolean allow_fill)
{
....
guint i;
....
for (; i < 4; i++) // <=
{
if (result->values[(i - 1) >> 1])
result->values[i] = _gtk_css_value_ref (result->values[(i - 1) >> 1]);
}
result->is_computed = TRUE;
for (; i < 4; i++) // <=
if (result->values[i] && !gtk_css_value_is_computed (result->values[i]))
{
result->is_computed = FALSE;
break;
}
....
}PVS-Studio warning: V621 [CWE-835] Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. gtkcssbordervalue.c 221
After completion of the first loop, the value of the icounter is 4. Hence, the second loop will not go for a single iteration. It lacks the counter reset to 0.
Fragment N3 Reusing the same constant
static void
gtk_list_base_class_init (GtkListBaseClass *klass)
{
....
properties[PROP_ORIENTATION] =
g_param_spec_enum ("orientation",
P_("Orientation"),
P_("The orientation of the orientable"),
GTK_TYPE_ORIENTATION,
GTK_ORIENTATION_VERTICAL,
G_PARAM_READWRITE | G_PARAM_EXPLICIT_NOTIFY |
G_PARAM_EXPLICIT_NOTIFY);
....
}PVS-Studio warning: V501 There are identical sub-expressions 'G_PARAM_EXPLICIT_NOTIFY' to the left and to the right of the '|' operator. gtklistbase.c 1151
The G_PARAM_EXPLICIT_NOTIFY constant is used twice to generate the mask. The programmer clearly intended to use different constants, and as a result, not all the necessary bits are set to the mask.
Fragment N4 Confusion in the order of arguments
First, let's look at the post_insert_fixup function declaration. Note the order of the formal arguments char_count_delta and line_count_delta.
static void post_insert_fixup (GtkTextBTree *tree,
GtkTextLine *insert_line,
int char_count_delta,
int line_count_delta);And now, let's look closer at the code snippet where this function is called:
void
_gtk_text_btree_insert (GtkTextIter *iter,
const char *text,
int len)
{
....
int line_count_delta; /* Counts change to total number of
* lines in file.
*/
int char_count_delta; /* change to number of chars */
....
post_insert_fixup (tree, line, line_count_delta, char_count_delta);
....
}PVS-Studio warning: V764 [CWE-683] Possible incorrect order of arguments passed to 'post_insert_fixup' function: 'line_count_delta' and 'char_count_delta'. gtktextbtree.c 1230
The third and fourth function argument are mixed up in places due to a typo. Since the types of arguments coincide, the code is compiled successfully without warnings, although it still does not make sense.
PVS-Studio has quite a lot of empirical diagnostics that reveal correct code in terms of compilation, but absurd in its essence. The analyzer allows you to identify these errors very early. A team will be able to focus on higher-level issues rather than search for typos during code review. Therefore, while you are reading this article, I suggest downloading the distribution and requesting a demo key.
Fragment N5 Another impressive case of arguments confusion
Pay attention to the arguments of the called function:
static guint
translate_keysym (GdkX11Keymap *keymap_x11,
guint hardware_keycode,
int group,
GdkModifierType state,
int *effective_group,
int *effective_level)
{
....
}Failed function call given above:
static gboolean
gdk_x11_keymap_translate_keyboard_state (GdkKeymap *keymap,
guint hardware_keycode,
GdkModifierType state,
int group,
guint *keyval,
int *effective_group,
int *level,
GdkModifierType *consumed_modifiers)
{
....
tmp_keyval = translate_keysym (keymap_x11, hardware_keycode,
group, state,
level, effective_group); // <=
....
}PVS-Studio warning: V764 [CWE-683] Possible incorrect order of arguments passed to 'translate_keysym' function: 'level' and 'effective_group'. gdkkeys-x11.c 1386
This time something is messed up in working with the keyboard. It's a typo again: the actual level and effective_group arguments got mixed up.
If someone has not yet decided to download and try PVS-Studio, then now is the perfect time :). Do you really like to spot such mistakes only by looking through the code? And God save us all from fighting them in the debugger!
Fragment N6 Forgotten asterisk (*)
gboolean
gtk_check_compact_table (...., int n_compose, ....)
{
....
guint16 *seq_index;
....
seq_index = bsearch (compose_buffer,
table->data,
table->n_index_size,
sizeof (guint16) * table->n_index_stride,
compare_seq_index);
if (!seq_index)
return FALSE;
if (seq_index && n_compose == 1)
return TRUE;
....
}PVS-Studio warning: V560 [CWE-571] A part of conditional expression is always true: seq_index. gtkimcontextsimple.c 475
The second seq_index pointer check doesn't make sense. The pointer is already checked above and is definitely not null when it comes to the second check. I don't know what exactly this code is supposed to do. I would dare assume that in the second case the intention was to check not the pointer itself, but the value that it accesses. In other words, it seems to me that the developer forgot to dereference the pointer. Then the correct code should be as follows:
if (!seq_index)
return FALSE;
if (*seq_index && n_compose == 1)
return TRUE;Fragment N7-N9. Repeated assignments
static void
gtk_message_dialog_init (GtkMessageDialog *dialog)
{
GtkMessageDialogPrivate *priv = ....;
....
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;
....
}PVS-Studio warnings:
Here this block of code repeats two times:
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;Perhaps the second unit is just redundant. Maybe something else had to be initialized. Either way it's some sort of a typo.
There is a couple of similar pointless reassignments:
Fragment N10 Using a pointer before checking
static gboolean
on_flash_timeout (GtkInspectorWindow *iw)
{
iw->flash_count++;
gtk_highlight_overlay_set_color (GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay),
&(GdkRGBA) {
0.0, 0.0, 1.0,
(iw && iw->flash_count % 2 == 0) ? 0.0 : 0.2
});
....
}PVS-Studio warning: V595 [CWE-476] The 'iw' pointer was utilized before it was verified against nullptr. Check lines: 194, 199. inspect-button.c 194
At the beginning, the iw pointer is boldly dereferenced to increment one of the class members. Only when we read the code below, we find out that this pointer may actually be null. This follows from the presence of the check:
(iw && iw->flash_count % 2 == 0)To fix the situation, one needs to add another check:
if (iw)
iw->flash_count++;
gtk_highlight_overlay_set_color (GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay),
&(GdkRGBA) {
0.0, 0.0, 1.0,
(iw && iw->flash_count % 2 == 0) ? 0.0 : 0.2
});However, this fix will still not be enough:). If we look closely, we can see another dereferencing when evaluating the arguments:
GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay)Since I am not familiar with the logic of the program, I will not build assumptions on how to further edit the code. Let's pass the baton to the project authors.
Fragment N11 Multiple use of a pointer before the check
static void
cups_dispatch_watch_finalize (GSource *source)
{
GtkPrintCupsDispatchWatch *dispatch;
....
const char *username;
char hostname[HTTP_MAX_URI];
char *key;
httpGetHostname (dispatch->request->http, hostname, sizeof (hostname));
if (is_address_local (hostname))
strcpy (hostname, "localhost");
if (dispatch->backend->username != NULL) // <=
username = dispatch->backend->username; // <=
else
username = cupsUser ();
key = g_strconcat (username, "@", hostname, NULL);
GTK_NOTE (PRINTING,
g_print ("CUPS backend: removing stored password for %s\n", key));
g_hash_table_remove (dispatch->backend->auth, key); // <=
g_free (key);
if (dispatch->backend) // <=
dispatch->backend->authentication_lock = FALSE;
....
}PVS-Studio warning: V595 [CWE-476] The 'dispatch->backend' pointer was utilized before it was verified against nullptr. Check lines: 1603, 1613. gtkprintbackendcups.c 1603
This is even more "fearless" code:). The authors dereference the dispatch->backend pointer (see code fragments highlighted by comments). Only after they have recalled about potentially null pointer did they write the check.
if (dispatch->backend)But it's too late :).
Fragment N12 No safety measures if both pointers are null
static GskRenderNode *
gtk_snapshot_collect_blend_top (GtkSnapshot *snapshot,
GtkSnapshotState *state,
GskRenderNode **nodes,
guint n_nodes)
{
GskRenderNode *bottom_node, *top_node, *blend_node;
GdkRGBA transparent = { 0, 0, 0, 0 };
top_node = gtk_snapshot_collect_default (snapshot, state, nodes, n_nodes);
bottom_node = state->data.blend.bottom_node != NULL
? gsk_render_node_ref (state->data.blend.bottom_node)
: NULL;
g_assert (top_node != NULL || bottom_node != NULL);
if (top_node == NULL)
top_node = gsk_color_node_new (&transparent, &bottom_node->bounds);
if (bottom_node == NULL)
bottom_node = gsk_color_node_new (&transparent, &top_node->bounds);
....
}V595 [CWE-476] The 'bottom_node' pointer was utilized before it was verified against nullptr. Check lines: 1189, 1190. gtksnapshot.c 1189
Both top_node and bottom_node pointers are not supposed to be null. We learn this from the line:
g_assert (top_node != NULL || bottom_node != NULL);But this does not protect the release version of the program, in which the macro g_assert will expand into a void. It's much better to explicitly consider such a case. For example, one can write as follows:
if (top_node == NULL && bottom_node == NULL)
{
g_assert (false);
return NULL;
}Fragment N13 Redundant check
static void
stash_desktop_startup_notification_id (void)
{
const char *desktop_startup_id;
desktop_startup_id = g_getenv ("DESKTOP_STARTUP_ID");
if (desktop_startup_id && *desktop_startup_id != '\0')
{
if (!g_utf8_validate (desktop_startup_id, -1, NULL))
g_warning ("DESKTOP_STARTUP_ID contains invalid UTF-8");
else
startup_notification_id =
g_strdup (desktop_startup_id ? desktop_startup_id : "");
}
g_unsetenv ("DESKTOP_STARTUP_ID");
}PVS-Studio warning: V547 [CWE-571] Expression 'desktop_startup_id' is always true. gdk.c 176
There may be no error here, but the code is superfluous. We should avoid writing excessive code. Firstly, it complicates the study and support. Secondly, when making edits an author is more likely to introduce an error as well.
Let's simplify this code by removing the second pointer check:
desktop_startup_id = g_getenv ("DESKTOP_STARTUP_ID");
if (desktop_startup_id && *desktop_startup_id != '\0')
{
if (!g_utf8_validate (desktop_startup_id, -1, NULL))
g_warning ("DESKTOP_STARTUP_ID contains invalid UTF-8");
else
startup_notification_id = g_strdup (desktop_startup_id);
}Fragment N14 Variable does not change its value
There are always many needless checks in projects, and these are rarely real errors. More often it's inaccuracy and redundant code. When writing articles, I look at always false/true conditions very superficially to save time and carefully study more interesting warnings. However, this does not mean that the authors of the project should not study such warnings. It's best to eliminate errors or remove redundant conditions at an early stage, thereby making code simpler and more elegant.
Another case where we can safely omit the check:
#define MAX_LIST_SIZE 1000
static void
gtk_recent_manager_real_changed (GtkRecentManager *manager)
{
....
int age;
int max_size = MAX_LIST_SIZE;
....
...//The max_size variable does not change here.
....
if (age == 0 || max_size == 0 || !enabled)
{
g_bookmark_file_free (priv->recent_items);
priv->recent_items = g_bookmark_file_new ();
priv->size = 0;
}
else
{
if (age > 0)
gtk_recent_manager_clamp_to_age (manager, age);
if (max_size > 0)
gtk_recent_manager_clamp_to_size (manager, max_size);
}
....
}PVS-Studio warning: V547 [CWE-571] Expression 'max_size > 0' is always true. gtkrecentmanager.c 480
The fact is that the value of the max_size variable after its declaration and initialization is no longer changed. It's rather weird. This code can be both redundant, or contain an error in its logic if someone forgot to change the variable's value somewhere.
Note. An attentive reader may ask a question: why is there no warning for the part of the subexpression "max_size == 0"? Well, it is. I just missed it during a cursory report review. I did not pay attention to this point when writing the article either. Here's this warning: V560 [CWE-570] A part of conditional expression is always false: max_size == 0. gtkrecentmanager.c 470.
Fragment N15, N16. Using an index before it is checked
static void
action_handle_method (GtkAtSpiContext *self,
const char *method_name,
GVariant *parameters,
GDBusMethodInvocation *invocation,
const Action *actions,
int n_actions)
{
....
int idx = -1;
g_variant_get (parameters, "(i)", &idx);
const Action *action = &actions[idx];
if (idx >= 0 && idx < n_actions)
g_dbus_method_invocation_return_value (
invocation, g_variant_new ("(s)", action->name));
else
g_dbus_method_invocation_return_error (invocation,
G_IO_ERROR,
G_IO_ERROR_INVALID_ARGUMENT,
"Unknown action %d",
idx);
....
}PVS-Studio warning: V781 [CWE-129] The value of the 'idx' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 71, 73. gtkatspiaction.c 71
First the idx variable is used to access array elements:
const Action *action = &actions[idx];And only then it is checked whether it is not negative or whether the value is too large in this variable:
if (idx >= 0 && idx < n_actions)Here's the result: it's too late for the check, the array index is already out of bounds, leading to undefined behavior.
Similar case: V781 [CWE-129] The value of the 'idx' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 132, 134. gtkatspiaction.c 132
Fragment N17, N18. Unreachable code
static gboolean
parse_n_plus_b (GtkCssParser *parser,
int before,
int *a,
int *b)
{
const GtkCssToken *token;
token = gtk_css_parser_get_token (parser);
if (gtk_css_token_is_ident (token, "n"))
{
....
return parse_plus_b (parser, FALSE, b);
}
else if (gtk_css_token_is_ident (token, "n-"))
{
....
return parse_plus_b (parser, TRUE, b);
}
else if (gtk_css_token_is (token, GTK_CSS_TOKEN_IDENT) &&
string_has_number (token->string.string, "n-", b))
{
....
return TRUE;
}
else
{
*b = before;
*a = 0;
return TRUE;
}
gtk_css_parser_error_syntax (parser, "Not a valid an+b type");
return FALSE;
}PVS-Studio warning: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. gtkcssselector.c 1077
The function contains an if-else-if-else... sequence. At the same time, the body of each conditional operator ends with an exit from the function. This is strange, since there is a piece of code at the end of the function that will never get control.
Another similar case with unreachable code: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. gtktreemodelfilter.c 3289
Fragment N19, N20. Integer division
static void
gtk_paint_spinner (GtkStyleContext *context,
cairo_t *cr,
guint step,
int x,
int y,
int width,
int height)
{
GdkRGBA color;
guint num_steps;
double dx, dy;
....
dx = width / 2;
dy = height / 2;
....
cairo_move_to (cr,
dx + (radius - inset) * cos (i * G_PI / half),
dy + (radius - inset) * sin (i * G_PI / half));
cairo_line_to (cr,
dx + radius * cos (i * G_PI / half),
dy + radius * sin (i * G_PI / half));
....
}PVS-Studio warnings:
The results of integer divisions are written to the dx and dy variables. These variables are of the double type which is questionable. Most likely it's just a slip-up. Here is a possible correct version:
dx = width / 2.0;
dy = height / 2.0;Similar suspicious divisions are found in the code snippet pointed to by these two warnings:
Fragment N21 Password may not be cleared in memory
As a special treat, I saved a highly interesting case. It is a common mistake when the memset is used to clear data in memory. Compilers like to remove such function calls for optimization purposes. In terms of C and C++, if the memory area is no longer used after filling, then there is no need to fill it at all. In other words, a compiler removes an entry of a value to a variable if no reading occurs from that variable.
I have considered such errors in articles many times, and I wouldn't like to fly off at a tangent. Therefore, if you are not yet familiar with this pattern, then I suggest you checking out the contents of these posts by links:
What's interesting in the case of GTK 4? The fact is that calling the free function occurs through an intermediate function and here it becomes more difficult to predict whether the compiler will start optimization or not.
In GTK 4 we see the g_free function that frees memory: It is implemented as follows:
void
g_free (gpointer mem)
{
free (mem);
TRACE(GLIB_MEM_FREE((void*) mem));
}Is g_free always just a wrapper over free? Since GLib 2.46 this is always the case. The documentation covers this question as follows:
It's important to match g_malloc() (and wrappers such as g_new()) with g_free(), g_slice_alloc() (and wrappers such as g_slice_new()) with g_slice_free(), plain malloc() with free(), and (if you're using C++) new with delete and new[] with delete[]. Otherwise bad things can happen, since these allocators may use different memory pools (and new/delete call constructors and destructors).
Since GLib 2.46 g_malloc() is hardcoded to always use the system malloc implementation.
So, since the memory in g_malloc is allocated using malloc, then the free function must always be called to release it.
Now let's look at the problem code.
void overwrite_and_free (gpointer data)
{
char *password = (char *) data;
if (password != NULL)
{
memset (password, 0, strlen (password));
g_free (password);
}
}PVS-Studio warning: V597 [CWE-14] The compiler could delete the 'memset' function call, which is used to flush 'password' object. The memset_s() function should be used to erase the private data. gtkprintbackendcups.c 848
After filling the memory with zeroes, the pointer to this area of memory is passed to the g_free function. The error may either reveal itself or not. It depends on the compiler and the optimization settings used. If the compiler performs interprocedural optimization and inserts the g_free function body into the overwrite_and_free function, it may conclude that the memset function is redundant and will remove it.
A very juicy unpleasant error from the field of information security.
The PVS-Studio static analyzer supports many scenarios how the tool can be used. Firstly, we mean the possibility of its integration with IntelliJ IDEA, Rider, Incredibuild, Jenkins, PlatformIO, Travis CI, GitLab CI/CD, CircleCI, TeamCity, Visual Studio and so on. Secondly, there are various options for its introduction, customization, use of notifications. In fact, we are not even happy to have so many different things in it. It's just impossible to make short concise documentation, as it was, for example, 10 years ago when PVS-Studio was just a Visual Studio plugin. For sure, no one reads the existing large documentation from cover to cover:). As a result, developers step on the same rake, make the same mistakes in implementation, and ask similar questions in support.
Unfortunately, it is impossible to remedy the situation with the help of a more understandable interface. Actually, in some cases there is not any interface :). There are only settings for configuration, integration, for example, with SonarQube.
Therefore, after consideration we came up with the idea of posting short video tutorials on certain aspects of using PVS-Studio. This way you will be able to easily find answers quicker and crack problems. More importantly, you can subscribe to the channel and find out about new features step by step when watching our new video tips after they are posted. It is better to get acquainted with PVS-Studio piece by piece, rather than scrape through all the documentation in one go :). It's not overwhelmingly likely that it will work, but let's give it a go! Consider subscribing: PVS-Studio capacities (YouTube).
0
0
0
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:
0