Ошибка настолько проста, что программисты её не замечают

12 Сен 2023

Автор: Андрей Карпов

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

Пользователи время от времени присылают нам различные фрагменты C++ кода, на которые анализатор PVS-Studio, по их мнению, выдал странные/ложные предупреждения. После чего мы дорабатываем диагностики или выписываем идеи по доработкам на потом. В общем, идёт обыкновенная работа по поддержке пользователей.

Однако в этот раз программист, который получил письмо от пользователя, прибежал возбуждённый ко мне с идеей, что про этот случай стоит что-то написать в блог. Мы не можем назвать пользователя или показать его код. Поэтому вот переписанный сокращённый вариант:

char *SomeGlobalPointer {};

void foo()
{
  SomeGlobalPointer = new char[1024];
}

int main()
{
  foo();

  if (!SomeGlobalPointer)
  {
    delete[] SomeGlobalPointer;
  }

  return 0;
}

Программист со стороны пользователя жалуется на "ложное" срабатывание PVS-Studio:

V575 The null pointer is passed into 'operator delete[]'. Inspect the argument.

Какой же это нулевой указатель? Вон же он явно инициализируется в функции.

Это интересный пример, когда код выглядит настолько простым, что человек не видит ошибку. Код для освобождения памяти скучен, и в него сложно вчитаться на обзоре кода. Я уже описывал подобный эффект, например, в статье "Зло живёт в функциях сравнения".

На самом деле перед нами неприметная опечатка в условии:

if (!SomeGlobalPointer)
{
  delete[] SomeGlobalPointer;
}

Оператор delete[] вызывается только в том случае, если указатель нулевой. В результате возникает утечка памяти. Естественно, должно быть наоборот:

if (SomeGlobalPointer)
{
  delete[] SomeGlobalPointer;
}

На самом деле проверка вообще не нужна. Оператор delete[] корректно обрабатывает нулевые указатели. Код можно упростить:

char *SomeGlobalPointer {};

void foo()
{
  SomeGlobalPointer = new char[1024];
}

int main()
{
  foo();
  delete[] SomeGlobalPointer;
  return 0;
}

Поскольку мы заговорили о рефакторинге, то сюда ещё просится умный указатель (std::unique_ptr или std::shared_ptr в зависимости от ситуации). Конечно, код вообще выглядит странным, но не забывайте, что в статье приведён синтетический вариант. Впрочем, мы отвлеклись.

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

Если бы не PVS-Studio, в коде так и осталась бы эта ошибка, приводящая к утечке памяти.

Любите статический анализ кода! Прокачайте процесс обзора кода, используя дополнительно анализатор кода!

Некоторые другие статьи о том, как статический анализ компенсирует нашу человеческую невнимательность:

#Cpp