PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#.

PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.

Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.

Проверяем исходный код 7-Zip с помощью PVS-Studio

Запись от el_programmer размещена 14.06.2016 в 10:45

Показов 3965 Комментарии 1

Метки 7zip, bugs, c++, cpp, open source, programming

Автор: Кирилл Юдинцев

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

Нажмите на изображение для увеличения
Название: image1.png
Просмотров: 598
Размер: 52.9 Кб
ID: 3879

Введение

Пара слов о проекте. 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

C++
1
2
3
4
5
6
7
8
9
10
11
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является лишним и не влияет на логику работы программы. Для исправления опечатки необходимо просто убрать это условие, тогда правильное выражение будет иметь следующий вид:

C++
1
2
3
4
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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
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

C++
1
2
3
4
5
6
7
8
9
static void AddRenamePair(...., NRecursedType::EEnum type, ....)
{
  ....
  if (type == NRecursedType::kRecursed)
    val.AddAscii("-r");
  else if (type == NRecursedType::kRecursed)    //<==
    val.AddAscii("-r0");
  ....
}

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

C++
1
2
3
4
5
6
7
namespace NRecursedType { 
  enum EEnum {
    kRecursed,
    kWildcardOnlyRecursed,
    kNonRecursed
  };
}

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

C++
1
2
3
4
5
6
7
8
9
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

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

C++
1
2
3
4
5
6
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

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

C++
1
2
3
4
5
6
7
8
9
10
11
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.

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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
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

А вот, как выглядит деструктор генерирующий исключение:

C++
1
2
3
4
5
6
7
CCtrlHandlerSetter::~CCtrlHandlerSetter()
{
  #if !defined(UNDER_CE) && defined(_WIN32)
  if (!SetConsoleCtrlHandler(HandlerRoutine, FALSE))
    throw "SetConsoleCtrlHandler fails";    //<==
  #endif
}

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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
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

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

C++
1
2
3
4
5
6
7
8
9
10
11
12
13
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

C++
1
2
3
4
static unsigned GetNumPrefixParts(const UStringVector pathParts)
{
  ....
}

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

C++
1
2
3
4
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 существенно облегчит эту работу.

Метки 7zip, bugs, c++, cpp, open source, programming

Размещено в Без категории

« Опечатки в Miranda IM Главная страница Занимательный C# »

Надоела реклама? Зарегистрируйтесь и она исчезнет полностью.

Всего комментариев 1

Комментарии

	Я совсем никудышный программист. Скачиваю 45.9 Мб из-за любопытства. У меня установлена VSC-2013. Потом напишу о впечатлениях.
	Запись от Infinity20_5 размещена 14.06.2016 в 13:54

Новые блоги и статьи Все статьи Все блоги /
Запрет дублирования строк в табличной части Maks 13.09.2026 Реализация из решения ниже выполнена на нетиповом справочнике "Нормы ТО" с табличной часть "Виды ТО", разработанного в КА2, со следующими реквизитами: - ВидТО (СправочникСсылка. ВидыТО); - ВидГСМ. . .	Скрипты Tampermonkey для CyberForum, ChatGPT, Claude и пр. Jin X 06.09.2026 Скрипты Tampermonkey для CyberForum, ChatGPT, Claude и пр. Работая с форумом и нейросетями в браузере часто хочется что-то подкорректировать или добавить какого-то функционала. Ниже прикреплён. . .	Программа опроса у.з. расходомера SLS-720F Argus19 02.09.2026 Программа опроса у. з. расходомера SLS-720F Программа опрашивает один раз в минуту три ультразвуковых расходомера SLS-720F через интерфейс RS-485 по протоколу Modbus RTU. Опрашиваются регистры. . .	Hyper-V: Компьютер должен поддерживать доверенный платформенный модуль 2.0. Maks 31.08.2026 При установке Windows 11 на виртуальную машину Hyper-V 2-го поколения вылезла такая ошибка: Решение: в параметрах виртуальной машины, в разделе "Безопасность" (Security) активировать флаг. . .
Архитектура биовида Стива в Майнкрафте: Зачем бонобо кубический каннибализм anaschu 30.08.2026 Кубический Вагинокапитализм в Minecraft: Математический инвариант ОДУ и рок Стивов-бонобо Главная задача разработанной «Модели Всего» — наглядно продемонстрировать наличие системной «судьбы». . .	Оттачиваю умение писать js программы. russiannick 30.08.2026 Проектом выходного дня стало написание Книги шифров Виженера. Итогом стала версия 200, синий туман. Синий туман назван так, потому что замораживает текст под собой. Нажатие синих кнопок управляют. . .	мат медиц модель 30. презентация проекта anaschu 27.08.2026 хоп хоп хоп хидахоп, а я кладую))	Как у меня протекала болезнь zorxor 27.08.2026 Здравствуйте, друзья! Эта запись блога предназначена именно для вас - для моих дорогих друзей, которые знали меня лично. Чтобы ответить на вопрос - а что же со мной произошло на самом деле? Я учился. . .

Сообщение форума

Отменить изменения