В статьях мы регулярно повторяем важную мысль: статический анализатор должен использоваться регулярно. В этом случае многие ошибки выявляются на самом раннем этапе, а их исправление максимально дёшево. Однако теория – это одно, но намного лучше подкреплять слова практическими примерами. Рассмотрим несколько свежих ошибок, появившихся в новом коде проекта Blender.

Недавно мы настроили регулярную проверку проекта Blender, о чём мой коллега рассказал в статье "Just for fun: команда PVS-Studio придумала мониторить качество некоторых открытых проектов". В дальнейшем планируем начать мониторить ещё некоторые интересные проекты.

Сразу скажу, что мы не ставим перед собой задачу найти как можно больше ошибок. Целью является периодическое написание небольших заметок (таких как эта), в которых мы будем на практике показывать достоинства регулярного анализа кода. Другими словами, мы иногда будем описывать некоторые интересные ошибки в новом коде, найденные при очередном ночном запуске PVS-Studio, и тем самым популяризировать правильное использование методологии статического анализа кода.

Итак, давайте посмотрим, что найдено в свежем коде проекта Blender.

Фрагмент первый: double-checked locking

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: Potentially unsafe double-checked locking. node_ui_storage.cc 46

Перед нами неправильная реализация блокировки с двойной проверкой. Для пояснения проблемы процитирую фрагмент статьи "C++ and the Perils of Double-Checked Locking", написанной Scott Meyers и Andrei Alexandrescu ещё в 2004 году. Как видите, проблема давно известна, но это не защищает разработчиков от того, чтобы наступать на одни и те же грабли. Хорошо, что анализатор PVS-Studio помогает выявлять подобные проблемы :). Итак, фрагмент из статьи:

Consider again the line that initializes pInstance: pInstance = newSingleton;

This statement causes three things to happen:

Step 1: Allocate memory to hold a Singleton object.

Step 2: Construct a Singleton object in the allocated memory.

Step 3: Make pInstance point to the allocated memory.

Of critical importance is the observation that compilers are not constrained to perform these steps in this order! In particular, compilers are sometimes allowed to swap steps 2 and 3. Why they might want to do that is a question we'll address in a moment. For now, let's focus on what happens if they do.

Consider the following code, where we've expanded pInstance's initialization line into the three constituent tasks we mentioned above and where we've merged steps 1 (memory allocation) and 3 (pInstance assignment) into a single statement that precedes step 2 (Singleton construction). The idea is not that a human would write this code. Rather, it's that a compiler might generate code equivalent to this in response to the conventional DCLP source code (shown earlier) that a human would write.

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

Подобные ошибки очень коварны! Они могут очень редко проявлять себя. Программа вроде как работает, проходит все тесты и так далее. Но время от времени она неожиданно падает у пользователей, и понять причину может быть крайне проблематично. Воспроизвести такую ошибку может быть очень сложно. Другим словами, исправление этой ошибки по жалобе от пользователей может обойтись в 1000 раз дороже, чем правка кода, сделанная по результату анализа кода с помощью PVS-Studio или аналогичного инструмента.

Примечание 1. Ошибка может и не существовать сейчас в двоичном коде, и всё зависит от компилятора, и ключей оптимизации. Однако, даже если сейчас всё работает хорошо, это не значит, что так будет всегда. Ошибка может проявить себя при смене компилятора/ключей оптимизации.

Примечание 2. Читатели обратили внимание, что описание проблемы блокировки с двойной проверкой устарело. В C++17 язык гарантированно выполняет все побочные эффекты, связанные с выражением new T перед выполнением эффектов самого присваивания (operator =). Другими словами, начиная с C++17, эту часть можно назвать "уже пофиксили, это не баг". Однако, запись всё равно не является атомарной, и возможно возникновение состояния гонки. Чтобы этого избежать, указатель должен быть объявлен как атомарный: std::atomic<NodeTreeUIStorage *> ui_storage\.

Фрагмент второй: realloc

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() possible leak: when realloc() fails in allocating memory, original pointer 'context->read_icons' is lost. Consider assigning realloc() to a temporary pointer. datatoc_icon.c 252

Если память не удастся выделить, функция realloc вернёт значение NULL. Нулевой указатель будет записан в переменную context->read_icons, а её предыдущее значение будет потеряно. Раз предыдущее значение указателя потеряно, то и невозможно освободить ранее выделенный блок памяти, на который ссылался этот указатель. Произойдёт утечка памяти.

Вторая: V522: There might be dereferencing of a potential null pointer 'context->read_icons'. Check lines: 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: The 'nldrag' pointer was utilized before it was verified against nullptr. Check lines: 1037, 1039. node_relationships.c

Один из самых частых паттернов ошибок (proof). В начале указатель nldrag разыменовывается. Но из следующего условного оператора становится видно, что на самом деле этот указатель может быть нулевым.

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

Кстати, нашлась ещё одна такая-же ошибка, но описывать её неинтересно. Приведу только сообщение: V595: The 'seq' pointer was utilized before it was verified against nullptr. Check lines: 373, 385. strip_add.c

Заключение

Используйте статические анализаторы кода регулярно. От этого выиграют как разработчики, так и пользователи. Вы можете скачать и попробовать PVS-Studio здесь. Спасибо за внимание.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio, Blender: Series of Notes on Advantages of Regular Static Analysis of Code.