Typos, null pointers, and treacherous TAB: 33 fragments in the GTK library
Tags: cpp, programming, opensource
Published: 2023-10-30 12:43:15

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.


![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/v4qpog8qk7m8xgl5xvjy.png)

I would prefer not to repeat the introduction from the previous article: ["Finding typos in the GTK 4 project by PVS-Studio"](https://pvs-studio.com/en/blog/posts/cpp/0793/). 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](https://gitlab.gnome.org/GNOME/gtk) 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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"](https://pvs-studio.com/en/blog/posts/cpp/1053/). 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 ](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/cpp/1076/)**