Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!
Возможно, вы уже читали недавнюю статью о процессе первого запуска PVS-Studio на примере проекта GTK 4 и о первичной фильтрации предупреждений. Теперь пришло время поработать с полученным отчётом более подробно. И как уже догадались наши постоянные читатели, предлагаю вашему вниманию статью с описанием найденных в коде ошибок.
У проекта GTK 4 хорошее качество кода
Далеко не всегда у меня бывает настроение выписать в статью побольше ошибок, как это было, например, в случае недавней публикации "Espressif IoT Development Framework: 71 выстрел в ногу". В этот раз я ограничусь 21 ошибкой в честь 2021 года :). Плюс, справедливости ради надо отметить высокое качество проекта GTK 4, и что плотность настоящих ошибок весьма мала.
Проект популярен, хорошо тестируется и, как я понимаю, уже проверяется такими инструментами, как Clang static analysis tool, Coverity, AddressSanitizer, UndefinedBehavior Sanitizer. В общем, с качеством кода всё хорошо, и найти хотя-бы десяток ошибок — это уже хорошее достижение.
О процессе проверки GTK 4 и первичной фильтрации отчёта, вы можете прочитать в статье Святослава Размыслова "GTK: Как выглядит первый запуск анализатора в цифрах". После первичной фильтрации в отчёте осталось 581 предупреждение первого и второго уровня достоверности (на третий уровень я заглядывать не стал). 581 предупреждение и только двадцать одна выписанная ошибка: разве не маловато? Это нормально.
Ещё раз повторю, что код проекта, на мой взгляд, является качественным. И большинство предупреждений возникают из-за неудачных макросов и использования директив условной компиляции. Причём, нельзя даже сказать, что анализатор выдал ложное срабатывание. Он вроде, как и прав, но толку от этого всё равно нет. Проще всего это пояснить на примере.
Представьте, что вы видите следующий код:
bool var;
var = true;
if (!var && foo)Думаю, вы согласитесь, что код выглядит подозрительно. Зачем так писать? Быть может где-то забыли изменить значение переменной var? Код подозрителен, и вы не удивитесь, что он не понравится и анализатору. Статический анализатор PVS-Studio выдаст предупреждение "A part of conditional expression is always false: !var". Обоснованное предупреждение? Да.
Но на практике, как всегда, есть нюансы. Вот идентичный по смыслу код из GTK 4, но ничего подозрительного и опасного в нём уже нет:
gboolean debug_enabled;
#ifdef G_ENABLE_DEBUG
debug_enabled = TRUE;
#else
debug_enabled = FALSE;
#endif
....
if (!debug_enabled && !keys[i].always_enabled)Анализатор по прежнему выдаёт предупреждение: V560 [CWE-570] A part of conditional expression is always false: !debug_enabled. gdk.c 281
Но как вы понимаете, толку от этого предупреждения нет. Вот такой он, реальный мир. И часто именно так и возникают бесполезные срабатывания.
К счастью, в этом ничего страшного нет. Как уже было указано в предыдущей статье, есть масса способов избавиться от этих лишних срабатываний: изменить код, точечно подавить предупреждения, размечать макросы, использовать базу для массового подавления предупреждений и так далее.
Замеченные ошибки
Как мне кажется, достаточно много найденных ошибок связано с невнимательностью. Другими словами, причиной появления этих ошибок являются опечатки. С них и начнём.
Опечатки
Фрагмент N1. Красивая опечатка в цикле
void
gsk_vulkan_image_upload_regions (GskVulkanImage *self,
GskVulkanUploader *uploader,
guint num_regions,
GskImageRegion *regions)
{
....
for (int i = 0; i < num_regions; i++)
{
m = mem + offset;
if (regions[i].stride == regions[i].width * 4)
{
memcpy (m, regions[i].data, regions[i].stride * regions[i].height);
}
else
{
for (gsize r = 0; r < regions[i].height; i++) // <=
memcpy (m + r * regions[i].width * 4,
regions[i].data + r * regions[i].stride, regions[i].width * 4);
}
....
}
....
}Предупреждение PVS-Studio: V533 [CWE-691] It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. gskvulkanimage.c 721
Обратите внимание, что во вложенном цикле инкрементируется не переменная r, а i. Что-то дополнительно пояснять здесь не требуется. Это золотая классика!
Фрагмент N2. Цикл который не выполняется
В предыдущем случае в функции мог начать выполняться цикл с неконтролируемом количеством итераций. И всё бы закончилось, когда функция memcpy записала бы что-то ну совсем не туда, куда можно. Т.е. всё бы закончилось Segmentation fault.
А здесь, наоборот второй цикл вообще не будет выполняться:
GtkCssValue *
_gtk_css_border_value_parse (GtkCssParser *parser,
GtkCssNumberParseFlags flags,
gboolean allow_auto,
gboolean allow_fill)
{
....
guint i;
....
for (; i < 4; i++) // <=
{
if (result->values[(i - 1) >> 1])
result->values[i] = _gtk_css_value_ref (result->values[(i - 1) >> 1]);
}
result->is_computed = TRUE;
for (; i < 4; i++) // <=
if (result->values[i] && !gtk_css_value_is_computed (result->values[i]))
{
result->is_computed = FALSE;
break;
}
....
}Предупреждение PVS-Studio: V621 [CWE-835] Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. gtkcssbordervalue.c 221
После завершения первого цикла, значение счётчика i равно 4. Следовательно, второй цикл не выполнит ни одной итерации. Не хватает сброса счётчика в ноль.
Фрагмент N3. Повторное использование одной и той же константы
static void
gtk_list_base_class_init (GtkListBaseClass *klass)
{
....
properties[PROP_ORIENTATION] =
g_param_spec_enum ("orientation",
P_("Orientation"),
P_("The orientation of the orientable"),
GTK_TYPE_ORIENTATION,
GTK_ORIENTATION_VERTICAL,
G_PARAM_READWRITE | G_PARAM_EXPLICIT_NOTIFY |
G_PARAM_EXPLICIT_NOTIFY);
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'G_PARAM_EXPLICIT_NOTIFY' to the left and to the right of the '|' operator. gtklistbase.c 1151
Для генерации маски дважды используется константа G_PARAM_EXPLICIT_NOTIFY. Программист явно задумывал использовать разные константы, и маске в итоге выставлены не все нужные биты.
Фрагмент N4. Путаница в порядке аргументов
Для начала взглянем на объявление функции post_insert_fixup. Обратите внимание на порядок формальных аргументов char_count_delta и line_count_delta.
static void post_insert_fixup (GtkTextBTree *tree,
GtkTextLine *insert_line,
int char_count_delta,
int line_count_delta);А теперь, изучим фрагмент кода, где происходит вызов этой функции:
void
_gtk_text_btree_insert (GtkTextIter *iter,
const char *text,
int len)
{
....
int line_count_delta; /* Counts change to total number of
* lines in file.
*/
int char_count_delta; /* change to number of chars */
....
post_insert_fixup (tree, line, line_count_delta, char_count_delta);
....
}Предупреждение PVS-Studio: V764 [CWE-683] Possible incorrect order of arguments passed to 'post_insert_fixup' function: 'line_count_delta' and 'char_count_delta'. gtktextbtree.c 1230
Из-за опечатки перепутаны местами третий и четвёртый аргумент функции. Так как типы аргументов совпадают, код успешно и без предупреждений компилируется, но смысла в нём нет.
В PVS-Studio достаточно много эмпирических диагностик, которые выявляют абсолютно корректный код с точки зрения компиляции, но абсурдные по своей сути. Использование анализатора позволяет выявить эти ошибки на самом раннем этапе и позволить команде при обзоре кода сосредоточиться на более высокоуровневых проблемах, а не выискивать опечатки. Поэтому пока вы читаете это статью, предлагаю параллельно скачать дистрибутив и запросить демонстрационный ключ.
Фрагмент N5. Ещё один впечатляющий случай путаницы в порядке аргументов
Обратите внимание на аргументы вызываемой функции:
static guint
translate_keysym (GdkX11Keymap *keymap_x11,
guint hardware_keycode,
int group,
GdkModifierType state,
int *effective_group,
int *effective_level)
{
....
}Неудачный вызов функции, которая была приведена выше:
static gboolean
gdk_x11_keymap_translate_keyboard_state (GdkKeymap *keymap,
guint hardware_keycode,
GdkModifierType state,
int group,
guint *keyval,
int *effective_group,
int *level,
GdkModifierType *consumed_modifiers)
{
....
tmp_keyval = translate_keysym (keymap_x11, hardware_keycode,
group, state,
level, effective_group); // <=
....
}Предупреждение PVS-Studio: V764 [CWE-683] Possible incorrect order of arguments passed to 'translate_keysym' function: 'level' and 'effective_group'. gdkkeys-x11.c 1386
В этот раз что-то напутано в работе с клавиатурой. Вновь опечатка: перепутаны местами фактические аргументы level и effective_group.
Если кто-то пока не решился скачать и попробовать PVS-Studio, то сейчас самое время :). Неужели вам действительно нравится выискивать подобные ошибки глазами? Или, не дай бог, сражаться с ними в отладчике?
Фрагмент N6. Забыли звёздочку (*)
gboolean
gtk_check_compact_table (...., int n_compose, ....)
{
....
guint16 *seq_index;
....
seq_index = bsearch (compose_buffer,
table->data,
table->n_index_size,
sizeof (guint16) * table->n_index_stride,
compare_seq_index);
if (!seq_index)
return FALSE;
if (seq_index && n_compose == 1)
return TRUE;
....
}Предупреждение PVS-Studio: V560 [CWE-571] A part of conditional expression is always true: seq_index. gtkimcontextsimple.c 475
Вторая проверка указателя seq_index не имеет смысла. Указатель уже проверен выше и в момент второй проверки точно не является нулевым. Я не знаю, что именно должен делать этот код, но рискну предположить, что во втором случае хотели проверить не сам указатель, а значение, на которое он ссылается. Другими словами, мне кажется, что забыли разыменовать указатель. Тогда правильный код должен быть таким:
if (!seq_index)
return FALSE;
if (*seq_index && n_compose == 1)
return TRUE;Фрагмент N7-N9. Повторные присваивания
static void
gtk_message_dialog_init (GtkMessageDialog *dialog)
{
GtkMessageDialogPrivate *priv = ....;
....
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;
....
}Предупреждения PVS-Studio:
- V519 [CWE-563] The 'priv->has_primary_markup' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 262, 264. gtkmessagedialog.c 264
- V519 [CWE-563] The 'priv->has_secondary_text' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 263, 265. gtkmessagedialog.c 265
Вот этот блок кода повторяется два раза:
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;Возможно, второй блок просто лишний. А возможно, забыли инициализировать что-то другое. В любом случае это какая-то разновидность опечатки.
Есть ещё пара таких же бессмысленных повторных присваиваний:
- V519 [CWE-563] The 'self->state' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2851, 2855. gdkevents.c 2855
- V519 [CWE-563] The 'display->width' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2635, 2640. gtktextlayout.c 2640
Проблемы с нулевыми указателями
Фрагмент N10. Использование указателя до проверки
static gboolean
on_flash_timeout (GtkInspectorWindow *iw)
{
iw->flash_count++;
gtk_highlight_overlay_set_color (GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay),
&(GdkRGBA) {
0.0, 0.0, 1.0,
(iw && iw->flash_count % 2 == 0) ? 0.0 : 0.2
});
....
}Предупреждение PVS-Studio: V595 [CWE-476] The 'iw' pointer was utilized before it was verified against nullptr. Check lines: 194, 199. inspect-button.c 194
В начале указатель iw смело разыменовывается, чтобы инкрементировать один из членов класса. И только при чтении кода ниже выясняется, что этот указатель на самом деле может быть нулевым. Это следует из наличия проверки:
(iw && iw->flash_count % 2 == 0)Чтобы исправить ситуацию, нужно добавить ещё одну проверку:
if (iw)
iw->flash_count++;
gtk_highlight_overlay_set_color (GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay),
&(GdkRGBA) {
0.0, 0.0, 1.0,
(iw && iw->flash_count % 2 == 0) ? 0.0 : 0.2
});Впрочем, такой правки окажется всё равно недостаточно :). Если присмотреться, то можно увидеть ещё одно разыменование при вычислении аргументов:
GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay)Так как я не знаком с логикой работой программы, то не буду строить предположения, как дальше править код, и передаю эстафету авторам проекта.
Фрагмент N11. Множественное использование указателя до проверки
static void
cups_dispatch_watch_finalize (GSource *source)
{
GtkPrintCupsDispatchWatch *dispatch;
....
const char *username;
char hostname[HTTP_MAX_URI];
char *key;
httpGetHostname (dispatch->request->http, hostname, sizeof (hostname));
if (is_address_local (hostname))
strcpy (hostname, "localhost");
if (dispatch->backend->username != NULL) // <=
username = dispatch->backend->username; // <=
else
username = cupsUser ();
key = g_strconcat (username, "@", hostname, NULL);
GTK_NOTE (PRINTING,
g_print ("CUPS backend: removing stored password for %s\n", key));
g_hash_table_remove (dispatch->backend->auth, key); // <=
g_free (key);
if (dispatch->backend) // <=
dispatch->backend->authentication_lock = FALSE;
....
}Предупреждение PVS-Studio: V595 [CWE-476] The 'dispatch->backend' pointer was utilized before it was verified against nullptr. Check lines: 1603, 1613. gtkprintbackendcups.c 1603
Это ещё более "бесстрашный" код :). Указатель dispatch->backend разыменовывается несколько раз (см. участки кода, выделенные комментариями). И только намного позже вспоминают, что указатель-то оказывается может быть нулевым! И делается соответствующая проверка:
if (dispatch->backend)Но уже поздно :).
Фрагмент N12. Нет защиты, если оба указателя будут нулевыми
static GskRenderNode *
gtk_snapshot_collect_blend_top (GtkSnapshot *snapshot,
GtkSnapshotState *state,
GskRenderNode **nodes,
guint n_nodes)
{
GskRenderNode *bottom_node, *top_node, *blend_node;
GdkRGBA transparent = { 0, 0, 0, 0 };
top_node = gtk_snapshot_collect_default (snapshot, state, nodes, n_nodes);
bottom_node = state->data.blend.bottom_node != NULL
? gsk_render_node_ref (state->data.blend.bottom_node)
: NULL;
g_assert (top_node != NULL || bottom_node != NULL);
if (top_node == NULL)
top_node = gsk_color_node_new (&transparent, &bottom_node->bounds);
if (bottom_node == NULL)
bottom_node = gsk_color_node_new (&transparent, &top_node->bounds);
....
}V595 [CWE-476] The 'bottom_node' pointer was utilized before it was verified against nullptr. Check lines: 1189, 1190. gtksnapshot.c 1189
Предполагается, что указатели top_node и bottom_node не могут быть одновременно нулевыми. Это подтверждается наличием строчки:
g_assert (top_node != NULL || bottom_node != NULL);Но это никак не защищает релизную версию программы, в которой макрос g_assert раскроется в пустоту. Намного лучше явно рассмотреть такой случай. Например, можно написать:
if (top_node == NULL && bottom_node == NULL)
{
g_assert (false);
return NULL;
}Неудачные или лишние проверки
Фрагмент N13. Лишняя проверка
static void
stash_desktop_startup_notification_id (void)
{
const char *desktop_startup_id;
desktop_startup_id = g_getenv ("DESKTOP_STARTUP_ID");
if (desktop_startup_id && *desktop_startup_id != '\0')
{
if (!g_utf8_validate (desktop_startup_id, -1, NULL))
g_warning ("DESKTOP_STARTUP_ID contains invalid UTF-8");
else
startup_notification_id =
g_strdup (desktop_startup_id ? desktop_startup_id : "");
}
g_unsetenv ("DESKTOP_STARTUP_ID");
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'desktop_startup_id' is always true. gdk.c 176
Возможно, здесь и нет ошибки, но код является избыточным. Избыточного кода следует избегать, так как он, во-первых, усложняет изучение и поддержку кода, а во-вторых, повышает вероятность внедрения ошибок при правке.
Данный код можно упростить, удалив вторую проверку указателя:
desktop_startup_id = g_getenv ("DESKTOP_STARTUP_ID");
if (desktop_startup_id && *desktop_startup_id != '\0')
{
if (!g_utf8_validate (desktop_startup_id, -1, NULL))
g_warning ("DESKTOP_STARTUP_ID contains invalid UTF-8");
else
startup_notification_id = g_strdup (desktop_startup_id);
}Фрагмент N14. Переменная не изменяет своё значение
Лишних проверок вообще всегда везде много, причем это редко настоящие ошибки. Чаще это неаккуратность и избыточный код. При написании статей я просматриваю всегда ложные/истинные условия весьма поверхностно, чтобы сэкономить время и повнимательнее изучить более интересные предупреждения. Но это не значит, что авторам проекта, не следует изучать такие предупреждения. Можно на раннем этапе устранить ошибки или удалить избыточные условия, тем самым делая код более простым и изящным.
Ещё один случай, когда проверка не имеет смысла:
#define MAX_LIST_SIZE 1000
static void
gtk_recent_manager_real_changed (GtkRecentManager *manager)
{
....
int age;
int max_size = MAX_LIST_SIZE;
....
.... // Переменная max_size не изменяется здесь.
....
if (age == 0 || max_size == 0 || !enabled)
{
g_bookmark_file_free (priv->recent_items);
priv->recent_items = g_bookmark_file_new ();
priv->size = 0;
}
else
{
if (age > 0)
gtk_recent_manager_clamp_to_age (manager, age);
if (max_size > 0)
gtk_recent_manager_clamp_to_size (manager, max_size);
}
....
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'max_size > 0' is always true. gtkrecentmanager.c 480
Дело в том, что значение переменной max_size после её объявления и инициализации, больше не изменяется. Это достаточно странно. Это код может быть как просто избыточным, так и содержать ошибку в логике своей работе, если переменную забыли где-то изменить.
Примечание. Внимательный читатель задал вопрос: а почему нет предупреждение на часть подвыражения "max_size == 0"? Оно есть. Просто я его пропустил при беглом просмотре отчёта и потом при написании статьи тоже не обратил на этот момент внимание. Вот это предупреждение: V560 [CWE-570] A part of conditional expression is always false: max_size == 0. gtkrecentmanager.c 470.
Фрагмент N15, N16. Использование индекса до его проверки
static void
action_handle_method (GtkAtSpiContext *self,
const char *method_name,
GVariant *parameters,
GDBusMethodInvocation *invocation,
const Action *actions,
int n_actions)
{
....
int idx = -1;
g_variant_get (parameters, "(i)", &idx);
const Action *action = &actions[idx];
if (idx >= 0 && idx < n_actions)
g_dbus_method_invocation_return_value (
invocation, g_variant_new ("(s)", action->name));
else
g_dbus_method_invocation_return_error (invocation,
G_IO_ERROR,
G_IO_ERROR_INVALID_ARGUMENT,
"Unknown action %d",
idx);
....
}Предупреждение PVS-Studio: V781 [CWE-129] The value of the 'idx' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 71, 73. gtkatspiaction.c 71
В начале переменная idx используется для доступа к элементам массива:
const Action *action = &actions[idx];И только затем проверяется, не отрицательное или не слишком ли большое значение находится в этой переменной:
if (idx >= 0 && idx < n_actions)Естественно, проверка выполнена слишком поздно, и уже произошел выход за границу массива, что приводит к неопределённому поведению программы.
Аналогичная ситуация: V781 [CWE-129] The value of the 'idx' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 132, 134. gtkatspiaction.c 132
Фрагмент N17, N18. Недостижимый код
static gboolean
parse_n_plus_b (GtkCssParser *parser,
int before,
int *a,
int *b)
{
const GtkCssToken *token;
token = gtk_css_parser_get_token (parser);
if (gtk_css_token_is_ident (token, "n"))
{
....
return parse_plus_b (parser, FALSE, b);
}
else if (gtk_css_token_is_ident (token, "n-"))
{
....
return parse_plus_b (parser, TRUE, b);
}
else if (gtk_css_token_is (token, GTK_CSS_TOKEN_IDENT) &&
string_has_number (token->string.string, "n-", b))
{
....
return TRUE;
}
else
{
*b = before;
*a = 0;
return TRUE;
}
gtk_css_parser_error_syntax (parser, "Not a valid an+b type");
return FALSE;
}Предупреждение PVS-Studio: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. gtkcssselector.c 1077
Функция содержит последовательность вида if-else-if-else… При этом тело каждого условного оператора заканчивается выходом из функции. И это странно, так как в конце функции находится фрагмент кода, который никогда не получит управление.
Ещё одна схожая ситуация с недостижимым кодом: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. gtktreemodelfilter.c 3289
Разное
Фрагмент N19, N20. Целочисленное деление
static void
gtk_paint_spinner (GtkStyleContext *context,
cairo_t *cr,
guint step,
int x,
int y,
int width,
int height)
{
GdkRGBA color;
guint num_steps;
double dx, dy;
....
dx = width / 2;
dy = height / 2;
....
cairo_move_to (cr,
dx + (radius - inset) * cos (i * G_PI / half),
dy + (radius - inset) * sin (i * G_PI / half));
cairo_line_to (cr,
dx + radius * cos (i * G_PI / half),
dy + radius * sin (i * G_PI / half));
....
}Предупреждения PVS-Studio:
- V636 [CWE-682] The 'width / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkcellrendererspinner.c 412
- V636 [CWE-682] The 'height / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkcellrendererspinner.c 413
Результаты целочисленных делений записываются в переменные dx и dy. Подозрительно, что эти переменные имеют тип double. Скорее всего, это просто оплошность и на самом деле должно быть написано так:
dx = width / 2.0;
dy = height / 2.0;Аналогичные подозрительные деления можно найти в фрагменте кода, на который указывают эти два предупреждения:
- V636 [CWE-682] The 'width / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkswitch.c 255
- V636 [CWE-682] The 'width / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkswitch.c 257
Фрагмент N21. Пароль может не затираться в памяти
Под конец статьи, я приберёг весьма интересный случай. Распространённой ошибкой является использование функции memset для затирания приватных данных в памяти. В целях оптимизации, компиляторы любят удалять такие вызовы функции, так как, с точки зрения языка C и C++, если область памяти больше не используется после заполнения, то, собственно, и заполнять её не нужно. Другими словами, это разновидность оптимизации, когда компилятор удаляет запись значения в переменную, если далее из этой переменной не происходит чтения.
Я рассматривал в статьях подобные ошибки много раз, и не хочется повторяться с её подробным описанием. Поэтому, если вы ещё не знакомы с этим паттерном, то предлагаю ознакомиться с материалом, перейдя по следующим ссылкам:
- CWE-14: Compiler Removal of Code to Clear Buffers
- Безопасная очистка приватных данных
Что интересно в случае с GTK 4? Дело в том, что вызов функции free происходит через промежуточную функцию и здесь становится сложнее предсказать, начнёт выполнять компилятор оптимизацию или нет.
Итак, для освобождения памяти в GTK 4 используется функция g_free. Она реализована следующим образом:
void
g_free (gpointer mem)
{
free (mem);
TRACE(GLIB_MEM_FREE((void*) mem));
}Всегда ли g_free является просто оберткой над free? Начиная с GLib 2.46 это всегда так. Вот что сказано на эту тему в документации:
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).
Since GLib 2.46 g_malloc() is hardcoded to always use the system malloc implementation.
Итак, раз память в g_malloc выделятся с помощью malloc, то для её освобождения должна всегда вызываться функция free.
Теперь давайте посмотрим на проблемный код.
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] 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 848
После заполнения памяти нулями, указатель на эту область памяти передаётся в функцию g_free. И здесь, как никогда, проявит себя ошибка или нет, зависит от компилятора и используемых настроек оптимизации. Если компилятор выполнит межпроцедурную оптимизацию и вставит тело функции g_free в функцию overwrite_and_free, то он может сообразить, что функция memset лишняя и удалить её.
Вот такая интересная неприятная ошибка из сферы информационной безопасности.
Заключение
Статический анализатор PVS-Studio поддерживает большое количество сценариев своего использования. Во-первых, имеется в виду возможность его интеграции с IntelliJ IDEA, Rider, IncrediBuild, Jenkins, PlatformIO, Travis CI, GitLab CI/CD, CircleCI, TeamCity, Visual Studio и так далее. Во-вторых, есть различные варианты его внедрения, настройки, использования уведомлений. Мы, на самом деле, даже не рады, что в нём так много всего разного. Просто невозможно сделать короткую лаконичную документацию, как это, например, было 10 лет назад, когда PVS-Studio был просто плагином только для Visual Studio. А существующую большую документацию естественно никто не читает :). В результате программисты наступают на одни и те же грабли, допускают одни и те же ошибки при внедрении и задают в поддержку схожие вопросы.
К сожалению, исправить ситуацию с помощью более понятного интерфейса как-то не получается. Собственно, часто и никакого интерфейса-то и нет :). А есть только конфигурационные настройки, для интеграции, например, с SonarQube.
Поэтому мы думали и придумали выкладывать короткие видеоруководства по отдельным аспектам использования PVS-Studio. Так их, возможно, будет легче найти, быстро посмотреть и понять, что к чему. И что ещё важнее, можно подписаться на канал и знакомиться с новшествами постепенно, просматривая наши новые видеосоветы после их публикации. Лучше знакомиться с PVS-Studio постепенно, а не сразу пытаться осилить документацию :). Конечно, нет уверенности, что всё это хорошо получится, но давайте попробуем! Приглашаю подписаться: Возможности PVS-Studio (YouTube).
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Finding Typos in the GTK 4 Project by PVS-Studio.
