Ошибки, которые не находит статический анализ кода, потому что он не используется

15 Июл 2019

Автор: Георгий Грибков

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

Идея очень проста. Поищем на GitHub примеры pull requests, в комментариях к которым указано, что это исправление ошибки. И попробуем найти эти ошибки с помощью статического анализатора кода PVS-Studio. Если исправленная ошибка находится анализатором, то это означает, что её можно было бы исправить ещё на этапе написания кода. А чем раньше ошибка исправляется, тем дешевле это обходится.

К сожалению, GitHub подкачал и не позволил сделать большую шикарную статью на эту тему. В самом GitHub тоже есть глюк (или фича), которая не позволяет искать комментарии в pull requests в проектах, написанных только на определённых языках программирования. Ну или я не "умею его готовить". Независимо от того, что я указываю искать комментарии в проектах на языке C++, C# или Java, выдаются результаты по всем языкам, включая PHP, Python, JavaScript и так далее. В результате искать подходящие случаи оказалось крайне утомительным занятием, и я ограничусь только несколькими примерами. Тем не менее, их достаточно, чтобы продемонстрировать полезность инструментов статического анализа кода при его регулярном использовании.

Что, если ошибка была бы отловлена на ранней стадии? Ответ несложен: программистам не понадобилось бы ждать её проявления, а затем искать и исправлять дефектный код.

Посмотрим на ошибки, которые мог бы сразу обнаружить PVS-Studio:

Первый пример взят из проекта SatisfactoryModLoader. До исправления ошибки код выглядел так:

// gets an API function from the mod handler
SML_API PVOID getAPIFunction(std::string name) {
  bool found = false;
  for (Registry reg : modHandler.APIRegistry) {
    if (reg.name == name) {
      found = true;
    }
  }
  if (!found) {
    std::string msg = ...;
    MessageBoxA(NULL, 
                msg.c_str(), 
                "SatisfactoryModLoader Fatal Error", 
                MB_ICONERROR);
    abort();
  }
}

Этот код содержал ошибку, на которую PVS-Studio сразу выдал бы предупреждение:

V591 Non-void function should return a value. ModFunctions.cpp 44

Приведенная выше функция не имеет return, поэтому она будет возвращать формально неопределенное значение. Программист не пользовался анализатором кода, поэтому ему пришлось самостоятельно искать ошибку. Функция после правки:

// gets an API function from the mod handler
SML_API PVOID getAPIFunction(std::string name) {
  bool found = false; 
  PVOID func = NULL;
  for (Registry reg : modHandler.APIRegistry) {
    if (reg.name == name) {
      func = reg.func;
      found = true;
    }
  }
  if (!found) {
    std::string msg = ...;
    MessageBoxA(NULL, 
                msg.c_str(), 
                "SatisfactoryModLoader Fatal Error", 
                MB_ICONERROR);
    abort();
  }
  return func;
}

Любопытно, что в коммите автор указал баг как критический: "fixed critical bug where API functions were not returned".

Во втором коммите из истории проекта mc6809 исправления были внесены в следующий код:

void mc6809dis_direct(
  mc6809dis__t *const dis,
  mc6809__t    *const cpu,
  const char   *const op,
  const bool          b16
)
{
  assert(dis != NULL);
  assert(op != NULL);

  addr.b[MSB] = cpu->dp;
  addr.b[LSB] = (*dis->read)(dis, dis->next++);

  ...

  if (cpu != NULL)
  {
    ...
  }
}

Автор исправил лишь одну строку. Он заменил выражение

addr.b[MSB] = cpu->dp;

на выражение

addr.b[MSB] = cpu != NULL ? cpu->dp : 0;

В старой версии кода не производилось никакой проверки cpu на равенство нулевому указателю. Если вдруг окажется, что в качестве второго аргумента в функцию mc6809dis_direct будет передан нулевой указатель, то в теле функции произойдет его разыменование. Результат плачевен и непредсказуем.

Разыменование нулевого указателя - один из самых частых паттернов, по поводу которых нам говорят: "Это не критическая ошибка. Ну живет она в коде и живет, а если разыменование случится - программа спокойно упадёт и всё". Странно и грустно слышать такое от C++ программистов, но жизнь есть жизнь.

В любом случае, в данном проекте такое разыменование всё-таки превратилось в баг, о чем нам говорит заголовок коммита: "Bug fix---NULL dereference".

Если бы разработчик проекта пользовался PVS-Studio, он бы еще два с половиной месяца назад (именно столько прошло с момента внесения ошибки) мог проверить свой код и обнаружить предупреждение:

V595 The 'cpu' pointer was utilized before it was verified against nullptr. Check lines: 1814, 1821. mc6809dis.c 1814

Таким образом, ошибка была бы устранена еще в момент её появления, что сэкономило бы время и, возможно, нервы разработчика :).

Пример еще одной интересной правки был найден в проекте libmorton.

Исправляемый код:

template<typename morton>
inline bool findFirstSetBitZeroIdx(const morton x, 
                                   unsigned long* firstbit_location)
{
#if _MSC_VER && !_WIN64
  // 32 BIT on 32 BIT
  if (sizeof(morton) <= 4) {
    return _BitScanReverse(firstbit_location, x) != 0;
  }
  // 64 BIT on 32 BIT
  else {
    *firstbit_location = 0;
    if (_BitScanReverse(firstbit_location, (x >> 32))) { // check first part
      firstbit_location += 32;
      return true;
    }
    return _BitScanReverse(firstbit_location, (x & 0xFFFFFFFF)) != 0;
  }
#elif  _MSC_VER && _WIN64
  ....
#elif __GNUC__
  ....
#endif
}

В своей правке программист заменяет выражение "firstbit_location += 32" на "*firstbit_location += 32". Программист ожидал, что число 32 будет прибавляться к значению переменной, к которой привязан указатель firstbit_location, но оно прибавлялось непосредственно к указателю. Измененное значение указателя больше нигде не использовалось, а ожидаемое значение переменной так и оставалось неизмененным.

PVS-Studio выдал бы на этот код предупреждение:

V1001 The 'firstbit_location' variable is assigned but is not used by the end of the function. morton_common.h 22

Казалось бы, что может быть ужасного в измененном, но далее не использованном выражении? Диагностика V1001 явно не выглядит так, будто предназначена для выявления особо опасных багов. Несмотря на это, она обнаружила важную ошибку, которая влияла на логику программы.

Более того, оказалось, что эту ошибку было не так просто найти! Мало того, что она находилась в программе с самого момента создания файла, так она еще и пережила множество правок в соседних строках и просуществовала в проекте аж целых 3(!) года! Все это время логика программы была нарушена, и она работала не совсем так, как ожидали этого разработчики. Если бы они использовали PVS-Studio, то ошибка была бы обнаружена гораздо раньше.

Под конец рассмотрим ещё один красивый пример. Пока я собирал исправления ошибок на GitHub, я несколько раз наткнулся на фикс с вот таким содержанием. Исправленная ошибка находилась здесь:

int kvm_arch_prepare_memory_region(...)
{
  ...
  do {
    struct vm_area_struct *vma = find_vma(current->mm, hva);
    hva_t vm_start, vm_end;
    ...
    if (vma->vm_flags & VM_PFNMAP) {
      ...
      phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) +
        vm_start - vma->vm_start;
      ...
    }
    ...
  } while (hva < reg_end);
  ...
}

На этот участок кода PVS-Studio выдал предупреждение:

V629 Consider inspecting the 'vma->vm_pgoff << 12' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. mmu.c 1795

Я посмотрел объявления переменных, используемых в выражении "phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) + vm_start - vma->vm_start;", и обнаружил, что приведенный выше код аналогичен следующему синтетическому примеру:

void foo(unsigned long a, unsigned long b)
{
  unsigned long long x = (a << 12) + b;
}

Если значение 32-битной переменной a больше, чем 0xFFFFF, то 12 старших битов будут иметь хотя бы одно ненулевое значение. После применения к этой переменной операции сдвига влево эти значимые биты будут потеряны, вследствие чего в x будет записана некорректная информация.

Чтобы устранить потерю старших битов, необходимо сначала привести a к типу unsigned long long, и только после этого производить операцию сдвига:

pa = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT;
pa += vm_start - vma->vm_start;

Тогда в pa всегда будет записываться корректное значение.

Все бы ничего, но этот баг, как и первый пример из статьи, тоже оказался критическим, о чем автор коммита написал отдельно в своем комментарии. Более того, эта ошибка попала ну просто в огромное количество проектов. Чтобы в полной мере оценить масштаб трагедии, предлагаю посмотреть на количество результатов при поиске этого багфикса на GitHub. Страшно, не правда ли?

Итак, я применил новый подход, чтобы продемонстрировать пользу регулярного использования статического анализатора кода. Надеюсь, вам понравилось. Скачайте и попробуйте статический анализатор кода PVS-Studio для проверки собственных проектов. На момент написания статьи в нём реализовано около 700 диагностических правил для обнаружения разнообразнейших паттернов ошибок. Поддерживаются языки C, C++, C# и Java.

#Cpp #StaticAnalysis