Опечатки, нулевые указатели и коварный таб: 33 фрагмента в библиотеке GTK26.10.2023 11:00

GTK — популярный фреймворк с открытым исходным кодом для создания графических интерфейсов, который интересно проверять с помощью анализатора PVS-Studio. Тем более, что предыдущую проверку мы делали около 3 лет назад, а значит, наверняка найдём в нём новые ошибки.

Очень не хотелось частично повторять введение из прошлой статьи «Выявляем опечатки в проекте GTK 4 с помощью PVS-Studio», но подозреваю, что далеко не все читатели знакомы с GTK. Поэтому вкратце: библиотека позволяет кроссплатформенно реализовывать графический пользовательский интерфейс. Полностью бесплатна и имеет открытый исходный код, лицензированный под GNU GPL, что позволяет использовать её в любых проектах (даже коммерческих).

Фрагмент 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: 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

В строковый литерал вкралась опечатка. Хотели экранировать двойные кавычки:

\"%s\"

Но случайно пропустили символ кавычек, и возникла несуществующая управляющая последовательность, состоящая из косой и символа процентов:

\%s\"

Как будет обработана эта последовательность — зависит от компилятора.

Фрагмент N2. Опечатка в условии

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: 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

Два раза переменная сравнивается с именованной константой Sass_OP: DIV. Есть большое подозрение, что второй константой должна быть Sass_OP: MOD.

Ошибку было бы проще заметить, если форматировать все однотипные блоки в условиях столбиком. Вот так:

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

Когда написал, понял, что код смотрится неопрятно и стало ещё хуже, чем было. Хотя я большой сторонник табличного форматирования кода, в данном случае оно как-то неуместно получается. Если вам непонятен термин «табличное форматирование», то предлагаю познакомиться с ним в мини-книге »60 антипаттернов для С++ программиста». Смотрите двадцатый вредный совет «Компактный код».

Фрагмент N3. Бессмысленный вызов 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: V575 [CWE-628, CERT-EXP37-C] The null pointer is passed into 'g_free' function. Inspect the first argument. gtkcssparser.c 189

Здесь второй вызов функции g_free явно лишний, так как в него всегда передаётся NULL. Ничего страшного из-за этого не случится. В g_free можно передать нулевой указатель. Просто лишня строчка кода.

Это тот случай, когда статический анализатор помогает сократить или упростить код. Сейчас будет аналогичная ситуация.

Фрагмент N4. Переусложнённая проверка

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: 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

Условие:

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

Можно сократить:

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

С точки зрения логики работы кода ничего не изменилось. Так стоит ли упрощать код? Я считаю, что да. Чем сложнее код, тем больше он «притягивает» ошибки.

Фрагмент N5-N11. Потенциальное использование нулевого указателя

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: V595 [CWE-476, CERT-EXP12-C] The 'action' pointer was utilized before it was verified against nullptr. Check lines: 449, 452. gtkactionmuxer.c 449

Указатель action используется, а затем проверяется на равенство NULL.

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

Наличие проверки говорит о том, что указатель может быть нулевым. Код явно следует переписать, например так:

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

Это не единственная подобная ошибка в коде. Как минимум она присутствует в следующих местах:

• 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

Фрагмент N12. Несовместимые способы выделения и освобождения памяти

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: 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

Память выделяется с помощью функции malloc, а освобождается с помощью функции g_free. В разделе «Memory Allocation» Glib указано, что так делать нельзя:

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

Фрагмент 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: 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

Этот фрагмент кода я описывал ещё в предыдущей статье начала 2021 года, но изменений в код не было внесено до сих пор. Код опасен тем, что компилятор потенциально может удалить вызов функции memset в целях оптимизации, так как этот буфер более не используется и сразу освобождается.

Пожалуй, надо будет отдельно исследовать этот момент. Интересно попробовать повторить, действительно ли получится скомпилировать код библиотеки так, чтобы вызов функции memset исчез. Решил попозже поэкспериментировать.

Ссылки для тех, что ещё не знаком с этим паттерном ошибки:

Фрагмент N14. Невалидный указатель

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: 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

Если указатель end нулевой, то к нему прибавляется длина строки. В результате получается невалидный указатель, который нельзя использовать. Скорее всего, на самом деле хотели написать вот это:

end = beg + strlen(beg);

Фрагмент N15. Утечка памяти

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: 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

Функция g_strdup_printf схожа со стандартной C функцией sprintf, но безопасней, так как она рассчитывает максимальное требуемое пространство и выделяет нужное количество памяти, в которую записывает результирующую строку. Возвращаемая строка должна освобождаться, когда больше не нужна.

Вот как раз освобождения буфера в этом коде нет. Поэтому на каждой итерации цикла будет происходить утечка памяти. Следует добавить вызов g_free при каждой новой итерации цикла:

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

Затрудняюсь сказать, есть ли какой-то стиль написания кода, чтобы избежать таких ошибок в программах на языке C, где вручную приходится управлять выделением и освобождением памяти. Наверное, поможет только внимательный обзор кода, статические и динамические анализаторы.

Фрагмент N16–29. Более простые утечки памяти

В библиотеке GTK анализатор PVS-Studio обнаруживает и другие утечки памяти, но они не столь интересны. Ошибки сводятся к тому, что существует выход из функции, при котором забыли освободить какую-то память. Пример:

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: 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

В данном случае, если списки оказываются разной длины, то происходит преждевременный выход из функции. При этом не происходит освобождение памяти, указатель на которую хранится в переменной new_order.

Фрагмент N30. Ошибка в логике

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:

• 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

С кодом явно что-то не так. Нет смысла перезаписывать значения переменных. Возможно, в коде не хватает else-ветки:

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

Я не уверен, что код следует исправлять именно так, т.к. не знаком с устройством проекта GTK. Я могу только сказать, что код содержит аномалию, а вот как её корректно исправить — могу только предполагать.

Фрагмент N31. Красивая опечатка в условии

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

Попробуйте, не читая предупреждение, найти ошибку. Видите её? Наш единорог пока ждёт вас и пьёт кофе.

В любом случае, думаю, это потребовало усилий. Хорошо, что в помощниках есть такие инструменты, как статические анализаторы.

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'keysb[2] > keysa[2]' is always false. gtktreelistrowsorter.c 158

Ошибка здесь:

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

Эти условия идентичны. Поменяли местами массивы и заменили < на >. В итоге получилось то же самое.

Фрагмент N32. Символ табуляции

И в конце очень красивый пример «кода с запахом». Прям с ног валит :)

Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 674, 675. sass2scss.cpp 675

Видите эти «два пробела»? Так вот, это не пробелы, а символ табуляции!

Вместо того, чтобы написать '\t', кто-то, не задумываясь, нажал символ табуляции. Очень плохая идея по множеству причин:

• Код сбивает с толку;
• Может так совпасть, что символ табуляции вообще будет выглядеть в коде как пробел, что ещё больше будет сбивать с толку;
• При рефакторинге, автоформатировании кода, слиянии кода и так далее, символ табуляции легко может быть заменён на несколько пробелов.

Использование табуляции в строковых литералах я описал в книге »60 антипаттернов для С++ программиста». Приглашаю почитать, если хочется узнать ещё больше способов, как не надо писать код.

Примечание. Наверное, вам не очень понятно, почему анализатор выдал предупреждение про одинаковые проверки. Дело в том, что анализатор выявил эту ошибку косвенно. Для упрощения лексера и парсера мы одинаково интерпретируем табуляции и пробелы внутри анализатора. Фактически все табуляции заменяются на пробелы. Поэтому и получилось, что анализатор считает, что условия идентичны.

Фрагмент N33. Не та константа

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: V557 [CWE-119, CERT-ARR30-C] Array overrun is possible. The value of 'format' index could reach 27. testupload.c 13

Константа GDK_MEMORY_N_FORMATS не имеет никакого отношения к массиву format_name. В массиве явно меньше элементов, чем 27. Так что проверка совсем не защищает от выхода за границу массива. Это интересный вариант опечатки, которая даже не понятно, как появилась. Мне не удалось найти рядом подходящих констант для проверки, поэтому на это место точно стоит взглянуть разработчикам.

Спасибо за внимание

Надеюсь, благодаря этой статье в библиотеке GTK будет исправлено несколько ошибок, а читатели больше проникнутся методологией статического анализа кода.

Прошу только учитывать, что подобные проверки не являются эффективным способом использования анализатора, а только демонстрируют его возможность находить баги и помогать при обзорах кода. Статический анализатор следует использовать регулярно, а не от случая к случаю.

Приглашаю попробовать PVS-Studio на ваших рабочих или домашних проектах.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Typos, null pointers, and treacherous TAB: 33 fragments in the GTK library.

© Habrahabr.ru