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, а вовсе не 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.

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

Комментарии (14)


  1. AllexIn
    22.06.2016 16:01
    +3

    По четвертой ошибке есть же простой совет — enum class. И там уже даже компилятор не позволит явно присвоить.



  1. lockywolf
    22.06.2016 19:27

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

    А оператор в цикле не может иметь побочного эффекта, меняющего EIP?


    1. mayorovp
      22.06.2016 21:29

      Нет, не может.


  1. Wesha
    23.06.2016 00:28
    +3

    > Как видите, случайно два раза инициализировали член low, но при этом забыли инициализировать high. Глубокомысленный комментарий здесь написать сложно.

    Чего ж сложного — Вы ж сами писали про эффект последней строки.
    image


  1. Arkham
    23.06.2016 06:17

    Однако, объект по-прежнему может быть скопирован с помощью конструктора копирования, создаваемого компилятором по умолчанию.

    Как исправить эту ошибку?


    1. mapron
      23.06.2016 06:19
      +1

      private:
      _acquire_protector& operator=(const _acquire_protector&);
      _acquire_protector(const _acquire_protector&); (или = delete)


      1. Arkham
        23.06.2016 08:35

        Спасибо.


  1. mapron
    23.06.2016 06:19
    +1

    Я правильно понимаю, что все ошибки найденные в статье, были в директории samples (т.е. примеры)?
    enum BJHandState — вообще странно, что там значения имеют префикс HR_. Ну и да, сам проект вроде как использует C++11, а тут на enum class поскупились.

    В общем, чтобы не давать хлеба разработчикам PVS-Studio, уважаемые разработчики библиотек, тщательнее вылизывайте примеры! :D


    1. Andrey2008
      23.06.2016 10:09

      Я правильно понимаю, что все ошибки найденные в статье, были в директории samples (т.е. примеры)?
      Кажется, да. Проект небольшой, поэтому искали где могли… :)


    1. Wesha
      23.06.2016 10:53
      +1

      Я правильно понимаю, что все ошибки найденные в статье, были в директории samples (т.е. примеры)?
      Так это ж ещё хуже, когда ошибки в «смотрите, дети, как надо писАть»!


      1. jaiprakash
        23.06.2016 11:04
        +2

        Вне зависимости от ударения в слове «писать» это плохо )


  1. jaiprakash
    23.06.2016 11:08
    +3

    Благодаря статье узнал что сказка про паровозик имеет не только версию, озвученную в «Майоре Пэйне».


    1. Wesha
      23.06.2016 18:11

      Это уже к любителям пони.