Andrey2008 Feb 13 2012 at 14:28

Повторная проверка проекта Notepad++

7 min

2.3K

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

Введение

Итак, мы проверили проект Notepad++ взятый из репозитория 31 января 2012. Для проверки использовался анализатор PVS-Studio версии 4.54.

Как уже было сказано, мы ранее проверяли этот проект. Ошибок нашли не много, но всё-таки что-то нашли. В новой версии проекта часть старых ошибок исправлена, а часть нет. Это странно. По всей видимости, прежняя заметка осталась незамеченной авторами Notepad++ и они не воспользовались PVS-Studio для проверки проекта. Возможно эта заметка все-таки привлечет авторов Notepad++, тем более что мы недавно изменили режим триала и все найденные с помощью PVS-Studio ошибки видны.

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

Например, исправлена ошибка, описанная в предыдущей статье, которая касалась заполнения массива _iContMap. Было так:

memset(_iContMap, -1, CONT_MAP_MAX);

Исправленный вариант:

memset(_iContMap, -1, CONT_MAP_MAX * sizeof(int));

Зато вот эта ошибка, продолжает жить и здравствовать:

bool isPointValid() {
  return _isPointXValid && _isPointXValid;
};

Диагностическое сообщение анализатора PVS-Studio:

V501 There are identical sub-expressions to the left and to the right of the '&&' operator: _isPointXValid && _isPointXValid Notepad++ parameters.h 166

Мы не будем возвращаться к тем ошибкам, которые были описаны в предыдущей статье. Рассмотрим то новое, что научился диагностировать анализатор PVS-Studio за прошедшее время:

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

Новые найденные ошибки

Ошибка N1. Выход за границы массива

int encodings[] = {
  1250, 
  1251, 
  1252, 
  ....
};

BOOL CALLBACK DefaultNewDocDlg::run_dlgProc(
  UINT Message, WPARAM wParam, LPARAM)
{
  ...
  for (int i = 0 ; i <= sizeof(encodings)/sizeof(int) ; i++)
  {
    int cmdID = em->getIndexFromEncoding(encodings[i]);
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V557 Array overrun is possible. The value of 'i' index could reach 46. Notepad++ preferencedlg.cpp 984

В цикле перебираются элементы массива «encodings». Ошибка заключается в неправильном условии остановки цикла. При последней итерации цикла происходит доступ к памяти за границей массива. Ошибку можно исправить, заменив условие "<=" на "<". Корректный код:

for (int i = 0 ; i < sizeof(encodings)/sizeof(int) ; i++)

Ошибка N2. Неправильное вычисление размера буфера

typedef struct tagTVITEMA {
  ...
  LPSTR     pszText;
  ...
} TVITEMA, *LPTVITEMA;

#define TVITEM TVITEMA

HTREEITEM TreeView::addItem(...)
{
  TVITEM tvi;
  ...
  tvi.cchTextMax =
    sizeof(tvi.pszText)/sizeof(tvi.pszText[0]); 
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V514 Dividing sizeof a pointer 'sizeof (tvi.pszText)' by another value. There is a probability of logical error presence. Notepad++ treeview.cpp 88

Размер буфера вычисляется с помощью выражения «sizeof(tvi.pszText)/sizeof(tvi.pszText[0])». Это выражение не имеет смысла. В нём размер указателя делится на размер одного символа. Как исправить код сказать сложно, так как мы не знакомы с логикой работы программы.

Ошибка N3. Неправильная проверка, что строка пустая

size_t Printer::doPrint(bool justDoIt)
{
  ...
  TCHAR headerM[headerSize] = TEXT("");
  ...
  if (headerM != '\0')
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *headerM != '\0'. Notepad++ printer.cpp 380

Указатель сравнивается с нулевым значением. Ноль задан литералом вида '\0'. Это подсказывает нам, что здесь забыто разыменование указателя. Корректный код:

if (*headerM != '\0')

Аналогичная ошибка допущена в другом месте:

V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *headerR != '\0'. Notepad++ printer.cpp 392

Ошибка N4. Опечатка в условии

DWORD WINAPI Notepad_plus::threadTextPlayer(void *params)
{
  ...
  const char *text2display = ...;
  ...
  if (text2display[i] == ' ' && text2display[i] == '.')
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V547 Expression is always false. Probably the '||' operator should be used here. Notepad++ notepad_plus.cpp 4967

Условие (text2display[i] == ' ' && text2display[i] == '.') никогда не выполняется. Символ не может одновременно являться и пробелом и точкой. Видимо здесь мы имеем дело с простой опечаткой и код должен был выглядеть следующим образом:

if (text2display[i] == ' ' || text2display[i] == '.')

Аналогичная ошибка допущена в другом месте:

V547 Expression is always false. Probably the '||' operator should be used here. Notepad++ notepad_plus.cpp 5032

Ошибка N5. Опечатка в условии

int Notepad_plus::getHtmlXmlEncoding(....) const
{
  ...
  if (langT != L_XML && langT != L_HTML && langT == L_PHP)
    return -1;
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. Notepad++ notepad_plus.cpp 853

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

if (langT == L_PHP)

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

if (langT != L_XML && langT != L_HTML && langT != L_PHP)

Ошибка N6. Неправильная работа с битами

TCHAR GetASCII(WPARAM wParam, LPARAM lParam)
{
  ...
  result=ToAscii(wParam,(lParam >> 16) && 0xff,
    keys,&dwReturnedValue,0);
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V560 A part of conditional expression is always true: 0xff. Notepad++ babygrid.cpp 694

В коде хотят извлечь третий байт из переменной 'lParam'. Из-за опечатки, код делает совсем не это. Ошибка заключается в том, что используется оператор '&&' вместо '&'. Корректный код:

result=ToAscii(wParam,(lParam >> 16) & 0xff,
  keys,&dwReturnedValue,0);

Ошибка N7. Недописанный код

#define SCE_T3_BRACE 20
static inline bool IsAnOperator(const int style) {
  return style == SCE_T3_OPERATOR || SCE_T3_BRACE;
}

Диагностическое сообщение анализатора PVS-Studio:

V560 A part of conditional expression is always true: 20. lextads3.cxx 700

Функция IsAnOperator() всегда возвращает 'true'. Корректный код:

return style == SCE_T3_OPERATOR ||
            style == SCE_T3_BRACE;

Ошибка N8. Код, который никогда не будет выполнен

static void ColouriseVHDLDoc(....)
{
  ...
      } else if (sc.Match('-', '-')) {
        sc.SetState(SCE_VHDL_COMMENT);
        sc.Forward();
      } else if (sc.Match('-', '-')) {
        if (sc.Match("--!"))
          sc.SetState(SCE_VHDL_COMMENTLINEBANG);
        else
          sc.SetState(SCE_VHDL_COMMENT);
      }
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 130, 133. lexvhdl.cxx 130

Если первое условие (sc.Match('-', '-')) истинно, то вторая проверка выполнена не будет. Это приведет к тому, что случай последовательность символов "--!" никогда не будет обработана правильно. По всей видимости, здесь часть кода лишняя и должно быть написано так:

static void ColouriseVHDLDoc(....)
{
  ...
      } else if (sc.Match('-', '-')) {
        if (sc.Match("--!"))
          sc.SetState(SCE_VHDL_COMMENTLINEBANG);
        else
          sc.SetState(SCE_VHDL_COMMENT);
      }
  ...
}

Ошибка N9. Лишний код

Есть фрагменты кода, которые не приводят к проблемам, но являются избыточными. Приведем два примера такого типа:

void Gripper::doTabReordering(POINT pt)
{
  ...
  else if (_hTab == hTabOld)
  {
    /* delete item on switch between tabs */
    ::SendMessage(_hTab, TCM_DELETEITEM, iItemOld, 0);
  }
  else
  {
    if (_hTab == hTabOld)
    {
      /* delete item on switch between tabs */
      ::SendMessage(_hTab, TCM_DELETEITEM, iItemOld, 0);
    }
  }
  ...
}

Диагностическое сообщение анализатора PVS-Studio:

V571 Recurring check. The 'if (_hTab == hTabOld)' condition was already verified in line 478. Notepad++ gripper.cpp 485

Вторая часть кода не имеет никакого смысла и может быть удалена.

А вот другой пример, где присутствуют лишние проверк. Указатели 'mainVerStr' и 'auxVerStr' всегда не равны нулю, так как это массивы, созданные на стеке:

LRESULT Notepad_plus::process(....)
{
  ...
  TCHAR mainVerStr[16];
  TCHAR auxVerStr[16];
  ...
  if (mainVerStr)
    mainVer = generic_atoi(mainVerStr);
  if (auxVerStr)
    auxVer = generic_atoi(auxVerStr);
  ...
}

Здесь можно написать проще:

mainVer = generic_atoi(mainVerStr);
auxVer = generic_atoi(auxVerStr);

Проверки, показанные выше, встречаются в проекте Notepad++ неоднократно:

V600 Consider inspecting the condition. The 'mainVerStr' pointer is always not equal to NULL. Notepad++ nppbigswitch.cpp 938

V600 Consider inspecting the condition. The 'auxVerStr' pointer is always not equal to NULL. Notepad++ nppbigswitch.cpp 940

V600 Consider inspecting the condition. The 'intStr' pointer is always not equal to NULL. Notepad++ preferencedlg.cpp 1871

V600 Consider inspecting the condition. The 'intStr' pointer is always not equal to NULL. Notepad++ userdefinedialog.cpp 222

V600 Consider inspecting the condition. The 'intStr' pointer is always not equal to NULL. Notepad++ wordstyledlg.cpp 539

Выводы

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

Зачем подолгу искать странное поведение программы, из-за неочищенного массива? Код «memset(_iContMap, -1, CONT_MAP_MAX)» легко и быстро находится статическим анализатором.

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

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

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

Tags:

Hubs:

PVS-Studio corporate blog