Єдиноріг, який зміг

The Little Unicorn That CouldОдна з команд розробників Microsoft вже використовує в роботі аналізатор PVS-Studio. Це добре, але недостатньо. Тому я продовжую демонструвати, яку користь може приносити статичний аналіз коду на прикладі проектів Microsoft. Три роки тому ми перевіряли проект Casablanca і не змогли в ньому нічого виявити. За цей проект був відзначений медаллю «безбажный код». Минув час, проект розвивався і ріс. У свою чергу, аналізатор PVS-Studio істотно просунувся в можливості аналізу коду. І нарешті я можу написати статтю про помилки, які аналізатор виявляє в проекті Casablanca (C++ REST SDK). Помилок мало, але те, що тепер їх достатньо для написання статті, говорить про ефективність PVS-Studio.

Casablanca
Як я вже зазначив в анотації, ми вже перевіряли проект Casablanca. Ви можете прочитати про це у статті "Маленька замітка про проект Casablanca".

Casablanca (C++ REST SDK) це невеликий проект, написаний на сучасному C++. Говорячи про сучасний мовою C++, я маю на увазі, що в коді активно використовується семантика переміщення (move semantics), лямбда-функції, auto і так далі. Нові можливості мови C++ дозволяють писати більш короткий і надійний код. Це підтверджується тим, що знайти помилки в цьому проекті непросте завдання. Хоча зазвичай ми робимо це вкрай легко.

Для зацікавлених, які ще проекти Microsoft ми перевіряли, наводжу список статей, присвячених цим перевіркам: Xamarin.Forms, CNTK, Microsoft Edge, CoreCLR, Windows 8 Driver Samples бібліотека Visual C++ 2012 / 2013, CoreFX, Roslyn, Microsoft Code Contracts, і скоро з'явиться стаття про перевірку WPF Samples.

Отже, проект Casablanca є зразком хорошого, якісного коду. Давайте подивимося, що ж все-таки можна знайти в ньому з допомогою статичного аналізатора коду PVS-Studio.

Що вдалося знайти поганого
Фрагмент N1: помилка

Є структура NumericHandValues, що містить два члена: low high. Ось оголошення цієї структури:
struct NumericHandValues
{
int low;
int high;
int Best() { return (high < 22) ? high : low; }
};

А тепер подивимося, як в одному місці ініціалізується ця структура:
NumericHandValues GetNumericValues()
{
NumericHandValues res;
res.low = 0;
res.low = 0;

....
}

Попередження PVS-Studio: V519 The 'res.low' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 130, 131. BlackJack_Client140 messagetypes.h 131

Як бачите, випадково два рази ініціалізувати член low, але при цьому забули ініціалізувати high. Глибокодумний коментар тут написати складно. Просто ніхто не застрахований від помилок.

Фрагмент N2: помилка звільнення пам'яті
void DealerTable::FillShoe(size_t decks)
{
std::shared_ptr<int> ss(new int[decks * 52]);
....
}

Попередження PVS-Studio: V554 Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. BlackJack_Server140 table.cpp 471

За замовчуванням розумний вказівник типу shared_ptr для знищення об'єкта викличе оператор delete без квадратних дужок []. В даному випадку це неправильно.

Коректний варіант коду повинен бути таким:
std::shared_ptr<int> ss(new int[decks * 52],
std::default_delete<int[]>());

Фрагмент N3: втрачений покажчик

Статичний член s_server_api представляє собою розумний покажчик та визначено наступним чином:
std::unique_ptr<http_server>
http_server_api::s_server_api((http_server*)nullptr);

Підозру викликає код наступної функції:
void http_server_api::unregister_server_api()
{
pplx::extensibility::scoped_critical_section_t lock(s_lock);

if (http_server_api::has_listener())
{
throw http_exception(_XPLATSTR("Server API ..... attached"));
}

s_server_api.release();
}

Попередження PVS-Studio: V530 The return value of function 'release' is required to be utilized. cpprestsdk140 http_server_api.cpp 64

Зверніть увагу на «s_server_api.release();». Після виклику функції release розумний покажчик більше не володіє об'єктом. Покажчик на об'єкт «втрачається» і об'єкт буде існувати до кінця життя програми.

Швидше за все, ми знову зіткнулися з помилкою. Я думаю, що хотіли викликати функцію reset, а зовсім не.

Фрагмент N4: не той enum

У проекті є два перерахування BJHandState BJHandResult, оголошені наступним чином:
enum BJHandState {
HR_Empty, HR_BlackJack, HR_Active, HR_Held, HR_Busted
};
enum BJHandResult {
HR_None, HR_PlayerBlackJack, HR_PlayerWin,
HR_ComputerWin, HR_Push
};

А тепер подивимося на фрагмент коду функції PayUp:
void DealerTable::PayUp(size_t idx)
{
....
if ( player.Hand.insurance > 0 &&
Players[0].Hand.state == HR_PlayerBlackJack )
{
player.Balance += player.Hand.insurance*3;
}
....
}

Попередження PVS-Studio: V556 The values of different enum types are compared. Types: BJHandState, BJHandResult. BlackJack_Server140 table.cpp 336

Змінна state тип BJHandState. А це означає, що програміст заплутався в перерахувань. По всій видимості код повинен виглядати так:
if ( player.Hand.insurance > 0 &&
Players[0].Hand.state == HR_BlackJack )

Забавно те, що це помилка насправді поки ніяк не позначається на роботі програми. Завдяки щасливому збігу обставин, на даний момент константи HR_BlackJack HR_PlayerBlackJack мають однакове значення, рівне 1. Справа в тому, що обидві ці константи перерахування знаходяться на одній позиції в списку. Однак, процес розвитку програми це може змінитися, і тоді виникне дивна, незрозуміла помилка.

Фрагмент N5: дивний break
web::json::value AsJSON() const 
{
....
int idx = 0;
for (auto iter = cards.begin(); iter != cards.end();)
{
jCards[idx++] = iter->AsJSON();
break;
}
....
}

Попередження PVS-Studio: V612 An unconditional 'break' within a loop. BlackJack_Client140 messagetypes.h 213

Оператор break в цьому коді виглядає вкрай підозріло. Цикл може зробити максимум одну ітерацію. Мені складно сказати, як повинен вести себе цей код, але, швидше за все, зараз з ним щось не так.

Інші дрібниці
Крім фрагментів коду, розглянутих вище і які претендують на звання помилок, аналізатор виявив також кілька неакуратних місць. Наприклад, використання постинкремента для ітераторів.
inline web::json::value
TablesAsJSON (...., std::shared_ptr<BJTable>> &tables)
{
web::json::value result = web::json::value::array();

size_t idx = 0;
for (auto tbl = tables.begin(); tbl != tables.end(); tbl++)
{
result[idx++] = tbl->second->AsJSON();
}
return result;
}

Попередження PVS-Studio: V803 Decreased performance. In case 'tbl' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. BlackJack_Client140 messagetypes.h 356

Це, звичайно, не помилка. Однак, гарним стилем вважається використання преинкремента: ++tbl. Для тих, хто сумнівається, що в цьому є сенс, я відсилаю до наступних двома статтями:
  1. Є практичний сенс використовувати для ітераторів префіксний оператор инкремента ++it, замість постфиксного it++. http://www.viva64.com/ru/b/0093/
  2. Pre vs. post increment operator — benchmark. http://silviuardelean.ro/2011/04/20/pre-vs-post-increment-operator/
У коді бібліотеки є ще 10 місць, де використовується постинкремент ітераторів в циклах, але я не бачу сенсу приводити їх у статті.

Розглянемо ще один приклад, коли аналізатор вказує на неакуратний код:
struct _acquire_protector
{
_acquire_protector (....);
~_acquire_protector();
size_t m_size;
private:
_acquire_protector& operator=(const _acquire_protector&);
uint8_t* m_ptr;
concurrency::streams::streambuf<uint8_t>& m_buffer;
};

Попередження PVS-Studio: V690 The '=' operator is declared as private in the '_acquire_protector' class, but the default copy constructor will still be generated by compiler. It is dangerous to use such a class. cpprestsdk140.uwp.staticlib fileio_winrt.cpp 825

Як бачимо, програміст заборонив використання оператора копіювання. Однак, об'єкт може бути скопійований з допомогою конструктора копіювання, створюваного компілятором за замовчуванням.

Висновок
Нарешті аналізатор PVS-Studio зміг до чого причепитися. Помилок знайшлося небагато, але все-таки вони є. А це означає, що якщо застосовувати статичний аналіз не разово, як я зробив зараз, а регулярно, то можна запобігати безліч помилок на самому ранньому етапі. Краще правити помилки відразу після написання коду, а не в процесі тестування, налагодження і тим більше після того, як про дефект повідомить один з користувачів.

посилання
  1. Назва статті є відсиланням до казки "Паровозик, який зміг".
  2. Посилання, де ви можете скачати аналізатор PVS-Studio і спробувати перевірити один зі своїх проектів на мові C, C++ або C#: http://www.viva64.com/ru/pvs-studio-download/



Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Andrey Karpov. The Little Unicorn That Could.

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

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

0 коментарів

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