Почему PVS-Studio не предлагает автоматические правки кода

19 Ноя 2020

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

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

Иногда программисты, которые начинают пробовать PVS-Studio спрашивают: почему инструмент не предлагает автоматически исправить ошибку? Что интересно, пользователи такой вопрос уже не задают. После некоторого времени использования анализатора, им становится понятно, что для подавляющего большинства обнаруживаемых ошибок никакая автоматическая замена невозможна. По крайней мере, пока не изобретут искусственный интеллект :).

Причина в том, что PVS-Studio не является анализатором стиля кода. Он не предлагает изменения, связанные с форматированием или именованием. Не предлагает он (по крайней мере на момент написания статьи :) заменить в C++ коде все NULL на nullptr. Это хоть и хорошее предложение, но оно не имеет практически ничего общего с поиском и устранением ошибок.

PVS-Studio выявляет ошибки и потенциальные уязвимости. Многие ошибки заставляют задуматься и требуют изменения поведения программы. И только программист может решить, как исправить ту или иную ошибку.

Анализатор, обнаружив ошибку, скорее всего, предложит упростить код, чтобы аномалия исчезла, но это не исправит саму ошибку. Понять же, что на самом деле должен делать код и предложить осмысленное полезное исправление, очень сложно.

Рассмотрим ошибку, которую я разбирал в статье "31 февраля".

static const int kDaysInMonth[13] = {
  0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
};

bool ValidateDateTime(const DateTime& time) {
  if (time.year < 1 || time.year > 9999 ||
      time.month < 1 || time.month > 12 ||
      time.day < 1 || time.day > 31 ||
      time.hour < 0 || time.hour > 23 ||
      time.minute < 0 || time.minute > 59 ||
      time.second < 0 || time.second > 59) {
    return false;
  }
  if (time.month == 2 && IsLeapYear(time.year)) {
    return time.month <= kDaysInMonth[time.month] + 1;
  } else {
    return time.month <= kDaysInMonth[time.month];
  }
}

Анализатор понимает, что обе проверки являются истинными. Но почему, анализатору непонятно. Он ничего не знает про дни, месяцы и прочие сущности. И научить понимать такое ой как непросто. Единственное, что реально сделать - это чтобы анализатор предлагал упростить функцию:

bool ValidateDateTime(const DateTime& time) {
  if (time.year < 1 || time.year > 9999 ||
      time.month < 1 || time.month > 12 ||
      time.day < 1 || time.day > 31 ||
      time.hour < 0 || time.hour > 23 ||
      time.minute < 0 || time.minute > 59 ||
      time.second < 0 || time.second > 59) {
    return false;
  }
  if (time.month == 2 && IsLeapYear(time.year)) {
    return true;
  } else {
    return true;
  }
}

Или, чего уж мелочиться, пусть он предложит такую автоматическую замену:

bool ValidateDateTime(const DateTime& time) {
  if (time.year < 1 || time.year > 9999 ||
      time.month < 1 || time.month > 12 ||
      time.day < 1 || time.day > 31 ||
      time.hour < 0 || time.hour > 23 ||
      time.minute < 0 || time.minute > 59 ||
      time.second < 0 || time.second > 59) {
    return false;
  }
  return true;
}

Прикольно, но бессмысленно ;). Анализатор убрал код, который с точки зрения языка C++ является лишним. И только человек может понять, является код действительно избыточным (а такое тоже часто бывает), или в коде допущена опечатка и надо заменить month на day.

Читатель может сказать, что я сгущаю краски и автоматическая замена вполне уместна. Нет. В подобном ошибаются люди, чего уж хотеть от бездушной программы. Вот смотрите, есть интересный пример ручной невнимательной правки, которая на самом деле ничего не исправляет. Раз не может человек, не сможет и программа.

В августе этого вирусного года я написал статью о проверки библиотеки PMDK. Среди прочего в статье рассматривалась ошибка неправильной защиты от переполнения:

static DWORD
get_rel_wait(const struct timespec *abstime)
{
  struct __timeb64 t;
  _ftime64_s(&t);
  time_t now_ms = t.time * 1000 + t.millitm;
  time_t ms = (time_t)(abstime->tv_sec * 1000 +
    abstime->tv_nsec / 1000000);

  DWORD rel_wait = (DWORD)(ms - now_ms);

  return rel_wait < 0 ? 0 : rel_wait;
}

Раз переменная rel_wait имеет беззнаковый тип, то последующая проверка rel_wait < 0 не имеет смысла. Предупреждение PVS-Studio: V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359

Кто-то воодушевился статьёй и начал массово исправлять описанные в ней ошибки: Fix various issues reported by PVS-Studio analysis.

И как же было предложено исправить код? Весьма бесхитростно: core: simplify windows timer implementation.

Но код был упрощен, а не исправлен! Это заметили и началась соответствующая дискуссия: ISSUE: os_thread_windows.c - get_rel_wait() will block if abstime is in the past.

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

Да и вообще, желание автоматической правки ошибок - это очень странное желание. Каждая правка, исправляющая баг, требует внимания и обзора кода. Более того, анализатор может выдавать ложные срабатывания, а значит править такой код вообще нельзя. Анализ кода и работа с предупреждениями - это не то место, где надо спешить. Лучше внедрять регулярный анализ кода и потихоньку исправлять ошибки, появляющиеся в новом коде.

#Cpp #StaticAnalysis