Проверка 7-Zip с помощью анализатора PVS-Studio

Одной из программ, которая позволяет решить проблему сжатия данных, является популярный файловый архиватор 7-Zip, которым я сам часто пользуюсь. Наши читатели давно просили нас проверить код этого приложения. Что ж, пора взглянуть на его исходный код и посмотреть, что PVS-Studio умеет обнаруживать в этом приложении.

Введение

Пара слов о проекте. 7-Zip — бесплатный файловый архиватор с высокой степенью сжатия данных, написанный на языках C и C++. Размер этого проекта составляет 235 000 строк кода. Он поддерживает несколько алгоритмов сжатия и различные форматы данных, включая собственный формат 7z, с высокоэффективным алгоритмом сжатия LZMA. Он находится в разработке с 1999 года, бесплатный и с открытым исходным кодом. 7-Zip — победитель конкурса SourceForge.net Community Choice Awards 2007 года в категориях Лучший проект и Лучший технический дизайн. Мы проверили версию 16.00, исходный код которой можно скачать по этой ссылке — http://www.7-zip.org/download.html

Результаты анализа

Для анализа 7-Zip использовался статический анализатор кода PVS-Studio v6.04. В этой статье мы приводим самые интересные предупреждения анализатора. Давайте посмотрим на них.

Опечатки в условных операторах

Мы довольно часто видим опечатки в условных операторах. Они могут причинить много боли, если будет большое количество проверок. Тогда нам на помощь приходит статический анализатор.

Вот несколько примеров этой ошибки.

V501 Слева и справа от оператора || одинаковые подвыражения Id == k_PPC. 7zupdate.cpp 41

void SetDelta()
{
  if (Id == k_IA64)
    Delta = 16;
  else if (Id == k_ARM || Id == k_PPC || Id == k_PPC)    //<==
    Delta = 4;
  else if (Id == k_ARMT)
    Delta = 2;
  else
    Delta = 0;
}

Анализатор обнаружил похожие условные выражения. В лучшем случае одно из условий для Id == k_PPC является избыточным и не влияет на логику программы. Чтобы исправить эту опечатку, нам нужно просто убрать это условие, тогда правильное выражение будет таким:

if (Id == k_IA64)
  Delta = 16;
else if (Id == k_ARM || Id == k_PPC)
  Delta = 4;

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

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

V501 Слева и справа от оператора «||» находятся идентичные подвыражения: offs ›= nodeSize || offs ›= размер узла hfshandler.cpp 915

HRESULT CDatabase::LoadCatalog(....)
{
  ....
  UInt32 nodeSize = (1 << hr.NodeSizeLog);
  UInt32 offs = Get16(p + nodeOffset + nodeSize - (i + 1) * 2);
  UInt32 offsNext = Get16(p + nodeOffset + nodeSize - (i + 2) * 2);
  UInt32 recSize = offsNext - offs;
  if (offs >= nodeSize
           || offs >= nodeSize    //<==
           || offsNext < offs
           || recSize < 6)
    return S_FALSE;
  ....
}

Проблема в повторяющемся условии offs ›= nodeSize.

Опечатки, скорее всего, появились из-за использования Copy-Paste для дублирования кода. Не имеет смысла рекомендовать не использовать метод копирования-вставки. Слишком удобно и полезно отказываться от такого функционала в редакторе. Надо просто более тщательно проверить полученный результат.

Идентичные сравнения

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

V517 Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверьте строки: 388, 390. archivecommandline.cpp 388

static void AddRenamePair(...., NRecursedType::EEnum type, ....)
{
  ....
  if (type == NRecursedType::kRecursed)
    val.AddAscii("-r");
  else if (type == NRecursedType::kRecursed)    //<==
    val.AddAscii("-r0");
  ....
}

NRecusedType определяется в коде следующим образом:

namespace NRecursedType { 
  enum EEnum {
    kRecursed,
    kWildcardOnlyRecursed,
    kNonRecursed
  };
}

В результате второе условие никогда не будет выполнено. Попробуем разобраться в этой проблеме подробнее. Судя по описанию параметров командной строки, параметр -r сигнализирует об использовании рекурсии для подкаталогов. Но в случае с параметром -r0 рекурсия используется только для имен шаблонов. Сравнивая это с определением NRecursedType, можно сделать вывод, что во втором случае следует использовать тип NRecursedType::kWildcardOnlyRecursed. Тогда правильный код будет таким:

static void AddRenamePair(...., NRecursedType::EEnum type, ....)
{
  ....
  if (type == NRecursedType::kRecursed)
    val.AddAscii("-r");
  else if (type == NRecursedType::kWildcardOnlyRecursed)    //<==
    val.AddAscii("-r0");
  ....
}

Условия, которые всегда либо истинны, либо ложны

Всегда нужно учитывать тип переменной — знаковая она или беззнаковая. Игнорирование этих особенностей может привести к неприятным последствиям.

V547 Выражение newSize ‹ 0 всегда ложно. Значение беззнакового типа никогда не равно 0. update.cpp 254

Вот пример того, как эта языковая функция была проигнорирована:

STDMETHODIMP COutMultiVolStream::SetSize(UInt64 newSize)
{
  if (newSize < 0)    //<==
    return E_INVALIDARG;
  ....
}

Дело в том, что newSize имеет беззнаковый тип, и условие никогда не будет истинным. Если в функцию SetSize попадает отрицательное значение, эта ошибка будет проигнорирована и функция начнет использовать неверный размер. В 7-Zip было еще два условия, которые всегда либо истинны, либо ложны из-за путаницы с signed/unsigned типами.

• V547 Выражение ‘rec.SiAttr.SecurityId ›= 0’ всегда верно. Значение беззнакового типа всегда ›= 0. ntfshandler.cpp 2142
• V547 Выражение ‘s.Len() ›= 0’ всегда верно. Значение беззнакового типа всегда ›= 0. xarhandler.cpp 258

Одно и то же условие проверяется дважды

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

V571 Периодическая проверка. Условие if (Result != ((HRESULT) 0L)) уже проверено в строке 56. extractengine.cpp 58

Вот фрагмент кода:

void Process2()
{
  ....
  if (Result != S_OK)
  {
    if (Result != S_OK)    //<==
      ErrorMessage = kCantOpenArchive;
    return;
  }
  ....
}

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

Еще один аналогичный фрагмент в 7-Zip code:

• V571 Периодическая проверка. Условие «!quoteMode» уже было проверено в строке 18. stringutils.cpp 20
• V571 Периодическая проверка. Условие IsVarStr(params[1], 22) уже проверено в строке 3377. nsisin.cpp 3381

Обработка подозрительных указателей

Были такие баги в 7-Zip code, когда указатель сначала разыменовывается, а уже потом проверяется на null.

V595 Указатель outStreamSpec использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 753, 755. lzmaalone.cpp 753

Это очень распространенная ошибка во всех программах. Обычно он появляется из-за небрежности в процессе рефакторинга. Доступ с помощью нулевого указателя приведет к неопределенному поведению. Рассмотрим фрагмент кода приложения, содержащего ошибку такого типа:

static int main2(int numArgs, const char *args[])
{
  ....
  if (!stdOutMode)
    Print_Size("Output size: ", outStreamSpec->ProcessedSize);   //<==
  if (outStreamSpec)    //<==
  {
    if (outStreamSpec->Close() != S_OK)
      throw "File closing error";
  }
  .... 
}

Указатель outStreamSpec разыменовывается в выраженииoutStreamSpec-›ProcessedSize. Затем он сверяется с нулевым значением. Проверка ниже в коде либо бессмысленна, либо мы должны сверить указатель в коде выше с нулевым значением. Вот список потенциально глючных фрагментов в коде программы:

• V595 Указатель «_file» использовался до того, как он был проверен на соответствие nullptr. Контрольные строки: 2099, 2112. Bench.cpp 2099
• V595 Указатель «ai» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 204, 214. updatepair.cpp 204
• V595 Указатель «options» использовался до того, как он был проверен на соответствие nullptr. Проверьте строки: 631, 636. zipupdate.cpp 631
• V595 Указатель volStreamSpec использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 856, 863. update.cpp 856

Исключение внутри деструктора

Когда в программе генерируется исключение, стек раскручивается, а объекты уничтожаются путем вызова деструкторов. Если деструктор объекта, уничтожаемого во время сворачивания стека, выдает другое исключение, которое покидает деструктор, библиотека C++ немедленно завершает работу программы, вызывая функцию terminate(). Поэтому деструкторы никогда не должны генерировать исключения. Исключение, созданное внутри деструктора, должно обрабатываться внутри того же деструктора.

Анализатор выдал следующее сообщение:

V509 Оператор throw внутри деструктора должен быть помещен в блок try..catch. Вызов исключения внутри деструктора является незаконным. consoleclose.cpp 62

Вот деструктор, который выдает исключение:

CCtrlHandlerSetter::~CCtrlHandlerSetter()
{
  #if !defined(UNDER_CE) && defined(_WIN32)
  if (!SetConsoleCtrlHandler(HandlerRoutine, FALSE))
    throw "SetConsoleCtrlHandler fails";    //<==
  #endif
}

Сообщение V509 предупреждает, что если объект CCtrlHandlerSetter будет уничтожен во время обработки обработки исключения, новое исключение вызовет немедленный сбой программы. Этот код должен быть написан таким образом, чтобы сообщать об ошибке в деструкторе без использования механизма исключений. Если ошибка не критична, то ее можно игнорировать.

CCtrlHandlerSetter::~CCtrlHandlerSetter()
{
  #if !defined(UNDER_CE) && defined(_WIN32)
  try
  {
    if (!SetConsoleCtrlHandler(HandlerRoutine, FALSE))
      throw "SetConsoleCtrlHandler fails";    //<==
  }
  catch(...)
  {
    assert(false);
  }
  #endif
}

Увеличение переменной типа bool

Исторически операция приращения возможна для переменной типа bool; операция устанавливает значение переменной в true. Эта особенность связана с тем, что ранее для представления логических переменных использовались целые значения. Позже эта функция осталась для поддержки обратной совместимости. Начиная со стандарта C++98, он помечен как устаревший и не рекомендуется для использования. В будущем стандарте C++17 эта возможность использовать приращение для логического значения помечена для удаления.

Мы нашли пару фрагментов, где до сих пор используется эта устаревшая функция.

• V552 Увеличивается переменная типа bool: numMethods++. Возможно, вместо этого следует увеличить другую переменную. wimhandler.cpp 308
• V552 Увеличивается переменная типа bool: numMethods++. Возможно, вместо этого следует увеличить другую переменную. wimhandler.cpp 318

STDMETHODIMP CHandler::GetArchiveProperty(....)
{
  ....
  bool numMethods = 0;
  for (unsigned i = 0; i < ARRAY_SIZE(k_Methods); i++)
  {
    if (methodMask & ((UInt32)1 << i))
    {
      res.Add_Space_if_NotEmpty();
      res += k_Methods[i];
      numMethods++;    //<==
    }
  }
  if (methodUnknown != 0)
  {
    char temp[32];
    ConvertUInt32ToString(methodUnknown, temp);
    res.Add_Space_if_NotEmpty();
    res += temp;
    numMethods++;    //<==
  }
  if (numMethods == 1 && chunkSizeBits != 0)
  {
    ....
  }
  ....
}

В этой ситуации возможны два варианта. Либо numMethods является флагом, и в этом случае лучше использовать инициализацию логическим значением numMethods = true. Или, судя по переменной, это счетчик, который должен быть целым числом.

Проверка неправильного выделения памяти

Анализатор обнаружил ситуацию, когда значение указателя, возвращаемое оператором new, сравнивается с нулем. Обычно это означает, что программа не будет вести себя так, как ожидает программист, в случае невозможности выделения памяти.

V668 Проверять указатель плагина на null нет смысла, так как память была выделена с помощью оператора новый. Исключение будет сгенерировано в случае ошибки выделения памяти. far.cpp 399

Вот как это выглядит в коде:

static HANDLE MyOpenFilePluginW(const wchar_t *name)
{
  ....
  CPlugin *plugin = new CPlugin(
    fullName,
    // defaultName,
    agent,
    (const wchar_t *)archiveType
    );
    if (!plugin)
      return INVALID_HANDLE_VALUE;
    ....
  }

Если оператору new не удалось выделить память, то в соответствии со стандартом C++ генерируется исключениеstd::bad_alloc(). Тогда проверка на null бессмысленна. Указатель плагина никогда не будет нулевым. Функция никогда не вернет постоянное значение INVALID_HANDLE_VALUE. Если невозможно выделить память, то возникает исключение, которое нужно обрабатывать на более высоком уровне, а проверку на null можно удалить. В случае, если нежелательно иметь исключения в приложении, мы можем использовать оператор new, который не генерирует исключений, и, таким образом, возвращаемое значение может быть проверено на нулевое значение. Таких проверок было еще три:

• V668 Проверять указатель m_Formats на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. enumformatetc.cpp 46
• V668 Проверять указатель m_States на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. bzip2decoder.cpp 445
• V668 Проверять указатель ThreadsInfo на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. bzip2encoder.cpp 170

Конструкции, требующие оптимизации

Теперь давайте поговорим о некоторых спотах, которые потенциально можно оптимизировать. Объект передается в функцию. Этот объект передается по значению, но не изменяется из-за ключевого слова const. Возможно, было бы разумно передать его константной ссылкой в ​​языке C++ или с помощью указателя в языке C.

Вот пример для вектора:

V801 Снижение производительности. Лучше переопределить первый аргумент функции как ссылку. Попробуйте заменить const .. pathParts на const .. &pathParts. wildcard.cpp 487

static unsigned GetNumPrefixParts(const UStringVector pathParts)
{
  ....
}

Во время вызова этой функции у нас будет вызов конструктора копирования для класса UStringVector. Это может значительно снизить производительность приложения, если такое копирование объектов происходит довольно часто. Этот код можно легко оптимизировать, добавив ссылку:

static unsigned GetNumPrefixParts(const UStringVector& pathParts)
{
  ....
}

Вот еще похожие фрагменты:

• V801 Снижение производительности. Лучше переопределить первый аргумент функции как ссылку. Попробуйте заменить «const .. props» на «const .. &props». тестдиалог.cpp 766
• V801 Instantiate CRecordVector ‹ CAttribIconPair ›: снижение производительности. Лучше переопределить первый аргумент функции как ссылку. Попробуйте заменить «const .. item» на «const .. &item». мойвектор.h 199

Вывод

7-Zip — небольшой проект, который развивался довольно долго, поэтому шансов найти большое количество серьезных багов было немного. Но все же есть некоторые фрагменты, на которые стоит обратить внимание, и статический анализатор кода PVS-Studio может помочь. Если вы разрабатываете проект на C, C++ или C#, предлагаю скачать PVS-Studio и проверить свой проект.

Кирилл Юдинцев