Кирилл Юдинцев

13 Июл 2016

Теги:

#Cpp

Проверяем исходный код библиотеки Nana с помощью PVS-Studio

13 Июл 2016

Автор: Кирилл Юдинцев

Введение
Опечатки в условиях
Использование нулевого указателя (не ошибка, но такие места всегда полезно проверить)
Некорректное использование умного указателя
Избыточное сравнение
Снова про опасное использование указателей
Использование функции SuspendThread()
Заключение

C выходом новых стандартов C++ в сообществе C++ разработчиков наблюдается переход на новый стиль программирования, так называемый современный C++. На свет стали появляться проекты, написанные в новом стиле. Статические анализаторы кода должны идти в ногу со временем, чтобы обнаруживать ошибки в современном C++ коде, и PVS-Studio - не исключение. Посмотрим на примере проекта Nana, справляется ли с современным С++ кодом анализатор PVS-Studio.

Введение

Для начала несколько слов о проекте. Nana является кроссплатформенной C++11 библиотекой для создания графических пользовательских интерфейсов. Библиотека имеет небольшой размер в 74 тысячи строк кода. Поддерживаются платформы Windows, Linux (X11) и macOS в экспериментальном режиме. Nana - это программное обеспечение с открытым исходным кодом, распространяемое по лицензии Boost Software License. Для проверки была выбрана версия 1.3.0, исходный код которой можно скачать по ссылке https://sourceforge.net/projects/nanapro/files/latest/download.

Опечатки в условиях

Опечатки в условных операторах встречаются достаточно часто. Библиотека Nana не оказалась исключением. Приведу пример найденной опечатки.

V501 There are identical sub-expressions 'fgcolor.invisible()' to the left and to the right of the '&&' operator. text_editor.cpp 1316

void text_editor::set_highlight(
  const std::string& name,
  const ::nana::color& fgcolor,
  const ::nana::color& bgcolor)
{
  if (fgcolor.invisible() && fgcolor.invisible())  // <=
  {
    keywords_->schemes.erase(name);
    return;
  }
  ....
}

Анализатор обнаружил одинаковые условные выражения. Скорее всего, здесь не обошлось без Copy-Paste. Программист скопировал выражение fgcolor.invisible(), а поменять имя переменной fgcolor забыл. Судя по параметрам функции, вместо fgcolor во втором подвыражении хотели использовать аргумент bgcolor. В таком случае корректный код должен выглядеть следующим образом:

void text_editor::set_highlight(
  const std::string& name,
  const ::nana::color& fgcolor,
  const ::nana::color& bgcolor)
{
  if (fgcolor.invisible() && bgcolor.invisible())
  {
    keywords_->schemes.erase(name);
    return;
  }
  ....
}

В программировании от использования Copy-Paste почти невозможно отказаться, поэтому необходимо просто более внимательно проверять скопированный и исправленный код. Ну а если такая ошибка была допущена, то сэкономить время на ее поиск поможет использование статического анализа кода.

Использование нулевого указателя (не ошибка, но такие места всегда полезно проверить)

Анализатор обнаружил фрагмент кода, который использует нулевой указатель.

V522 Dereferencing of the null pointer 'debug' might take place. text_token_stream.hpp 669

Вот как это выглядит в коде приложения:

void parse(....)
{
  ....
  switch(tk)
  {
    ....
    default:
      int * debug = 0;  //for debug.
      *debug = 0;
  }
  ....
}

В данной ситуации, судя по комментарию, такой трюк с указателем debug используется с целью отладки. Но я все же решил привести этот фрагмент для примера. Нужно быть аккуратнее с такими вещами, так как впоследствии логика работы программы может измениться, и нас будет ждать неприятный сюрприз. В любом случае, этот код не помешает лишний раз проверить.

Некорректное использование умного указателя

Вот и добрались до C++11. Анализатор обнаружил ситуацию, когда использование умного указателя может привести к неопределенному поведению, в частности - к повреждению кучи или аварийному завершению программы. Ошибка заключается в том, что память выделяется и освобождается различными методами.

V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. text_editor.cpp 3137

void text_editor::_m_draw_string(....) const
{
  ....
  for (auto & ent : reordered)
  {
    ....
    std::size_t len = ent.end - ent.begin;
    ....
    if (....)
    {
      ....
    }
    else if (pos <= a.x && a.x < str_end)
    {
      ....
      std::unique_ptr<unsigned> pxbuf_ptr(new unsigned[len]);  // <=
    }
  }
}

В данной ситуации класс unique_ptr используется для управления памятью, выделенной для массива. По умолчанию unique_ptr использует для освобождения памяти оператор delete. В результате чего, возникнет неопределенное поведение программы. Для исправления ошибки необходимо использовать частичную специализацию unique_ptr для массива. Тогда для очистки памяти будет вызываться оператор delete[]. Корректный вариант кода будет выглядеть следующим образом:

std::unique_ptr<unsigned[]> pxbuf_ptr(new unsigned[len]);

Избыточное сравнение

Иногда в условных операторах встречаются избыточные сравнения. Такие сравнения могут скрывать в себе потенциальные ошибки. Анализатор обнаружил избыточное сравнение в коде и выдал соответствующее предупреждение:

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. window_manager.cpp 467

void window_manager::destroy_handle(core_window_t* wd)
{
  ....
  if((wd->other.category == category::root_tag::value) ||
     (wd->other.category != category::frame_tag::value))  // <=
  {
   impl_->misc_register.erase(wd->root);
   impl_->wd_register.remove(wd);
  }
}

Поясню на простом примере:

if (a == 1 || a != 5)

Условие будет выполнено в том случае, если a != 5. Первая часть выражения бессмысленна. Проанализировав код, можно прийти к одному из двух выводов. Или выражение можно упростить, убрав первую часть. Тогда код будет выглядеть следующим образом:

if (a != 5)

Или выражение содержит ошибку. В этом случае после исправления получим код:

if (a == 1 || not_a_var != 5)

В найденном фрагменте, скорее всего, имеет место первый случай и упрощенное выражение будет иметь следующий вид:

void window_manager::destroy_handle(core_window_t* wd)
{
  ....
  if(wd->other.category != category::frame_tag::value)
  {
   impl_->misc_register.erase(wd->root);
   impl_->wd_register.remove(wd);
  }
}

Снова про опасное использование указателей

При использовании указателей в C/C++ следует проявлять особую внимательность. Если нет уверенности, что по указателю есть данные, нужно обязательно проверять его на нулевое значение. Обращение по нулевому указателю приведет к неопределенному поведению или аварийному завершению программы.

V595 The 'owner' pointer was utilized before it was verified against nullptr. Check lines: 299, 315. window_manager.cpp 299

window_manager::core_window_t*
window_manager::create_root(core_window_t* owner,    )
{
  ....
  if (nested)
  {
    wd->owner = nullptr;
    wd->parent = owner;
    wd->index = static_cast<unsigned>(owner->children.size());
    owner->children.push_back(wd);  // <=
  }
  ....
  if (owner 
      && owner->other.category == category::frame_tag::value)  // <=
    insert_frame(owner, wd);
  ....
}

V595 является, пожалуй, самым распространенным предупреждением среди всех проектов. Еще одно аналогичное предупреждение из проекта:

V595 The 'wd' pointer was utilized before it was verified against nullptr. Check lines: 1066, 1083. window_manager.cpp 1066

Использование функции SuspendThread()

Анализатор обнаружил, что в программе используется функция SuspendThread():

V720 It is advised to utilize the 'SuspendThread' function only when developing a debugger (see documentation for details). pool.cpp 225

void _m_suspend(pool_throbj* pto)
{
  pto->thr_state = state::idle;
#if defined(NANA_WINDOWS)
  ::SuspendThread(pto->handle);  // <=
#elif defined(NANA_POSIX)
  std::unique_lock<std::mutex> lock(pto->wait_mutex);
  pto->suspended = true;
  pto->wait_cond.wait(lock);
  pto->suspended = false;
#endif
}

Сам по себе вызов этой функций не является ошибкой. Однако, разработчики часто используют ее не по назначению. Из-за этого программа может вести себя не так, как ожидает программист. Функция SuspendThread() должна помогать при разработке отладчиков и подобных им приложений. Если она используется в прикладном программном обеспечении для задач синхронизации, то высока вероятность получения ошибки.

Суть проблемы неправильного использования функции SuspendThread() изложена в следующих статьях:

Заключение

Nana - это небольшой проект, и найти много ошибок в нем не удалось. Однако, на некоторые места все же стоит обратить внимание. Нашлась и ошибка, связанная с использованием стандарта C++11. Конечно, этого недостаточно для раскрытия возможностей PVS-Studio по работе с C++11 проектами. Поэтому мы всегда будем рады вашим предложениям. Если у вас есть на примете проекты, написанные в современном C++ стиле, дайте нам знать, и мы попробуем их проверить. Для обратной связи пишите через форму обратной связи.

В заключение хотелось бы сказать, что при разработке программного обеспечения не стоит ограничиваться тестами и code review. Использование статического анализатора позволяет писать более качественный код, экономя время на поиске ошибок. Поэтому предлагаю всем желающим попробовать PVS-Studio в своих проектах, написанных на языке C, C++ или C#.

#Cpp