Перевіряємо вихідний код 7-Zip з допомогою PVS-Studio

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





Введення
Пара слів про проект. 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 error closing";
}
.... 
}

Покажчик 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 testing in 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 testing in 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 testing in 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 testing in 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/


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Kirill Yudintsev. Checking 7-Zip with PVS-Studio analyzer.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.