GTK – популярный фреймворк с открытым исходным кодом для создания графических интерфейсов, который интересно проверять с помощью анализатора PVS-Studio. Тем более, что предыдущую проверку мы делали около 3 лет назад, а значит, наверняка найдём в нём новые ошибки.
Очень не хотелось частично повторять введение из прошлой статьи "Выявляем опечатки в проекте GTK 4 с помощью PVS-Studio", но подозреваю, что далеко не все читатели знакомы с GTK. Поэтому вкратце: библиотека позволяет кроссплатформенно реализовывать графический пользовательский интерфейс. Полностью бесплатна и имеет открытый исходный код, лицензированный под GNU GPL, что позволяет использовать её в любых проектах (даже коммерческих).
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\"Как будет обработана эта последовательность — зависит от компилятора.
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 антипаттернов для С++ программиста". Смотрите двадцатый вредный совет "Компактный код".
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 можно передать нулевой указатель. Просто лишня строчка кода.
Это тот случай, когда статический анализатор помогает сократить или упростить код. Сейчас будет аналогичная ситуация.
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;С точки зрения логики работы кода ничего не изменилось. Так стоит ли упрощать код? Я считаю, что да. Чем сложнее код, тем больше он "притягивает" ошибки.
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);Это не единственная подобная ошибка в коде. Как минимум она присутствует в следующих местах:
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).
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 исчез. Решил попозже поэкспериментировать.
Ссылки для тех, что ещё не знаком с этим паттерном ошибки:
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);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, где вручную приходится управлять выделением и освобождением памяти. Наверное, поможет только внимательный обзор кода, статические и динамические анализаторы.
В библиотеке 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.
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:
С кодом явно что-то не так. Нет смысла перезаписывать значения переменных. Возможно, в коде не хватает else-ветки:
if (position >= n_items)
{
*out_start = n_items;
*out_end = G_MAXUINT;
}
else
{
*out_start = 0;
*out_end = n_items;
}Я не уверен, что код следует исправлять именно так, т.к. не знаком с устройством проекта GTK. Я могу только сказать, что код содержит аномалию, а вот как её корректно исправить – могу только предполагать.
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] ? ....Эти условия идентичны. Поменяли местами массивы и заменили < на >. В итоге получилось то же самое.
И в конце очень красивый пример "кода с запахом". Прям с ног валит :)
Предупреждение 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 антипаттернов для С++ программиста". Приглашаю почитать, если хочется узнать ещё больше способов, как не надо писать код.
Примечание. Наверное, вам не очень понятно, почему анализатор выдал предупреждение про одинаковые проверки. Дело в том, что анализатор выявил эту ошибку косвенно. Для упрощения лексера и парсера мы одинаково интерпретируем табуляции и пробелы внутри анализатора. Фактически все табуляции заменяются на пробелы. Поэтому и получилось, что анализатор считает, что условия идентичны.
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 на ваших рабочих или домашних проектах.