Одна из команд разработчиков 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, а вовсе не release.

Фрагмент 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.