33 fragments in the GTK library

GTK is a popular open-source framework for creating graphical interfaces, which is interesting to check using the PVS-Studio analyzer. Moreover, we did the previous check about 3 years ago, which means we will probably find new errors in it.

I really didn’t want to partially repeat the introduction from the previous article.”Identifying typos in a GTK 4 project using PVS-Studio“, but I suspect that not all readers are familiar with GTK. Therefore, in short: the library allows you to implement a graphical user interface cross-platform. It is completely free and open source sourcelicensed under the GNU GPL, which allows it to be used in any projects (even commercial ones).

Fragment N1. Escape sequence

gboolean
gtk_builder_value_from_string_type (....)
{
  g_set_error (error,
               GTK_BUILDER_ERROR,
               GTK_BUILDER_ERROR_INVALID_VALUE,
               "Object named \"%s\" is of type \"%s\" which is not compatible "
               "with expected type \%s\"",
               string, G_OBJECT_TYPE_NAME (object), g_type_name (type));
}

PVS-Studio warning: V1094 The representation of conditional escape sequence ‘\%’ is implementation-defined. Using this sequence in the string literal can lead to unexpected results. gtkbuilder.c 2674

There was a typo in the string literal. Wanted to escape double quotes:

\"%s\"

But we accidentally missed the quote character, and a non-existent escape sequence appeared, consisting of a slash and a percent symbol:

\%s\"

How this sequence will be processed depends on the compiler.

Fragment N2. Typo in condition

Value* op_color_number(enum Sass_OP op, ....)
{
  double rval = rhs.value();

  if ((op == Sass_OP::DIV || op == Sass_OP::DIV) && rval == 0) {
    // comparison of Fixnum with Float failed?
    throw Exception::ZeroDivisionError(lhs, rhs);
  }
  ....
}

PVS-Studio warning: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the ‘||’ operator: op == Sass_OP::DIV || op == Sass_OP::DIV operators.cpp 250

The variable is compared to the named constant twice Sass_OP::DIV. There is a strong suspicion that the second constant should be Sass_OP::MOD.

It would be easier to notice the error if all blocks of the same type were formatted in a column in the conditions. Like this:

if ((   op == Sass_OP::DIV
     || op == Sass_OP::DIV)
    && rval == 0) {
  // comparison of Fixnum with Float failed?
  throw Exception::ZeroDivisionError(lhs, rhs);
}

When I wrote it, I realized that the code looked untidy and became even worse than before. Although I am a big proponent of tabular formatting of code, in this case it somehow turns out to be inappropriate. If you don’t understand the term “tabular formatting”, then I suggest you familiarize yourself with it in the mini-book “60 antipatterns for a C++ programmer“. See the twentieth bad tip, “Compact code.”

Fragment N3. Pointless challenge g_free

gtk_css_parser_resolve_url (GtkCssParser *self,
                            const char   *url)
{
  char *scheme;

  scheme = g_uri_parse_scheme (url);
  if (scheme != NULL)
    {
      GFile *file = g_file_new_for_uri (url);
      g_free (scheme);
      return file;
    }
  g_free (scheme);     // <=

  if (self->directory == NULL)
    return NULL;

  return g_file_resolve_relative_path (self->directory, url);
}

PVS-Studio warning: V575 [CWE-628, CERT-EXP37-C] The null pointer is passed into ‘g_free’ function. Inspect the first argument. gtkcssparser.c 189

Here’s the second function call g_free clearly superfluous, since it is always transmitted NULL. Nothing bad will happen because of this. IN g_free You can pass a null pointer. Just an extra line of code.

This is the case when a static analyzer helps to shorten or simplify the code. Now there will be a similar situation.

Fragment N4. Overcomplicated check

static gboolean
is_codepoint (const char *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;
}

PVS-Studio warning: V590 [CWE-571, CERT-MSC01-C] Consider inspecting the ‘str[0] == ‘\0’ || str[0] != ‘U” expression. The expression is excessive or contains a misprint. gtkcomposetable.c 119

Condition:

if (str[0] == '\0' || str[0] != 'U' || str[1] == '\0')
  return FALSE;

You can shorten:

if (str[0] != 'U' || str[1] == '\0')
  return FALSE;

From the point of view of the logic of the code, nothing has changed. So is it worth simplifying the code? I think so. The more complex the code, the more it attracts errors.

Fragment N5-N11. Potential uses of null pointer

static void
notify_observers_added (GtkActionMuxer *muxer,
                        GtkActionMuxer *parent)
{
  ....
  Action *action;
  ....
  while (....)
  {
    ....
    if (!action->watchers)
      continue;

    for (node = action ? action->watchers : NULL; node; node = node->next)
      gtk_action_observer_primary_accel_changed (node->data,
                                                 GTK_ACTION_OBSERVABLE (muxer),
                                                 action_name, NULL);
  ....
}

PVS-Studio warning: V595 [CWE-476, CERT-EXP12-C] The ‘action’ pointer was utilized before it was verified against nullptr. Check lines: 449, 452. gtkactionmuxer.c 449

Pointer action is used and then checked for NULL.

if (!action->watchers)
....
for (node = action ? ....)

The presence of a check indicates that the pointer may be null. The code should clearly be rewritten, for example like this:

if (action == NULL || action->watchers == NULL)
  continue;

for (node = action->watchers; node; node = node->next)
  gtk_action_observer_primary_accel_changed (node->data,
                                             GTK_ACTION_OBSERVABLE (muxer),
                                             action_name, NULL);

This is not the only such error in the code. At a minimum, it is present in the following places:

• V595 [CWE-476, CERT-EXP12-C] The ‘icon’ pointer was utilized before it was verified against nullptr. Check lines: 2225, 2231. gtkicontheme.c 2225
• V595 [CWE-476, CERT-EXP12-C] The ‘iw’ pointer was utilized before it was verified against nullptr. Check lines: 194, 199. inspect-button.c 194
• V595 [CWE-476, CERT-EXP12-C] The ‘contents’ pointer was utilized before it was verified against nullptr. Check lines: 493, 501. file.cpp 493
• V595 [CWE-476, CERT-EXP12-C] The ‘dispatch->data_poll’ pointer was utilized before it was verified against nullptr. Check lines: 1519, 1523. gtkprintbackendcups.c 1519
• V595 [CWE-476, CERT-EXP12-C] The ‘top’ pointer was utilized before it was verified against nullptr. Check lines: 1024, 1051. gdkscreen-x11.c 1024
• V595 [CWE-476, CERT-EXP12-C] The ‘dispatch->backend’ pointer was utilized before it was verified against nullptr. Check lines: 1615, 1624. gtkprintbackendcups.c 1615

Fragment N12. Incompatible methods for allocating and freeing memory

static char *
strip_ulines (const char *text,
              guint      *accel_key)
{
  ....
  new_text = malloc (strlen (text) + 1);     // <=
  ....
  return new_text;
}

static void
finish_text (UriParserData *pdata)
{
  ....
  char *text;
  ....
  if (pdata->strip_ulines && strchr (pdata->text_data->str, '_'))
  {
    text = strip_ulines (pdata->text_data->str, &pdata->accel_key);  // <=
    text_len = strlen (text);
  }
  else
  {
    text = pdata->text_data->str;
    text_len = pdata->text_data->len;
  }
  ....
  if (text != pdata->text_data->str)
    g_free (text);                                                   // <=
  ....
}

PVS-Studio warning: V611 [CWE-762, CERT-MEM51-CPP] The memory was allocated using ‘malloc’ function but was released using the ‘g_free’ function. Consider inspecting operation logics behind the ‘text’ variable. gtklabel.c 3383

Memory is allocated using the function mallocand is released using the function g_free. In chapter “Memory Allocation” Glib states that this cannot be done:

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).

Fragment N13. CWE-14

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, CERT-MSC06-C] 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 855

I described this piece of code in a previous article at the beginning of 2021, but no changes have been made to the code until now. The code is dangerous because the compiler could potentially remove the function call memset for optimization purposes, since this buffer is no longer used and is immediately freed.

Perhaps it will be necessary to study this point separately. It would be interesting to try and see if it would actually be possible to compile the library code so that the function call memset disappeared. I decided to experiment later.

Links for those who are not yet familiar with this error pattern:

Fragment N14. Invalid pointer

Offset Offset::init(const char* beg, const char* end)
{
  Offset offset(0, 0);
  if (end == 0) {
    end += strlen(beg);
  }
  offset.add(beg, end);
  return offset;
}

PVS-Studio warning: V769 [CWE-119, CERT-EXP08-C] The ‘end’ pointer in the ‘end += strlen(beg)’ expression equals nullptr. The resulting value is senseless and it should not be used. position.cpp 36

If the pointer end is zero, then the length of the string is added to it. The result is an invalid pointer that cannot be used. Most likely, what they really wanted to write was this:

end = beg + strlen(beg);

Fragment N15. Memory leak

static gboolean
query_tooltip (....)
{
  ....
  char *s, *s2;
  ....
  do {
    s = g_strdup_printf ("%.*f", precision, self->scale);
    l = strlen (s) - 1;
    while (s[l] == '0')
      l--;
    if (s[l] == '.')
      s[l] = '\0';
    precision++;
  } while (strcmp (s, "0") == 0);

  label = gtk_label_new (s);
  g_free (s);
  ....
}

PVS-Studio warning: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] The ‘s’ pointer was assigned values ​​twice without releasing the memory. A memory leak is possible. demo3widget.c 79

Function g_strdup_printf similar to standard C function sprintf, but is safer because it calculates the maximum space required and allocates the required amount of memory into which it writes the resulting string. The returned string should be freed when no longer needed.

There is no buffer release in this code. Therefore, at each iteration of the loop, a memory leak will occur. Call should be added g_free with each new iteration of the loop:

s = NULL;
do {
  g_free(s); // В g_free можно безопасно передавать NULL.
  s = g_strdup_printf ("%.*f", precision, self->scale);
  l = strlen (s) - 1;
  while (s[l] == '0')
    l--;
  if (s[l] == '.')
    s[l] = '\0';
  precision++;
} while (strcmp (s, "0") == 0);

label = gtk_label_new (s);
g_free (s);

It’s hard to say whether there is any style of writing code to avoid such errors in C programs, where you have to manually manage memory allocation and deallocation. Probably, only a careful review of the code, static and dynamic analyzers will help.

Fragment N16-29. Simpler memory leaks

The PVS-Studio analyzer also detects other memory leaks in the GTK library, but they are not so interesting. The errors boil down to the fact that there is an exit from a function in which they forgot to free some memory. Example:

static void
update_columns (GtkTreeView *view, ViewColumnModel *view_model)
{
  ....
  int *new_order;
  new_order = g_new (int, length);      // <=
  ....
  while (a->data == b->data)
  {
    a = a->next;
    b = b->next;
    if (a == NULL)
      return;                           // <=
    m++;
  }
  ....
  g_free (new_order);
  ....
}

PVS-Studio warning: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] The function was exited without releasing the ‘new_order’ pointer. A memory leak is possible. testtreecolumns.c 441

In this case, if the lists are of different lengths, the function exits prematurely. In this case, the memory is not freed, the pointer to which is stored in the variable new_order.

Fragment N30. Error in logic

static void
gtk_section_model_default_get_section (GtkSectionModel *self,
                                       guint            position,
                                       guint           *out_start,
                                       guint           *out_end)
{
  guint n_items = g_list_model_get_n_items (G_LIST_MODEL (self));

  if (position >= n_items)
    {
      *out_start = n_items;
      *out_end = G_MAXUINT;
    }

  *out_start = 0;
  *out_end = n_items;
}

PVS-Studio warnings:

• V519 [CWE-563, CERT-MSC13-C] The ‘*out_start’ variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 67, 71. gtksectionmodel.c 71
• V519 [CWE-563, CERT-MSC13-C] The ‘*out_end’ variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 68, 72. gtksectionmodel.c 72

Something is clearly wrong with the code. There is no point in overwriting variable values. Perhaps the code is missing else-branches:

if (position >= n_items)
{
  *out_start = n_items;
  *out_end = G_MAXUINT;
}
else
{
  *out_start = 0;
  *out_end = n_items;
}

I’m not sure that the code should be corrected this way, because… I’m not familiar with the GTK project. I can only say that the code contains an anomaly, but I can only guess how to correct it correctly.

Fragment N31. Nice typo in the condition

static int
gtk_tree_list_row_sort_keys_compare (gconstpointer a,
                                     gconstpointer b,
                                     gpointer      data)
{
  ....
  gpointer *keysa = (gpointer *) a;
  gpointer *keysb = (gpointer *) b;
  ....
  resa = unpack (keysa, &keysa, &sizea);
  resb = unpack (keysb, &keysb, &sizeb);
  if (!resa)
    return resb ? GTK_ORDERING_LARGER :
      (keysa[2] < keysb[2] ? GTK_ORDERING_SMALLER : 
      (keysb[2] > keysa[2] ? GTK_ORDERING_LARGER : GTK_ORDERING_EQUAL));
  else if (!resb)
    return GTK_ORDERING_SMALLER;
  ....
}

Try to find the error without reading the warning. Do you see her? Our unicorn is still waiting for you and drinking coffee.

Either way, I think it took some effort. It’s good that the helpers have tools such as static analyzers.

PVS-Studio warning: V547 [CWE-570] Expression ‘keysb[2] >keysa[2]’ is always false. gtktreelistrowsorter.c 158

The error is here:

.... keysa[2] < keysb[2] ? ....
.... keysb[2] > keysa[2] ? ....

These conditions are identical. Swap the arrays and replace < on >. In the end the same thing happened.

Fragment N32. Tab character

And at the end there is a very beautiful example of “smell code”. It knocks me off my feet 🙂

PVS-Studio warning: V581 [CWE-670] The conditional expressions of the ‘if’ statements located alongside each other are identical. Check lines: 674, 675. sass2scss.cpp 675

See those “two spaces”? So, these are not spaces, but a tab character!

Instead of typing ‘\t’, someone unthinkingly pressed the tab character. A very bad idea for many reasons:

• The code is confusing;
• It may coincide that the tab character will generally look like a space in the code, which will be even more confusing;
• When refactoring, auto-formatting code, merging code, and so on, the tab character can easily be replaced by several spaces.

I described the use of tabs in string literals in the book “60 antipatterns for a C++ programmer“. I invite you to read it if you want to learn even more ways on how not to write code.

Note. It’s probably not very clear to you why the analyzer issued a warning about identical checks. The fact is that the analyzer detected this error indirectly. To simplify the lexer and parser, we treat tabs and spaces the same way inside the parser. In fact, all tabs are replaced with spaces. Therefore, it turned out that the analyzer considers the conditions to be identical.

Fragment N33. Wrong constant

typedef enum {
  GDK_MEMORY_B8G8R8A8_PREMULTIPLIED,
  GDK_MEMORY_A8R8G8B8_PREMULTIPLIED,
  GDK_MEMORY_R8G8B8A8_PREMULTIPLIED,
  ....
  // Тут много вырезано.
  // Всего здесь 27 именованных констант.
  // GDK_MEMORY_N_FORMATS - двадцать восьмая,
  // но равна она 27, так как значения идут от 0.
  ....
  GDK_MEMORY_A16 GDK_AVAILABLE_ENUMERATOR_IN_4_12,
  GDK_MEMORY_A16_FLOAT GDK_AVAILABLE_ENUMERATOR_IN_4_12,
  GDK_MEMORY_A32_FLOAT GDK_AVAILABLE_ENUMERATOR_IN_4_12,

  GDK_MEMORY_N_FORMATS
} GdkMemoryFormat;

static const char *format_name[] = {
  "BGRAp", "ARGBp", "RGBAp",
  "BGRA", "ARGB", "RGBA", "ABGR",
  "RGB", "BGR", NULL
};

static const char *
format_to_string (GdkMemoryFormat format)
{
  if (format < GDK_MEMORY_N_FORMATS)
    return format_name[format];
  else
    return "ERROR";
}

PVS-Studio warning: V557 [CWE-119, CERT-ARR30-C] Array overrun is possible. The value of ‘format’ index could reach 27. testupload.c 13

Constant GDK_MEMORY_N_FORMATS has nothing to do with the array format_name. The array clearly has fewer elements than 27. So the check does not protect against going beyond the array boundary at all. This is an interesting variant of a typo, which is not even clear how it appeared. I couldn’t find any suitable constants nearby to check, so this is definitely a place developers should take a look at.

Thank you for your attention

I hope this article will fix a few bugs in the GTK library and give readers more insight into static code analysis methodology.

Please keep in mind that such checks are not an effective way to use the analyzer, but only demonstrate its ability to find bugs and help with code reviews. A static analyzer should be used regularly, not occasionally.

I invite you try PVS-Studio on your work or home projects.

If you want to share this article with an English-speaking audience, please use the translation link: Andrey Karpov. Typos, null pointers, and treacherous TAB: 33 fragments in the GTK library.