Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

** На сайте установлена reCAPTCHA и применяются
Политика конфиденциальности и Условия использования Google.
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
GBP
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

** На сайте установлена reCAPTCHA и применяются
Политика конфиденциальности и Условия использования Google.
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

** На сайте установлена reCAPTCHA и применяются
Политика конфиденциальности и Условия использования Google.
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

** На сайте установлена reCAPTCHA и применяются
Политика конфиденциальности и Условия использования Google.
Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

** На сайте установлена reCAPTCHA и применяются
Политика конфиденциальности и Условия использования Google.
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Вторая проверка WinMerge с помощью PVS-…

Вторая проверка WinMerge с помощью PVS-Studio

28 Мар 2012

Статья продолжает развивать тему, что статические анализаторы кода, это инструменты постоянного, а не разового применения.

Введение

Анализатор PVS-Studio позволяет выявлять ошибки в приложениях на языке Си/Си++. Однажды, с его помощью был проверен проект WinMerge. Ошибок было найдено немного и с их описанием можно познакомиться в статье "Сравнение статического анализа общего назначения из Visual Studio 2010 и PVS-Studio на примере обнаруженных ошибок в пяти открытых проектах" [1].

С того времени прошёл год и возникло желание проверить новую версию WinMerge, используя новую версию PVS-Studio. Далее будут приведены результаты этой повторной проверки. Но важнее то, какой из этой проверки можно сделать вывод:

Нет никакого смысла один раз проверить проект, используя инструмент статического анализа кода, и успокоиться на этом. Анализ должен выполняться регулярно.

И вот почему:

  • Каждая новая версия анализатора, как правило, содержит новые диагностические правила, а значит позволят выявить больше ошибок.
  • В процессе написания нового кода возникают новые ошибки. Наиболее дешёвым способом их обнаружения многих из них является использование статических анализаторов кода [2].

Вернемся теперь к найденным в коде дефектам. Заметим, что многие из описанных ошибок относятся не к самому проекту WinMerge, а к используемым в нем библиотекам. Однако, это не имеет значения. Хотелось показать, что анализатор PVS-Studio быстро развивается и учится выявлять всё новые разновидности дефектов. И это удается продемонстрировать на примерах.

Фрагменты очень подозрительного кода

Фрагмент N1

BOOL CCrystalEditView::
DoDropText (....)
{
  ...
  UINT cbData = (UINT) ::GlobalSize (hData);
  UINT cchText = cbData / sizeof(TCHAR) - 1;
  if (cchText < 0)
    return FALSE;
  ...
}

Диагностика анализатора PVS-Studio: V547 Expression 'cchText < 0' is always false. Unsigned type value is never < 0. Merge ccrystaleditview.cpp 1135

Функция GlobalSize() возвращает в случае ошибки значение 0. Если это произойдёт, данная ситуация будет обработана неправильно. Код построен с использованием беззнаковых типов данных. В том числе и переменная 'cchText' имеет тип 'unsigned'. Это значит, что условие "cchText < 0" всегда ложно. Код можно исправить, переписав его следующим образом:

UINT cbData = (UINT) ::GlobalSize (hData);
if (cbData < sizeof(TCHAR))
  return FALSE;
UINT cchText = cbData / sizeof(TCHAR) - 1;

Фрагмент N2

bool isopenbrace (TCHAR c)
{
  return c == _T ('{') || c == _T ('(') ||
         c == _T ('[') || c == _T ('<');
}

bool isclosebrace (TCHAR c)
{
  return c == _T ('}') || c == _T ('}') ||
         c == _T (']') || c == _T ('>');
}

Диагностика анализатора PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '||' operator: c == L'}' || c == L'}' Merge ccrystaleditview.cpp 1556

В функции isclosebrace() переменная 'c' два раза сравнивается с символом '}'. Если изучить код функции isopenbrace(), то становится ясно, что во втором случае переменная 'c' должна сравниваться с символом ')'.

Фрагмент N3

static HRESULT safeInvokeA(....)
{
  HRESULT h;
  ...
  // set h to FAILED
  h = -1;
  ...
}

Диагностика анализатора PVS-Studio: V543 It is odd that value '-1' is assigned to the variable 'h' of HRESULT type. Merge plugins.cpp 992

Присваивать значение -1 переменной, имеющей тип HRESULT, это очень некрасиво и неправильно.

HRESULT - это 32-разрядное значение, разделенное на три различных поля: код серьезности ошибки, код устройства и код ошибки. Для работы со значением HRESULT служат специальные константы, такие как S_OK, E_FAIL, E_ABORT и так далее. А для проверки значений тип HRESULT предназначены такие макросы, как SUCCEEDED, FAILED.

Запись значения "-1" некорректна. Если хочется сообщить о какой-то непонятной ошибке, то следует использовать значение 0x80004005L (Unspecified failure). Эта и аналогичные константы, описаны в "WinError.h".

Аналогичная ошибка также имеется здесь:

V543 It is odd that value '-1' is assigned to the variable 'h' of HRESULT type. Merge plugins.cpp 1033

Фрагмент N4

int TimeSizeCompare::CompareFiles(....)
{
  UINT code = DIFFCODE::SAME;
  ...
  if (di.left.size != di.right.size)
  {
    code &= ~DIFFCODE::SAME;
    code = DIFFCODE::DIFF;
  }
  ...
}

Диагностика анализатора PVS-Studio: V519 The 'code' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 79, 80. Merge timesizecompare.cpp 80

Возможно, здесь есть ошибка, а возможно нет. Будучи не знакомым с внутренним устройством проекта WinMerge мне сложно делать выводы.

Варианты:

  • Код содержит ошибку и вторая строка должна выглядеть, например, так: "code |= DIFFCODE::DIFF;".
  • Код корректен. Первая строчка лишняя.

Фрагмент N5

BOOL CEditDropTargetImpl::
OnDrop (....)
{
  bool bDataSupported = false;

  m_pOwner->HideDropIndicator ();

  if ((!m_pOwner) ||
      (!(m_pOwner->QueryEditable ())) ||
      (m_pOwner->GetDisableDragAndDrop ()))
  ...
}

Диагностика анализатора PVS-Studio: V595 The 'm_pOwner' pointer was utilized before it was verified against nullptr. Check lines: 1033, 1035. Merge ccrystaleditview.cpp 1033

Как видно из условия "if ((!m_pOwner) ....)" указатель 'm_pOwner' может быть равен нулю. Однако, прежде чем проверка будет выполнена, этот указатель уже используется в высказывании 'm_pOwner->HideDropIndicator()'. Таким образом, вместо штатной обработки нулевого указателя возникнет ошибка сегментации.

Фрагмент N6

BCMenu *BCMenu::FindMenuOption(int nId, UINT& nLoc)
{
  ...
  nLoc = -1;
  ...
}

BOOL BCMenu::ModifyODMenuW(....)
{
  UINT nLoc;
  ...
  BCMenu *psubmenu = FindMenuOption(nID,nLoc);
  ...
  if (psubmenu && nLoc>=0)
    mdata = psubmenu->m_MenuList[nLoc];
  ...
}

Диагностика анализатора PVS-Studio: V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1232

При определённых обстоятельствах функция FindMenuOption() возвращает в переменной 'nLoc' значение -1. Так как переменная 'nLoc' имеет беззнаковый тип, то на самом деле функция вернёт 0xFFFFFFFFu.

Теперь рассмотрим код в функции ModifyODMenuW(). Условие "nLoc>=0" всегда истинно. Это значит, что ситуация, когда функция FindMenuOption() возвращает -1, будет обработана неверно.

Аналогичные ошибки:

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1263

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1285

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1309

V547 Expression 'loc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1561

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 2409

Фрагмент N7

В программе существует класс CompareOptions, который имеет виртуальные методы, но не имеет виртуального деструктора. От него наследуются другие классы, например, DiffutilsOptions. Соответственно, отсутствие виртуального деструктора, хотя может и не приводить к катастрофическому результату, тем не менее, является ошибкой.

Диагностика анализатора PVS-Studio: V599 The virtual destructor is not present, although the 'CompareOptions' class contains virtual functions. Merge diffcontext.cpp 90

Приводить соответствующие участки кода здесь нет смысла, так как они займут много места.

Следует отметить, что анализатор PVS-Studio достаточно аккуратен в диагностике и не ругается на все подряд классы, в которых отсутствует виртуальный деструктор. Понять, как анализатор диагностирует этот тип ошибки, можно познакомившись с её описанием: V599. The virtual destructor is not present, although the 'Foo' class contains virtual functions.

Фрагмент N8

static void StoreDiffData(....)
{
  ...
  GetLog()->Write
  (
    CLogFile::LCOMPAREDATA,
    _T("name=<%s>, leftdir=<%s>, rightdir=<%s>, code=%d"),
    di.left.filename.c_str(),
    di.left.path.c_str(),
    di.right.path.c_str(), di.diffcode
  );
  pCtxt->m_pCompareStats->AddItem(di.diffcode.diffcode);
  ...
}

Диагностика анализатора PVS-Studio: V510 The 'Write' function is not expected to receive class-type variable as sixth actual argument. Merge dirscan.cpp 565

Переменная 'di.diffcode' представляет собой структуру типа DIFFCODE. По всей видимости, корректный код должен был выглядеть так:

CLogFile::LCOMPAREDATA, _T(...., di.diffcode.diffcode);

Фрагмент N9

static DIFFITEM *AddToList(....,
 const DirItem * lent, const DirItem * rent,
 ....)
{
  ...
  if (lent)
  {
    ...
  }
  else
  {
    di->left.filename = rent->filename;
  }

  if (rent)
  {
  ...
}

Диагностика анализатора PVS-Studio: V595 The 'rent' pointer was utilized before it was verified against nullptr. Check lines: 608, 611. Merge dirscan.cpp 608

Указатель 'rent' используется без проверки на равенство нулю. Возможно на практике такая ситуация никогда не возникает. Тем не менее, проверка "if (rent)" намекает, что такое теоретически возможно.

Фрагмент N10

String FileFilterHelper::ParseExtensions(....) const
{
  String strParsed;
  String strPattern;
  ...
  strParsed = _T("^");
  strPattern = string_makelower(strPattern);
  strParsed = strPattern;
  ...
}

Диагностика анализатора PVS-Studio: V519 The 'strParsed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 339, 342. Merge filefilterhelper.cpp 342

Переменной 'strParsed' два раза подряд присваиваются различные значения. В этом коде присутствует ошибка или лишнее присваивании. Похожий случай уже рассматривался ранее.

Фрагмент N11

void CLogFile::EnableLogging(BOOL bEnable)
{
  ...
  Write(_T("Path: %s\n*******\n"), m_strLogPath);
  ...
}

Диагностика анализатора PVS-Studio: V510 The 'Write' function is not expected to receive class-type variable as second actual argument. Merge logfile.cpp 85

Переменная 'm_strLogPath' имеет тип std::wstring. Это значит, что в лог запишется мусор. Корректный вариант кода:

Write(_T("Path: %s\n*******\n"), m_strLogPath.c_str());

Фрагмент N12

void CMergeDoc::Computelinediff(
  CCrystalTextView * pView1, CCrystalTextView * pView2, 
  ....)
{
  ...
  if (pView1->GetTextBufferEol(line) !=
      pView1->GetTextBufferEol(line))
  ...
}

Диагностика анализатора PVS-Studio: V501 There are identical sub-expressions 'pView1->GetTextBufferEol(line)' to the left and to the right of the '!=' operator. Merge mergedoclinediffs.cpp 216

Два раза используется переменная 'pView1'. Скорее всего, в этом коде имеется опечатка, и корректный код должен был выглядеть так:

if (pView1->GetTextBufferEol(line) !=
    pView2->GetTextBufferEol(line))

Фрагмент N13

void CSplashWnd::OnPaint()
{
  ...
  String text = LoadResString(IDS_SPLASH_DEVELOPERS);

  // avoid dereference of empty strings and
  // the NULL termiated character
  if (text.length() >= 0)
  {
  ...
}

Диагностика анализатора PVS-Studio: V547 Expression 'text.length() >= 0' is always true. Unsigned type value is always >= 0. Merge splash.cpp 262

Проверка "text.length() >= 0" не имеет смысла. Тип 'String' представляет из себя 'std::wstring'. Функция 'std::wstring::length()' всегда возвращает значение большее или равное 0.

Фрагмент N14

void CPreferencesDlg::AddPage(CPropertyPage* pPage, ....)
{
  ...
  m_tcPages.SetItemData(hti, (DWORD)pPage);
  ...
}

Диагностика анализатора PVS-Studio: V205 Explicit conversion of pointer type to 32-bit integer type: (DWORD) pPage Merge preferencesdlg.cpp 200

Теоретически (но вряд ли практически) в 64-битной программе, объект, на который указывает 'pPage', может находиться за пределами младших четырёх гигабайт. Это несет потенциальную опасность, так как указатель явно приводится к 32-битному типу 'DWORD'. Безопасный код должен выглядеть так:

m_tcPages.SetItemData(hti, (DWORD_PTR)pPage);

Заключение

Обнаружены и другие подозрительные места в коде. Но заявить о наличии в них ошибок я затрудняюсь. Главное, что прогресс в развитии анализатора PVS-Studio очевиден.

Если вы хотите скачать пробную полнофункциональную версию анализатора, то перейдите по этой ссылке: /ru/pvs-studio/download/. С новой моделью триала вы можете получить пользу от анализатора без его покупки

Если у вас возникли вопросы касательно этой статьи или анализатора, то предлагаю познакомиться с заметкой "Ответы на вопросы, которые часто задают после прочтения наших статей" [3]. Также вы можете написать и задать вопросы лично мне и моим коллегам, используя форму обратной связи.

Библиографический список

  • Евгений Рыжков. Сравнение статического анализа общего назначения из Visual Studio 2010 и PVS-Studio на примере обнаруженных ошибок в пяти открытых проектах. https://pvs-studio.com/ru/blog/posts/a0073/
  • Андрей Карпов. Лев Толстой и статический анализ кода. https://pvs-studio.com/ru/blog/posts/0105/
  • Андрей Карпов. Ответы на вопросы, которые часто задают после прочтения наших статей. https://pvs-studio.com/ru/blog/posts/0132/

Популярные статьи по теме
Как и почему статические анализаторы борются с ложными срабатываниями

Дата: 20 Мар 2017

Автор: Андрей Карпов

В своей предыдущей статье я писал, что мне не нравится подход, при котором статические анализаторы кода оцениваются с помощью синтетических тестов. В статье приводился пример, воспринимаемый анализат…
Главный вопрос программирования, рефакторинга и всего такого

Дата: 14 Апр 2016

Автор: Андрей Карпов

Вы угадали, ответ - "42". Здесь приводится 42 рекомендации по программированию, которые помогут избежать множества ошибок, сэкономить время и нервы. Автором рекомендаций выступает Андрей Карпов - тех…
Зло живёт в функциях сравнения

Дата: 19 Май 2017

Автор: Андрей Карпов

Возможно, читатели помнят мою статью под названием "Эффект последней строки". В ней идёт речь о замеченной мной закономерности: ошибка чаще всего допускается в последней строке однотипных блоков текс…
Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей

Дата: 21 Ноя 2018

Автор: Андрей Карпов

Краткое описание технологий, используемых в инструменте PVS-Studio, которые позволяют эффективно обнаруживать большое количество паттернов ошибок и потенциальных уязвимостей. Статья описывает реализа…
Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Дата: 31 Июл 2017

Автор: Андрей Карпов

После большой статьи про проверку операционной системы Tizen мне было задано много вопросов о проценте ложных срабатываний и о плотности ошибок (сколько ошибок PVS-Studio выявляет на 1000 строк кода)…
PVS-Studio ROI

Дата: 30 Янв 2019

Автор: Андрей Карпов

Время от времени нам задают вопрос, какую пользу в денежном эквиваленте получит компания от использования анализатора PVS-Studio. Мы решили оформить ответ в виде статьи и привести таблицы, которые по…
Эффект последней строки

Дата: 31 Май 2014

Автор: Андрей Карпов

Я изучил множество ошибок, возникающих в результате копирования кода. И утверждаю, что чаще всего ошибки допускают в последнем фрагменте однотипного кода. Ранее я не встречал в книгах описания этого …
Как PVS-Studio оказался внимательнее, чем три с половиной программиста

Дата: 22 Окт 2018

Автор: Андрей Карпов

PVS-Studio, как и другие статические анализаторы кода, часто выдаёт ложные срабатывания. Но не стоит спешить считать странные срабатывания ложными. Это короткая история о том, как PVS-Studio вновь ок…
Бесплатный PVS-Studio для тех, кто развивает открытые проекты

Дата: 22 Дек 2018

Автор: Андрей Карпов

В канун празднования нового 2019 года команда PVS-Studio решила сделать приятный подарок всем контрибьюторам open-source проектов, хостящихся на GitHub, GitLab или Bitbucket. Им предоставляется возмо…
Любите статический анализ кода!

Дата: 16 Окт 2017

Автор: Андрей Карпов

Я в шоке от возможностей статического анализа кода, хотя сам участвую в разработке инструмента PVS-Studio. На днях я был искренне удивлён тому, что анализатор оказался умнее и внимательнее меня.

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

Следующие комментарии

На сайте установлена reCAPTCHA и применяются
Политика конфиденциальности и Условия использования Google.
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять