Typos, null pointers, and treacherous TAB: 33 fragments in the GTK library

GTK is a popular open-source framework for creating graphical user interfaces. The project is interesting to check with the PVS-Studio analyzer. Especially since the last time we checked it was about 3 years ago. So, we'll probably find new errors in it.

I would prefer not to repeat the introduction from the previous article: "Finding typos in the GTK 4 project by PVS-Studio". However, I guess that not all of our readers are familiar with GTK. I'll make it short: GTK is a framework that enables you to implement a cross-platform GUI. It's a completely free and open-source framework licensed under the GNU GPL, which allows you to use it in any project (even a commercial one).

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));
}

The 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

A typo lurks in the string literal. Developers wanted to use an escape sequence for double quotes:

\"%s\"

But the quote character was left out, and a non-existent control sequence containing a slash and a percent sign appeared:

\%s\"

The way the sequence is 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);
  }
  ....
}

The 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 twice to the Sass_OP::DIV named constant. I am pretty sure 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);
}

By the time I was done writing it, I realized that the code looked messy and even worse than it did before. Even though I prefer table-style formatting, it is somewhat irrelevant in this case. If you don't know what "table-style formatting" is, you may want to read about it in a mini-book "60 terrible tips for a C++ developer". Look for the terrible tip N20 — "Compact code".

Fragment N3. Pointless g_free call

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);
}

The 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

The second call to the g_free function is unnecessary here because NULL is always passed to it. Nothing bad will happen because of it. You can pass a null pointer to g_free. It's just a redundant line of code.

This is the case when a static analyzer helps shorten or simplify the code. The next example is similar.

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;
}

The 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

The condition:

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

It can be shortened:

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

Nothing has changed in terms of the code logic. So, should we simplify the code? In my opinion, yes. The more complex the code is, the more errors it "attracts".

Fragments N5-N11. Potential use of the 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);
  ....
}

The 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

The action pointer is used and only then checked if it's NULL.

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

The check indicates that the pointer may be null. It's better to rewrite the code in this way, for example:

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. It is present in the following fragments:

• 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

The full article is here