Статический анализ исходного кода на примере WinMerge

30 Окт 2010

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

Сегодня я хочу посвятить пост тематике, почему инструменты анализа исходного кода полезны вне зависимости от уровня знаний и опыта программиста. А польза такого анализа будет продемонстрирована на примере инструмента, который известен всем программистам - WinMerge.

Чем раньше ошибка в коде приложения будет обнаружена, тем дешевле стоит ее исправление. Отсюда следует вывод, что наиболее дешево и просто ошибка может быть устранена в процессе написания кода. А еще лучше, если ошибка вовсе не будет написана. Вот только захотел сделать ошибку, так сразу хлоп себя по рукам и код написан уже правильно. Но так как-то не получается. Подход "надо писать без ошибок" все равно не работает.

Даже высококвалифицированный программист, который никуда не торопится, совершает ошибки, начиная от простейших опечаток и кончая логическими ошибками в алгоритмах. Здесь срабатывает закон больших чисел. Вот вроде в каждом конкретном операторе "if" сделать ошибку невозможно. А написал 200 сравнений, и один раз, да ошибся. Про это интересно рассказывает Андрей Уразов в своем докладе "Программирование, ориентированное на качество" на конференции CodeFest 2010. Кратко я хочу привести следующую его мысль. Как бы ни были опытны разработчики, ошибки все равно появляются в коде. Эти ошибки невозможно прекратить делать. Но со многими из них можно успешно бороться на гораздо более раннем этапе, чем это делается обычно.

Обычно самым первым уровнем обороны от ошибок является создание юнит-тестов на вновь написанный код. Иногда тесты пишутся еще до кода, который они будут проверять. Однако у юнит-тестов есть свой ряд недостатков, которые я не буду подробно рассматривать, так как они и так известны программистам. Не всегда легко создать юнит-тест для функции, которая требует большой предварительной подготовки данных. Юнит-тесты становятся обузой, если требования к проекту быстро меняются. Тесты отнимают много времени на написание и сопровождение. Тестами не всегда просто покрыть все ветвления. А еще вы можете получить в подарок монолитный проект, в котором юнит-тестов просто не существует и не планировалось. Не отрицая огромной пользы юнит-тестов, я считаю, что хотя это хороший оборонительный рубеж, его можно и стоит существенно укрепить.

Часто программисты пренебрегают еще более ранним уровнем обороны - статическим анализом кода. Многие используют возможности анализа кода, не выходя за рамки диагностических предупреждений выдаваемых компиляторами. А между тем существует целый класс инструментов, позволяющих выявить на этапе кодирования значительный процент логических ошибок и простых опечаток. Эти инструменты осуществляют более высокоуровневую проверку кода, опираясь на знание некоторых паттернов кодирования, используют эвристические алгоритмы, имеют гибкую систему настройки.

У статического анализа, конечно тоже, есть недостатки. Многие виды ошибок он просто не в состоянии обнаружить. Анализаторы дают ложные срабатывания и заставляют вносить в код такие правки, чтобы этот код им понравился и был затем оценен как безопасный.

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

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

Я хотел выбрать что-то небольшое и общеизвестное. Поскольку я использую не так много инструментов, то листая список программ в меню "Пуск", мой выбор остановился на WinMerge. Программа WinMerge доступна в исходных кодах, невелика (около 186000 строк). Программа достаточна качественная. Говорю на основании того, что я пользуюсь ей без нареканий, и что 25% исходного кода занимают комментарии (хороший признак). В общем, оптимальный выбор.

Была скачена последняя доступная версия 2.13.20 (от 20.10.2010). Для анализа я воспользовался разрабатываемым нами прототипом анализатора общего назначения. Остановлюсь на этом чуть подробнее.

Сейчас в состав статического анализатора PVS-Studio входит два набора правил. Один предназначен для выявления 64-битных дефектов, а другой предназначен для проверки OpenMP программ. В настоящее время мы занимаемся разработкой набора правил общего назначения. Пока не доступна даже beta-версия, но уже что-то работает и мне очень хочется хоть немного реальной войны с ошибками. Мы планируем сделать новый набор правил бесплатным, так что просьба не писать про саморекламу. Общественности мы представим новый инструмент через 1-2 месяца в рамках PVS-Studio 4.00.

Итак, вот некоторые интересные моменты, которые я обнаружил в исходном коде WinMerge-2.13.20 в течение получаса (15 минут проверка, 15 минут просмотр результатов). Есть и другие подозрительные места, но разобраться, есть ли там действительно ошибка или нет - требует усилий. Сейчас у меня нет задачи найти как можно больше дефектов в одном проекте. Хочется изящно показать, чем полезен статический анализ и как даже беглым изучением можно быстро выявить ряд ошибок.

Пример первый. Анализатор указал мне на несколько ошибок "V530 - The return value of function 'Foo' is required to be utilized". Обычно эти предупреждения возникают в связи с неверным использованием функций. Рассмотрим фрагмент кода:

/**
* @brief Get the file names on both sides for specified item.
* @note Return empty strings if item is special item.
*/
void CDirView::GetItemFileNames(int sel,
  String& strLeft, String& strRight) const
{
  UINT_PTR diffpos = GetItemKey(sel);
  if (diffpos == (UINT_PTR)SPECIAL_ITEM_POS)
  {
    strLeft.empty();
    strRight.empty();
  }
  else
  {
     ...
  }
}

Функция должна вернуть в определенной ситуации две пустых строки. Однако по невнимательности вместо std::string::clear() вызываются функции std::string::empty(). Это, кстати, не такая редкая ошибка, как может показаться. Я встречал ее во многих других проектах. В том числе она есть и в другой функции WinMerge:

/**
* @brief Clear variant's value (reset to defaults).
*/
void VariantValue::Clear()
{
  m_vtype = VT_NULL;
  m_bvalue = false;
  m_ivalue = 0;
  m_fvalue = 0;
  m_svalue.empty();
  m_tvalue = 0;
}

Здесь опять не происходит ожидаемой очистки строки.

А вот сработало предупреждение "V501 - There are identical sub-expressions to the left and to the right of the '||' operator":

BUFFERTYPE m_nBufferType[2];
...
// Handle unnamed buffers
if ((m_nBufferType[nBuffer] == BUFFER_UNNAMED) ||
    (m_nBufferType[nBuffer] == BUFFER_UNNAMED))
  nSaveErrorCode = SAVE_NO_FILENAME;

Если посмотреть код рядом, то по аналогии здесь должно быть написано:

(m_nBufferType[0] == BUFFER_UNNAMED) ||
(m_nBufferType[1] == BUFFER_UNNAMED)

А если и не так, то все равно имеется какая-то ошибка.

При возникновении различных аварийных ситуаций WinMerge попробует сообщить об ошибках, но во многих случаях у него это плохо получится. Это кстати хороший пример, как анализатор кода может выявлять ошики в редко используемых участках программы. В коде имеется несколько ошибок, о которых PVS-Studio сообщает так: "V510 - The 'Format' function is not expected to receive class-type variable as 'N' actual argument". Пример кода:

String GetSysError(int nerr);
...
CString msg;
msg.Format(
_T("Failed to open registry key HKCU/%s:\n\t%d : %s"),
f_RegDir, retVal, GetSysError(retVal));

На первый взгляд все хорошо. Вот только тип "String" есть не что иное как "std::wstring". А следовательно, в лучшем случае мы распечатаем абракадабру, а в худшем произойдет ошибка доступа к памяти (Access Violation). Вместо указателя на строку в стек помещается объект типа "std::wstring". Подробнее данную ситуацию я описывал в заметке "Большой брат помогает тебе". Корректный код должен содержать вызов c_str():

msg.Format(
_T("Failed to open registry key HKCU/%s:\n\t%d : %s"),
f_RegDir, retVal, GetSysError(retVal).c_str());

Пойдем дальше. Вот обнаружен весьма подозрительный код. Есть здесь ошибка или нет, я не знаю. Но странно, что две ветки оператора "if" содержат полностью идентичный код. Анализатор предупредил об этой ситуации диагностическим сообщением "V532 - The 'then' statement is equivalent to the 'else' statement". Вот он этот подозрительный код:

if (max < INT_MAX)
{
  for (i = min; i < max; i++)
  {
    if (eptr >= md->end_subject ||
        IS_NEWLINE(eptr))
      break;
    eptr++;
    while (eptr < md->end_subject &&
           (*eptr & 0xc0) == 0x80)
      eptr++;
    }
  }
else
{
  for (i = min; i < max; i++)
  {
    if (eptr >= md->end_subject ||
        IS_NEWLINE(eptr))
      break;
    eptr++;
    while (eptr < md->end_subject &&
           (*eptr & 0xc0) == 0x80)
      eptr++;
    }
  }
}

Вот чувствую что здесь: "Это ж-ж-ж неспроста".

Ну и еще один пример и завершим, пожалуй. Анализатор нашел подозрительный цикл: "V534 - It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i'." Исходный код:

// Get length of translated array of bytes from text.
int Text2BinTranslator::iLengthOfTransToBin(
  char* src, int srclen )
{
  ...
    for (k=i; i<srclen; k++)
    {
      if (src[k]=='>')
        break;
    }
  ...
}

Этот код предрасположен к Access Violation. Цикл должен продолжаться пока не найдется символ '>' или не закончится строка длиной в 'srclen' символов. Вот только случайно для сравнения использована переменная 'i', а не 'k'. Если символ '>' найден не будет, то все вероятно закончится печально.

Заключение

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

#Cpp