Трепещи, мир! Мы выпустили PVS-Studio 4.00 с анализатором общего назначения

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

Вы можете скачать PVS-Studio по адресу http://www.viva64.com/ru/pvs-studio/download/.

В статье кратко рассказывается о новых возможностях PVS-Studio. На примере статического анализа исходного кода проекта TortoiseSVN будет продемонстрировано использование новых диагностических возможностей.

PVS-Studio - современный анализатор исходного кода, интегрирующийся в среду Microsoft Visual Studio 2005/2008/2010. Анализатор позволяет удобно работать со списком предупреждений и использовать для своей работы несколько ядер процессора. PVS-Studio ориентирован на разработчиков современных Windows-приложений на языке Си/Си++/Си++0x.

До настоящего момента в PVS-Studio входило два набора правил. Первый для выявления дефектов в 64-битных программах. Второй для выявления дефектов в параллельных программах, построенных на основе технологии OpenMP.

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

Сейчас вы можете познакомиться с новым набором правил, скачав PVS-Studio 4.00 BETA. Мы будем рады получить от вас замечания по поводу замеченных ошибок и пожелания по улучшению. Сразу хочу заметить, что для начала мы реализовали только 50 правил общего назначения. Это немного. Поэтому если вы, скачав и попробовав PVS-Studio, сразу не найдете что-то интересное в своем коде, не торопитесь с выводами. Предлагаем вам в будущем вновь попробовать PVS-Studio, когда набор проверок будет существенно увеличен. Мы планируем в скором времени активно поработать над расширением базы диагностических правил (если хватит здоровья и повезет).

Продемонстрируем использование нового набора правил PVS-Studio на примере TortoiseSVN. TortoiseSVN –это клиент для системы контроля версий Subversion, выполненный как расширение оболочки Windows. TortoiseSVN хорошо знаком многим разработчикам и, думаю, подробнее описывать это приложение нет смысла. Отмечу только, что в 2007 году на SourceForge.net TortoiseSVN был признан лучшим проектом в категории "инструменты и утилиты для разработчиков".

Шаг 1

Скачиваем PVS-Studio с сайта компании ООО "Системы программной верификации" (это мы). Надеюсь, вам будет приятно, что не надо заполнять никаких анкет или разгадывать капчу. Просто скачиваем.

Шаг 2

Устанавливаем PVS-Studio. Можно смело нажимать кнопку "Next". Настраивать ничего не надо. Дистрибутив PVS-Studio подписан цифровой подписью. Однако некоторые антивирусы все равно могут насторожиться от действий, связанных с интеграцией в Visual Studio. Всё разрешаем.

Шаг 3

Скачиваем комплект исходных текстов проекта TortoiseSVN. Мы работали с исходным кодом версии 1.6.11.

Шаг 4

Открываем проект TortoiseSVN и запускаем анализ. Для этого в меня PVS-Studio выберем команду Check Solution.

Шаг 5

Анализатор на некоторое время задумается (проект TortoiseSVN не так прост и содержит кучу файлов). Поэтому не торопимся делать что-то в этот момент и немного подождем. Через некоторое время появится диалог прогресса и начнется анализ. Скорость анализа зависит от количества ядер в процессоре. Если PVS-Studio будет съедать слишком много ресурсов, можно в настройках ограничить его желания.

Сообщения анализатор выдает в свое собственное окошко, где имеются элементы управления для включения/выключения различных типов сообщений. И мы воспользуемся этими возможностями. Сейчас совершенно не интересно смотреть на множество ошибок связанных с 64-битностью.

В окне виден набор из трех кнопок, отвечающий за вывод сообщений из трех наборов правил.

1) 64 - диагностические сообщения о 64-битных дефектах (Viva64);

2) MP - диагностические сообщения о параллельных дефектах (VivaMP);

3) GA - диагностические сообщения общего типа (General Analysis).

Нам интересен только набор правил общего назначения (GA). Отжимаем другие кнопки, и лишние сообщения в списке будут тут же спрятаны.

Ждем завершения анализа.

Шаг 6

Анализ закончен, и мы видим список всех мест в программе, для которых анализатор рекомендует сделать обзор кода (code review).

Все предупреждения разделены на 3 уровня важности (это новое в PVS-Studio 4.00). Обычно, имеет смысл смотреть только первый и второй уровень. PVS-Studio 4.00 BETA выдала 33 предупреждения первого уровня, 14 предупреждений второго уровня и 8 предупреждений третьего уровня.

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

Шаг 7

Рассмотрим интересные места в коде, обнаруженные анализатором.

Ситуация 1

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

V530 The return value of function 'empty' is required to be utilized. contextmenu.cpp 434

V530 The return value of function 'remove' is required to be utilized. contextmenu.cpp 442

STDMETHODIMP CShellExt::Initialize(....)
{
  ...
  ignoredprops.empty();
  for (int p=0; p<props.GetCount(); ++p)
  {
    if (props.GetItemName(p).
          compare(SVN_PROP_IGNORE)==0)
    {
      std::string st = props.GetItemValue(p);
      ignoredprops = UTF8ToWide(st.c_str());
      // remove all escape chars ('\\')
      std::remove(ignoredprops.begin(),
                  ignoredprops.end(), '\\');
      break;
    }
  }
  ...
}

Здесь и далее будет даваться только краткий комментарий к фрагментам кода. Подробнее, узнать, почему код считается подозрительным, вы можете в online-документации к PVS-Studio. Также в комплекте с PVS-Studio идет документация (та же самая) в формате PDF-файла. Далее будут даваться ссылки на соответствующие описания диагностических сообщений.

Предупреждение V530 подсказывает нам, что "ignoredprops.empty()" вовсе не очищает строку, а "std::remove()" на самом деле не удалит символы.

Ситуация 2

Здесь происходит проверка, что переменная типа 'char' больше или равна значению 0x80.

V547 Expression 'c >= 0x80' is always false. The value range of signed char type: [-128, 127]. pathutils.cpp 559

CString CPathUtils::PathUnescape (const char* path)
{
  // try quick path
  size_t i = 0;
  for (; char c = path[i]; ++i)
    if ((c >= 0x80) || (c == '%'))
    {
      // quick path does not work for
      // non-latin or escaped chars
      std::string utf8Path (path);
      CPathUtils::Unescape (&utf8Path[0]);
      return CUnicodeUtils::UTF8ToUTF16 (utf8Path);
    }
  ...
}

Сообщение V547 уведомляет, что такая проверка бессмысленна. Значение типа 'char' всегда будет меньше 0x80, а значит условие всегда ложно. И возможно именно из-за этой ошибки в коде написан комментарий "quick path does not work for non-latin or escaped chars". Конечно не работает, но не потому что не получается конвертировать строку. Когда попадается non-latin символ, мы просто не заходим в тело оператора 'if'.

Ситуация 3

Множество потоков создается и убивается с помощью функций CreateThread/ExitThread. А, следовательно, мы рискуем быстро исчерпать стек потока или не освободить часть ресурсов, когда поток завершается. Подробнее читайте в описании предупреждения V513. Намного безопаснее использовать функции _beginthreadex() и _endthreadex().

Пример кода приводить здесь смысла нет, а текст всех сообщений одинаков:

V513 Use _beginthreadex/_endthreadex functions instead of CreateThread/ExitThread functions. crashhandler.cpp 379

Ситуация 4

Это относится к сопутствующим TortoiseSVN утилитам. Очень вероятно, что при работе с CrashLog все закончится другим Crash.

V510 The 'printf_s' function is not expected to receive class-type variable as fourth actual argument. excprpt.cpp 199

string CExceptionReport::getCrashLog()
{
  ...
  _tprintf_s(buf, _T("%s\\%s.xml"),
  getenv("TEMP"), CUtility::getAppName());
  ...
}

Сообщение V510 предупреждает, что передавать в функцию printf_s параметр типа std::string очень плохо. А функция CUtility::getAppName() возвращает именно std::string. Ошибка в том, что забыли написать ".c_str()". Последствием может быть как просто некорректный вывод данных, так и падение программы.

Ситуация 5

Хотели очистить массив, но не смогли.

V530 The return value of function 'empty' is required to be utilized. mailmsg.cpp 40

CMailMsg& CMailMsg::SetFrom(string sAddress,
                            string sName)
{
   if (initIfNeeded())
   {
      // only one sender allowed
      if (m_from.size())
         m_from.empty();
      m_from.push_back(TStrStrPair(sAddress,sName));
   }
   return *this;
}

Вновь сообщение V530 подсказывает, что вместо "clear()" случайно написано "empty()".

Ситуация 6

И в функции SetTo() тоже не смогли очистить массив.

V530 The return value of function 'empty' is required to be utilized. mailmsg.cpp 54

CMailMsg& CMailMsg::SetTo(string sAddress,
                          string sName)
{
   if (initIfNeeded())
   {
      // only one recipient allowed
      if (m_to.size())
         m_to.empty();

      m_to.push_back(TStrStrPair(sAddress,sName));
   }
   return *this;
}

Ситуация 7

Есть, конечно, и ложные срабатывания. Вот, например, фрагмент кода из библиотеки zlib, включенной в проект TortoiseSVN. Ошибки здесь нет, но отметить такое место сообщением V501 очень полезно.

V501 There are identical sub-expressions to the left and to the right of the '-' operator: size - size zutil.c 213

voidpf zcalloc (opaque, items, size)
    voidpf opaque;
    unsigned items;
    unsigned size;
{
  /* make compiler happy */
  if (opaque) items += size - size;
  return (voidpf)calloc(items, size);
}

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

Ситуация 8

С кодировками есть и другие тёмные места. Вот еще одно условие, которое всегда ложно.

V547 Expression '* utf8CheckBuf >= 0xF5' is always false. The value range of signed char type: [-128, 127]. tortoiseblame.cpp 312

BOOL TortoiseBlame::OpenFile(const TCHAR *fileName)
{
  ...
  // check each line for illegal utf8 sequences.
  // If one is found, we treat
  // the file as ASCII, otherwise we assume
  // an UTF8 file.
  char * utf8CheckBuf = lineptr;
  while ((bUTF8)&&(*utf8CheckBuf))
  {
    if ((*utf8CheckBuf == 0xC0)||
        (*utf8CheckBuf == 0xC1)||
        (*utf8CheckBuf >= 0xF5))
    {
      bUTF8 = false;
      break;
    }
   ...
  }

Кстати, условия "*utf8CheckBuf == 0xC0", "*utf8CheckBuf == 0xC1" тоже всегда ложны. Поэтому код "bUTF8 = false;" никогда не получит управление. То, что анализатор PVS-Studio не стал ругать выражение "*utf8CheckBuf == 0xC0" является недоработкой. Записали в список улучшений. В следующей версии будем ругаться и на это.

Ситуация 9

Со следующим сообщением не все так просто. Теоретически в коде имеется ошибка. Практически - этот код работает.

V507 Pointer to local array 'stringbuf' is stored outside the scope of this array. Such a pointer will become invalid. mainwindow.cpp 277

LRESULT CALLBACK CMainWindow::WinMsgHandler(....)
{
  ...
  if (pNMHDR->code == TTN_GETDISPINFO)
  {
    LPTOOLTIPTEXT lpttt;

    lpttt = (LPTOOLTIPTEXT) lParam;
    lpttt->hinst = hResource;

    // Specify the resource identifier of the
    // descriptive text for the given button.
    TCHAR stringbuf[MAX_PATH] = {0};
    ...
    lpttt->lpszText = stringbuf;
  }
  ...
}

Диагностическое сообщение V507 предупреждает, что объект используется уже после окончания своего существования. Буфер 'stringbuf' будет использоваться уже после выхода из тела оператора 'if'.

Если бы 'stringbuf' был бы объектом класса, например std::string, то данный код вел бы себя некорректно. Мы бы использовали уже разрушенный объект. Но здесь 'stringbuf' является массивом, созданным в стеке. Компилятор Visual C++ повторно не использует данный участок стека, и буфер будет существовать до конца работы функции 'CMainWindow::WinMsgHandler'. Таким образом, никакой ошибки не возникнет, хотя код потенциально опасен.

Ситуация 10

И еще одно такое же место, как и в примере выше. Вновь это код, который работает, но хрупок.

V507 Pointer to local array 'stringbuf' is stored outside the scope of this array. Such a pointer will become invalid. picwindow.cpp 443

if ((HWND)wParam == m_AlphaSlider.GetWindow())
{
  LPTOOLTIPTEXT lpttt;

  lpttt = (LPTOOLTIPTEXT) lParam;
  lpttt->hinst = hResource;
  TCHAR stringbuf[MAX_PATH] = {0};
  _stprintf_s(stringbuf, .....);
  lpttt->lpszText = stringbuf;
}

Ситуация 11

Плохая идея бросать и не обрабатывать в деструкторе исключения.

V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. cachefileoutbuffer.cpp 52

CCacheFileOutBuffer::~CCacheFileOutBuffer()
{
  if (IsOpen())
  {
    streamOffsets.push_back (GetFileSize());
    size_t lastOffset = streamOffsets[0];
    for (size_t i = 1,
         count = streamOffsets.size();
         i < count; ++i)
    {
      size_t offset = streamOffsets[i];
      size_t size = offset - lastOffset;

      if (size >= (DWORD)(-1))
        throw CStreamException("stream too large");

      Add ((DWORD)size);
      lastOffset = offset;
    }

    Add ((DWORD)(streamOffsets.size()-1));
  }
}

Сообщение V509 предупреждает, что если объект CCacheFileOutBuffer разрушается в процессе обработки исключения, то новое исключение приведет к немедленному аварийному завершению программы.

Ситуация 12

Очередное сравнение, которое не имеет смысла.

V547 Expression 'endRevision < 0' is always false. Unsigned type value is never < 0. cachelogquery.cpp 999

typedef index_t revision_t;
...
void CCacheLogQuery::InternalLog (
   revision_t startRevision
 , revision_t endRevision
 , const CDictionaryBasedTempPath& startPath
 , int limit
 , const CLogOptions& options)
{
  ...
  // we cannot receive logs for rev < 0
  if (endRevision < 0)
    endRevision = 0;

  ...
}

Отрицательных значений здесь просто не может быть. Переменная endRevision имеет беззнаковый тип, а, следовательно, всегда endRevision >= 0. Потенциальная беда состоит в том, что если где-то раньше отрицательное значение превратилось в беззнаковый тип, то мы начнем работать с очень большим числом. Для этого ведь проверки нет.

Ситуация 13

Больше полезных сообщений первого уровня нет. Пока нет. Ведь это только первая проба пера новых возможностей PVS-Studio. У нас записано в план развития не менее 150 примеров, которые стоит научиться выявлять. Еще раз спасибо читателям, кто откликнулись на предыдущие статьи и присылали примеры, в которых можно выявить ошибки статическим анализом на этапе написания кода.

Заглянем на второй уровень. Мы обнаружим одну интересную ошибку, связанную с использованием методики Copy-Paste. Кстати, на третьем уровне не оказалось вообще ничего интересного, так что не зря мы его по умолчанию отключаем.

V524 It is odd that the 'GetDbgHelpVersion' function is fully equivalent to the 'GetImageHlpVersion' function (SymbolEngine.h, line 98). symbolengine.h 105

BOOL GetImageHlpVersion(DWORD &dwMS, DWORD &dwLS)
{
  return(GetInMemoryFileVersion(("DBGHELP.DLL"),
                                dwMS,
                                dwLS)) ;
}

BOOL GetDbgHelpVersion(DWORD &dwMS, DWORD &dwLS)
{
  return(GetInMemoryFileVersion(("DBGHELP.DLL"),
                                dwMS,
                                dwLS)) ;
}

Сообщение V524 выдается в том случае, если обнаруживаются две подозрительные одинаковые функции. Скорее всего, первая функция должна получать версию файла "imagehlp.dll", а вовсе не "dbghelp.dll ".

Шаг 8

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

Что касается найденных ошибок, то мы сообщим о них разработчикам TortoiseSVN.

Шаг 9

Теперь поговорим немного о ложных срабатываниях. Приведем некоторые примеры, чтобы пояснить, что такое ложное срабатывание и как с ними можно поступить.

Первое ложное срабатывание. PVS-Studio не разобрался в играх с копированием памяти.

V512 A call of the 'memcpy' function will lead to a buffer overflow or underflow. resmodule.cpp 838

const WORD*
CResModule::CountMemReplaceMenuExResource(....)
{
  ...
  if (newMenu != NULL) {
    CopyMemory(&newMenu[*wordcount], p0, 7 * sizeof(WORD));
  }
   ...
}

Предупреждение V512 должно указывать на то, что буфер обработан не полностью или что, наоборот, мы вышли за его пределы. Сейчас анализатор ошибся, решив, что мы будем работать только с одним объектом типа WORD, а скопировать хотим 7 объектов.

Второе ложное срабатывание. Здесь анализатор считает, что обработали только часть массива.

V512 A call of the 'memcmp' function will lead to a buffer overflow or underflow. sshsha.c 317

static int sha1_96_verify(....)
{
  unsigned char correct[20];
  sha1_do_hmac(handle, blk, len, seq, correct);
  return !memcmp(correct, blk + len, 12);
}

Действительно сравнивается только часть массива 'correct', но так и было задумано.

Третий пример ложного срабатывания.

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. tree234.c 195

static void *add234_internal(....)
{
  ...
  if ((c = t->cmp(e, n->elems[0])) < 0)
    childnum = 0;
  else if (c == 0)
    return n->elems[0]; /* already exists */
  else if (n->elems[1] == NULL
       || (c = t->cmp(e, n->elems[1])) < 0)
         childnum = 1;
  else if (c == 0)
    return n->elems[1]; /* already exists */
  else if (n->elems[2] == NULL
       || (c = t->cmp(e, n->elems[2])) < 0)
         childnum = 2;
  else if (c == 0)
    return n->elems[2]; /* already exists */
  else
    childnum = 3;
  ...
}

Анализатору не нравится, что несколько раз присутствует проверка 'c == 0'. Код корректен, так как переменная 'c' меняется внутри других условий "c = t->cmp(e, n->elems[2])". Однако это достаточно редкая ситуация. Чаще сообщение V517 указывает на настоящие дефекты в коде.

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

Поступать с ложными срабатываниями можно по-разному:

1) Можно переписать код. Иногда это вполне разумно. Последнему примеру с ложным срабатыванием рефакторинг вовсе не повредит (речь идет про функцию add234_internal и предупреждение V517).

2) Можно отключить в настройках некоторые проверки, которые в ваших проектах всегда дают ложное срабатывание. После отключения, из таблицы сразу исчезнут все эти сообщения. Подробнее здесь: "Settings: Detectable Errors".

3) Если ложные срабатывания относятся к коду, которые не стоит проверять, то можно исключить отдельные файлы или папки. Можно использовать маски. Подробнее здесь "Settings: Don't Check Files". Это удобно, чтобы исключать из анализа сторонние библиотеки.

4) Можно использовать механизм подавления сообщений, содержащих определенный текст. Подробнее здесь: "Settings: Message Suppression".

5) Есть ситуации, когда следует подавить конкретное предупреждение. В этом случае можно использовать функцию "Mark as False Alarm" ("Пометить как ложное срабатывание"). В этом случае в код вставляется маленький комментарий вида: "//-Vкод_ошибки". Размеченные таким образом участки кода могут быть скрыты в списке ошибок. Для этого служит кнопка FA, включающая/выключающая показ помеченных сообщений. Подробнее здесь: "Подавление ложных предупреждений".

Всем спасибо за внимание. Пробуйте PVS-Studio. Присылайте свои отзывы. Задавайте вопросы. Покажите примеры, если у вас найдется что-то интересное. Предлагайте новые диагностические правила для реализации.

С уважением, Андрей Карпов, один из разработчиков PVS-Studio.

Вы можете связаться с нами с помощью страницы "Обратная связь".

Или по E-mail: support[@]viva64.com , karpov[@]viva64.com.

Пишите нам, у нас замечательная поддержка. Я лично, автор статьи, приму участие в общении, а не абстрактный человек-робот, как бывает в большинстве компаний.