Проверяем исходный код 7-Zip с помощью PVS-Studio06.06.2016 16:33

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

feb328b1f6984c91b71b711b481a9c9d.png

Введение


Пара слов о проекте. 7-Zip — свободный файловый архиватор с высокой степенью сжатия данных, написанный на языках C и C++. Он имеет небольшой размер в 235 тысяч строк кода. Поддерживает несколько алгоритмов сжатия и множество форматов данных, включая собственный формат 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 There are identical sub-expressions 'Id == k_PPC' to the left and to the right of the '||' operator. 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 There are identical sub-expressions to the left and to the right of the '||' operator: offs >= nodeSize || offs >= nodeSize 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 The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 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");
  ....
}


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

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 Expression 'newSize < 0' is always false. Unsigned type value is never < 0. update.cpp 254

Так выглядит пример кода из программы в котором эта особенность языка была проигнорирована: 

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


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

  • V547 Expression 'rec.SiAttr.SecurityId >= 0' is always true. Unsigned type value is always >= 0. ntfshandler.cpp 2142
  • V547 Expression 's.Len () >= 0' is always true. Unsigned type value is always >= 0. xarhandler.cpp 258


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


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

V571 Recurring check. The 'if (Result!= ((HRESULT) 0L))' condition was already verified in line 56. extractengine.cpp 58

Так выглядит фрагмент кода программы: 

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


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

Ещё похожие места в коде 7-Zip:

  • V571 Recurring check. The '! quoteMode' condition was already verified in line 18. stringutils.cpp 20
  • V571 Recurring check. The 'IsVarStr (params[1], 22)' condition was already verified in line 3377. nsisin.cpp 3381


Подозрительная работа с указателями


Встречается в коде 7-Zip и ошибка, когда указатель в начале разыменовывается, а только потом проверяется на равенство нулю.

V595 The 'outStreamSpec' pointer was utilized before it was verified against nullptr. Check lines: 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 The '_file' pointer was utilized before it was verified against nullptr. Check lines: 2099, 2112. bench.cpp 2099
  • V595 The 'ai' pointer was utilized before it was verified against nullptr. Check lines: 204, 214. updatepair.cpp 204
  • V595 The 'options' pointer was utilized before it was verified against nullptr. Check lines: 631, 636. zipupdate.cpp 631
  • V595 The 'volStreamSpec' pointer was utilized before it was verified against nullptr. Check lines: 856, 863. update.cpp 856


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


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

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

V509 The 'throw' operator inside the destructor should be placed within the try…catch block. Raising exception inside the destructor is illegal. 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 она помечена как deprecated и не рекомендуется к использованию. В готовящемся стандарте С++17 возможность использования инкремента для булевой переменной помечена для удаления.

В коде 7-Zip было найдено пару мест, где используется эта устаревшая возможность.

  • V552 A bool type variable is being incremented: numMethods ++. Perhaps another variable should be incremented instead. wimhandler.cpp 308
  • V552 A bool type variable is being incremented: numMethods ++. Perhaps another variable should be incremented instead. 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 There is no sense in testing the 'plugin' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. 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 (). Таким образом проверять на равенство нулю не имеет смысла. Указатель plugin никогда не будет равен нулю. Функция никогда не вернет константное значение INVALID_HANDLE_VALUE. Если выделить память невозможно, то возникает исключение, которое лучше обрабатывать на более высоком уровне, а проверку на равенство нулю можно просто удалить. Ну или если исключения в приложении нежелательны, то можно использовать оператор new не генерирующий исключений, в этом случае можно проверять возвращаемое значение на ноль. В коде приложения встретилось еще 3 подобных проверки:

  • V668 There is no sense in testing the 'm_Formats' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. enumformatetc.cpp 46
  • V668 There is no sense in testing the 'm_States' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. bzip2decoder.cpp 445
  • V668 There is no sense in testing the 'ThreadsInfo' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. bzip2encoder.cpp 170


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


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

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

V801 Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const… pathParts' with 'const… &pathParts'. wildcard.cpp 487

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


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

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


Вот еще некоторые похожие места:

  • V801 Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const… props' with 'const… &props'. benchmarkdialog.cpp 766
  • V801 Instantiate CRecordVector < CAttribIconPair >: Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const… item' with 'const… &item'. yvector.h 199


Заключение


7-Zip — это небольшой проект, развивающийся уже достаточно давно и найти большого количества серьезных ошибок конечно не удалось. Но в коде все же есть места, на которые нужно обратить внимание и статический анализатор кода PVS-Studio существенно облегчит эту работу. Если вы разрабатываете проект на C, C++ или C#, предлагаю не откладывая скачать PVS-Studio и проверить свой проект: http://www.viva64.com/ru/pvs-studio-download/

8d241b5bf34747169141ed7c1997143b.png


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Kirill Yudintsev. Checking 7-Zip with PVS-Studio analyzer.

© Habrahabr.ru