В наших статьях мы регулярно повторяем важную мысль: статический анализатор нужно использовать регулярно. Это помогает обнаруживать и дешево исправлять многие ошибки на самой ранней стадии. В теории выглядит красиво. Как известно, действия по-прежнему говорят громче слов. Давайте посмотрим на некоторые недавние ошибки в новом коде проекта Blender.
Недавно мы организовали регулярную проверку проекта Blender, как описал мой коллега в статье Ради интереса: команда PVS-Studio придумала контролировать качество некоторых проектов с открытым исходным кодом. В дальнейшем планируем начать мониторинг еще нескольких интересных проектов.
Сразу скажу, что мы не ставим перед собой задачу найти как можно больше ошибок. Цель состоит в том, чтобы изредка писать небольшие заметки (вроде этой), в которых мы покажем на практике преимущества регулярного анализа кода. Другими словами, мы будем описывать некоторые интересные ошибки в новом коде, обнаруженные во время обычного ночного запуска PVS-Studio, и тем самым способствовать правильному использованию методологии статического анализа кода.
Итак, давайте посмотрим, что мы нашли в последнем коде проекта Blender.
Фрагмент 1: блокировка с двойной проверкой
typedef struct bNodeTree { .... struct NodeTreeUIStorage *ui_storage; } bNodeTree;static void ui_storage_ensure(bNodeTree &ntree) { /* As an optimization, only acquire a lock if the UI storage doesn't exist, * because it only needs to be allocated once for every node tree. */ if (ntree.ui_storage == nullptr) { std::lock_guard<std::mutex> lock(global_ui_storage_mutex); /* Check again-- another thread may have allocated the storage while this one waited. */ if (ntree.ui_storage == nullptr) { ntree.ui_storage = new NodeTreeUIStorage(); } } }
Предупреждение PVS-Studio: V1036: Потенциально небезопасная блокировка с двойной проверкой. node_ui_storage.cc 46
Это неправильная реализация блокировки с двойной проверкой. Для пояснения проблемы приведу отрывок из статьи C++ и опасности двойной проверки блокировки, написанной Скоттом Мейерсом и Андреем Александреску еще в 2004 году. Хотя эта проблема известна давно, некоторые разработчики продолжают стрелять себе в ногу. Хорошо, что анализатор PVS-Studio помогает выявлять такие проблемы :). Фрагмент из статьи:
Вновь рассмотрим строку, которая инициализируетpInstance: pInstance = newSingleton;
Это выражение вызывает три вещи:
Шаг 1. Выделите память для хранения объектаSingleton.
Шаг 2. Создайте объектSingletonв выделенной памяти.
Шаг 3. УкажитеpInstanceна выделенную память.
Очень важно отметить, что компиляторы не обязаны выполнять эти шаги в указанном порядке! В частности, компиляторам иногда разрешается поменять местами шаги 2 и 3. Почему они могут захотеть это сделать — вопрос, который мы рассмотрим чуть позже. А пока давайте сосредоточимся на том, что произойдет, если они это сделают.
Рассмотрите следующий код, в котором мы расширили строку инициализацииpInstanceдо трех составных задач, упомянутых выше, и где мы объединили шаги 1 (выделение памяти) и 3 ( присваивание pInstance) в один оператор, который предшествует шагу 2 (конструкцияSingleton). Идея не в том, что человек напишет этот код. Скорее компилятор может сгенерировать код, эквивалентный этому, в ответ на обычный исходный код DCLP (показан ранее), который написал бы человек.
Если вы хотите узнать больше о написании блокировки с двойной проверкой, рекомендую прочитать описание диагностики и статью. Ссылки были даны выше. Продолжайте читать, чтобы узнать, что для нас имеет большое значение во всей этой инициативе.
Такие ошибки очень коварны! Они очень редко могут проявить себя. Программа вроде работает, проходит все тесты и так далее. Но время от времени он неожиданно дает сбой на стороне пользователей. Понять причину бывает крайне сложно. Воспроизведение такой ошибки может стать тяжелой борьбой. Это означает, что после сообщения от пользователя исправление ошибки может стоить в 1000 раз больше, чем редактирование кода после анализа кода с помощью PVS-Studio или другого подобного инструмента.
Примечание 1. На данный момент бинарный код может не содержать ошибки — все зависит от компилятора и ключей оптимизации. И хотя сейчас все работает хорошо, в будущем все может измениться. Ошибка может проявиться после смены компилятора или ключей оптимизации.
Примечание 2. Наши читатели заметили, что проблема блокировки с двойной проверкой устарела. В C++17 язык выполняет все побочные эффекты, связанные с подвыражением new T, до выполнения побочных эффектов присваивания (оператор ‘=’). Другими словами, начиная с C++17, вы можете считать это исправленным, а не ошибкой. Однако выражение не является атомарным и возможно состояние гонки. Чтобы избежать этого, объявите указатель атомарным: std::atomic‹NodeTreeUIStorage *› ui_storage.
Фрагмент второй: перераспределение
static void icon_merge_context_register_icon(struct IconMergeContext *context,
const char *file_name,
struct IconHead *icon_head)
{
context->read_icons = realloc(context->read_icons,
sizeof(struct IconInfo) * (context->num_read_icons + 1));
struct IconInfo *icon_info = &context->read_icons[context->num_read_icons];
icon_info->head = *icon_head;
icon_info->file_name = strdup(path_basename(file_name));
context->num_read_icons++;
}Здесь анализатор PVS-Studio выдает два предупреждения, что правильно. Действительно, у нас есть две ошибки разных типов.
Во-первых: V701: возможная утечка realloc(): когда realloc() не удается выделить память, исходный указатель context-›read_icons теряется. Рассмотрите возможность назначения realloc() временному указателю. datatoc_icon.c 252
Если память не может быть выделена, функция realloc возвращает NULL. Нулевой указатель будет записан в переменную context-›read_icons, а его предыдущее значение будет потеряно. Так как предыдущее значение указателя потеряно, то невозможно освободить ранее выделенный блок памяти, к которому этот указатель обращался. Произойдет утечка памяти.
Во-вторых: V522: возможно разыменование потенциального нулевого указателя context-›read_icons. Проверить строки: 255, 252. datatoc_icon.c
Описанная выше ошибка, по мнению автора кода, не является действительной ошибкой. Не было намерения писать код, который продолжал бы работать, если бы невозможно было увеличить блок выделенной памяти. Этот случай просто не рассматривается. Автор предполагает, что если бы память не могла быть выделена, то программа просто вылетала бы при разыменовании нулевого указателя. Так что разработчик спокойно работает с указателем, не выполняя его предварительную проверку. Оставим в стороне вопрос о том, насколько красиво такое поведение программы. На мой взгляд, такое поведение библиотек недопустимо.
Здесь интереснее другое. На самом деле сбоя может и не быть. Значение записывается не в нулевой указатель, а куда-то дальше. Теоретически возможно, что этого адреса уже нет в защищенной от записи странице памяти, и сбоя не будет. Некоторые случайные данные в памяти будут испорчены, и программа продолжит выполнение. Последствия работы с поврежденными данными непредсказуемы. Подробнее читайте в статье «Почему важно проверять, что вернула функция malloc».
Фрагмент третий: разыменование указателя перед проверкой
static int node_link_invoke(bContext *C, wmOperator *op, const wmEvent *event)
{
....
bNodeLinkDrag *nldrag = node_link_init(bmain, snode, cursor, detach);
nldrag->last_picked_multi_input_socket_link = NULL;
if (nldrag) {
op->customdata = nldrag;
....
}Предупреждение PVS-Studio: V595: Указатель ‘nldrag’ использовался до того, как был проверен на nullptr. Контрольные строки: 1037, 1039. node_relationships.c
Один из самых распространенных паттернов ошибок (proof). Сначала разыменовывается указатель nldrag. Но из следующего условного оператора становится ясно, что этот указатель может быть нулевым.
Все просто и понятно. Согласитесь, такую ошибку лучше сразу исправлять при написании кода, а не разбираться с ней после того, как ее обнаружит QA-специалист или пользователь.
Кстати, была еще такая ошибка, но не вижу смысла ее описывать. Приведу только сообщение: V595: Указатель seq был использован до проверки на nullptr. Проверить строки: 373, 385. strip_add.c
Заключение
Регулярно используйте статические анализаторы кода. От этого выигрывают как разработчики, так и пользователи. Скачать и попробовать PVS-Studio можно здесь. Спасибо за внимание!
