Единорог, который смог
Одна из команд разработчиков 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 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 ss(new int[decks * 52],
std::default_delete());
Фрагмент N3: потерянный указатель
Статический член s_server_api представляет собой умный указатель и определён следующим образом:
std::unique_ptr
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> &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. Для тех, кто сомневается, что в этом есть смысл, я отсылаю к следующим двум статьям:
- Есть ли практический смысл использовать для итераторов префиксный оператор инкремента ++it, вместо постфиксного it++. http://www.viva64.com/ru/b/0093/
- 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& 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 смог к чему-то придраться. Ошибок нашлось немного, но всё-таки они есть. А это значит, что если применять статический анализ не разово, как я сделал сейчас, а регулярно, то можно предотвращать множество ошибок на самом раннем этапе. Лучше править ошибки сразу после написания кода, а не в процессе тестирования, отладки и тем более после того как о дефекте сообщит один из пользователей.
Дополнительные ссылки
- Название статьи является отсылкой к сказке «Паровозик, который смог».
- Ссылка, где вы можете скачать анализатор PVS-Studio и попробовать проверить один из своих проектов на языке C, C++ или C#: http://www.viva64.com/ru/pvs-studio-download/
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. The Little Unicorn That Could.
