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

15 Июн 2014

Теги:

#Cpp

Легко и просто проверяем Firefox с помощью PVS-Studio Standalone

15 Июн 2014

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

Mozilla Firefox
PVS-Studio Standalone
Результаты проверки Mozilla Firefox
Более тщательная проверка
Заключение
Дополнительные ссылки

Три года назад мы уже проверяли Mozilla Firefox с помощью анализатора PVS-Studio. Тогда это было неудобно и затруднительно. Для Firefox отсутствует проектный файл для Visual Studio. Сборка осуществляется с помощью make-файлов. Поэтому просто взять и проверить проект нельзя. Требовалось интегрировать PVS-Studio в систему сборки, что оказалось трудной задачей. В результате, как мне помнится, была проанализирована только часть проекта. Но всё поменялось, когда появился PVS-Studio Standalone. Теперь можно отследить все запуски компиляторов и легко проверить проект.

Mozilla Firefox

Думаю, рассказывать про Firefox нет необходимости. Однако, формат статьи требует всё-таки написать немного о проверенном проекте. Поленюсь и воспользуюсь описанием из Wikipedia:

Mozilla Firefox - свободный браузер на движке Gecko, разработкой и распространением которого занимается Mozilla Corporation. Третий по популярности браузер в мире и первый среди свободного ПО - в августе 2013 года его рыночная доля составила 19,26%. В браузере присутствует интерфейс со многими вкладками, проверка орфографии, поиск по мере набора, "живые закладки", менеджер закачек, поле для обращения к поисковым системам. Новые функции можно добавлять при помощи расширений.

Мы уже пытались проверить Firefox. Частично это нам даже удалось. По результатам проверки была написана статья "Как уменьшить вероятность ошибки на этапе написания кода. Заметка N4". Сложность заключалась в том, что нужно вставить вызов command-line версии PVS-Studio в make-файлы. Сделать это в большом незнакомом проекте бывает затруднительно. Именно поэтому мы в дальнейшем не делали попыток перепроверить Firefox. Ситуация изменилась с появлением PVS-Studio Standalone.

PVS-Studio Standalone

PVS-Studio Standalone может быть использован в 2 режимах:

Удобная работа с файлом отчета (*.plog), который содержит информацию о найденных ошибках на компьютере, где не установлен Visual Studio.
Отслеживание запуска компилятора и сбор всей необходимой информации для дальнейшей проверки файлов. Нам сейчас интересен именно этот режим работы.

Теперь не обязательно интегрировать command-line версию PVS-Studio в make-файлы. Можно проверить Firefox намного проще. Мы именно так и сделали. Последовательность действий:

Запускаем программу PVS-Studio Standalone;
Выполняем команду "Compiler Monitoring";
Компилируем проект Firefox;
Останавливаем процедуру слежения ("Stop Monitoring");
Запускается проверка файлов;
Изучаем предупреждения, выданные анализатором кода.

Более подробно, как использовать этот режим, можно прочитать здесь.

Результаты проверки Mozilla Firefox

Проект Firefox очень качественный. В добавок, как я понимаю, при его разработке уже применяются инструменты статического анализ кода: Coverity и Klocwork. По крайней мере, я встречал упоминания этих анализаторов в некоторых файлах.

Поэтому найти хоть что-то - это уже достижение. Давайте посмотрим, какие предупреждения анализатора PVS-Studio показались мне интересными.

Опечатка N1

NS_IMETHODIMP
nsNativeThemeWin::WidgetStateChanged(....)
{
  ....
  if (aWidgetType == NS_THEME_WINDOW_TITLEBAR ||
      aWidgetType == NS_THEME_WINDOW_TITLEBAR_MAXIMIZED ||
      aWidgetType == NS_THEME_WINDOW_FRAME_LEFT ||
      aWidgetType == NS_THEME_WINDOW_FRAME_RIGHT ||
      aWidgetType == NS_THEME_WINDOW_FRAME_BOTTOM ||
      aWidgetType == NS_THEME_WINDOW_BUTTON_CLOSE ||
      aWidgetType == NS_THEME_WINDOW_BUTTON_MINIMIZE ||   <<<===
      aWidgetType == NS_THEME_WINDOW_BUTTON_MINIMIZE ||   <<<===
      aWidgetType == NS_THEME_WINDOW_BUTTON_RESTORE) {
    *aShouldRepaint = true;
    return NS_OK;
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'aWidgetType == 237' to the left and to the right of the '||' operator. nsnativethemewin.cpp 2475

Переменная 'aWidgetType' два раза сравнивается с константой NS_THEME_WINDOW_BUTTON_MINIMIZE. Это опечатка. Второй раз переменную нужно сравнить с константой NS_THEME_WINDOW_BUTTON_MAXIMIZE.

Опечатка N2

bool nsHTMLCSSUtils::IsCSSEditableProperty(....)
{
  ....
  if (aAttribute && aAttribute->EqualsLiteral("align") &&
      (nsEditProperty::ul == tagName          <<<<====
       || nsEditProperty::ol == tagName
       || nsEditProperty::dl == tagName
       || nsEditProperty::li == tagName
       || nsEditProperty::dd == tagName
       || nsEditProperty::dt == tagName
       || nsEditProperty::address == tagName
       || nsEditProperty::pre == tagName
       || nsEditProperty::ul == tagName)) {   <<<<====
    return true;
  }
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'nsEditProperty::ul == tagName' to the left and to the right of the '||' operator. nshtmlcssutils.cpp 432

Переменная 'tagName' два раза сравнивается с nsEditProperty::ul. Возможно, одна проверка лишняя. Или забыли сравнить с чем-то ещё.

Опечатка N3

void Reverb::process(....)
{
  ....
  bool isCopySafe =
    destinationChannelL &&
    destinationChannelR &&
    size_t(destinationBus->mDuration) >= framesToProcess &&
    size_t(destinationBus->mDuration) >= framesToProcess;
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'size_t (destinationBus->mDuration) >= framesToProcess' to the left and to the right of the '&&' operator. reverb.cpp 192

Переменная 'framesToProcess' два раза сравнивается с 'size_t(destinationBus->mDuration)'.

Опечатка N4

float
PannerNode::ComputeDopplerShift()
{
  ....
  double scaledSpeedOfSound = listener->DopplerFactor() /
                              listener->DopplerFactor();
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'listener->DopplerFactor()' to the left and to the right of the '/' operator. pannernode.cpp 529

Очень подозрительное выражение, которое стоит проверить.

Опечатка N5

bool DataChannelConnection::SendDeferredMessages()
{
  ....
  if ((result = usrsctp_sendv(mSocket, data, ...., 0) < 0)) {
  ....
}

Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. datachannel.cpp 1105

Не там поставлена скобка. Упростим выражение, чтобы ошибка была лучше заметна:

if ((result = foo() < 0))

Это выражение вычисляется так. Результат, который вернула функция, сравнивается с 0. Затем true или false записывается в переменную 'result'. Ошибка в том, что не там закрывается одна из скобок. На самом деле, программист хотел написать выражение:

if ((result = foo()) < 0)

Теперь значение, которое вернула функция, записывается в переменную 'result'. И только потом это значение сравнивается с нулём.

Опечатка N6

void nsRegion::SimplifyOutwardByArea(uint32_t aThreshold)
{
  ....
  topRects = destRect;
  bottomRects = bottomRectsEnd;
  destRect = topRects;
  ....
}

Предупреждение PVS-Studio: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 358, 360. nsregion.cpp 360

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

Некорректная проверка N1

enum nsBorderStyle {
  eBorderStyle_none = 0,
  ....
};
....
NS_IMETHODIMP
nsWindow::SetNonClientMargins(nsIntMargin &margins)
{
  if (!mIsTopWidgetWindow ||
      mBorderStyle & eBorderStyle_none ||
      mHideChrome)
    return NS_ERROR_INVALID_ARG;
  ....
}

Предупреждение PVS-Studio: V616 The 'eBorderStyle_none' named constant with the value of 0 is used in the bitwise operation. nswindow.cpp 2278

Выражение "mBorderStyle & eBorderStyle_none" не имеет смысла. Отсутствие стилей (eBorderStyle_none) кодируется значением 0. По все видимости, код условие следует записать так:

if (!mIsTopWidgetWindow ||
    mBorderStyle != eBorderStyle_none ||
    mHideChrome)

Некорректная проверка N2

NS_IMETHODIMP nsWindowsRegKey::ReadStringValue(....)
{
  ....
  DWORD type;
  ....
  if (type != REG_SZ && type == REG_EXPAND_SZ &&
      type == REG_MULTI_SZ)
    return NS_ERROR_FAILURE;
  ....
}

Предупреждение PVS-Studio: V547 Expression is always false. Probably the '||' operator should be used here. nswindowsregkey.cpp 292

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

if (... && type == 2 && type == 7)

Это условие всегда ложно.

Скорее всего, код должен быть таким:

if (type != REG_SZ && type != REG_EXPAND_SZ &&
    type != REG_MULTI_SZ)

Некорректная проверка N3

const SafepointIndex *
IonScript::getSafepointIndex(uint32_t disp) const
{
  ....
  size_t minEntry = 0;
  ....
  size_t guess = ....;
  ....
  while (--guess >= minEntry) {
    guessDisp = table[guess].displacement();
    JS_ASSERT(guessDisp >= disp);
    if (guessDisp == disp)
      return &table[guess];
  }
  ....
}

Предупреждение PVS-Studio: V547 Expression '-- guess >= minEntry' is always true. Unsigned type value is always >= 0. ion.cpp 1112

Цикл остановится только тогда, когда будет найден нужный элемент. Если такого элемента нет, условие остановки цикла никогда не выполнится, и произойдёт выход за границы массива.

Причина в том, что переменная 'guess' имеет беззнаковый тип. Это значит, что условие (‑‑guess >= 0) всегда истинно.

Невнимательность N1

void WinUtils::LogW(const wchar_t *fmt, ...)
{
  ....
  char* utf8 = new char[len+1];
  memset(utf8, 0, sizeof(utf8));
  ....
}

Предупреждение PVS-Studio: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. winutils.cpp 146

Выражение 'sizeof(utf8)' возвращает размер указателя, а не размер выделенного буфера памяти. Правильный вариант кода:

memset(utf8, 0, sizeof(*utf8) * (len+1));

Невнимательность N2

Как всегда встречается код, где указатель в начале используется, а только затем проверяется на равенство нулю. Приведу в статье только один такой случай. Остальные ошибки авторы Firefox смогут найти, запустив анализатор.

void
nsHttpTransaction::RestartVerifier::Set(
  int64_t contentLength, nsHttpResponseHead *head)
{
  if (mSetup)
    return;

  if (head->Status() != 200)    <<<<====
    return;

  mContentLength = contentLength;

  if (head) {                   <<<<====
  ....
}

Предупреждение PVS-Studio: V595 The 'head' pointer was utilized before it was verified against nullptr. Check lines: 1915, 1920. nshttptransaction.cpp 1915

В начале указатель 'head' разыменовывается в выражении "head->Status()". А затем он проверяется на равенство нулю.

Невнимательность N3

NPError NPP_New(....)
{
  ....
  InstanceData* instanceData = new InstanceData;
  ....
  NPError err = pluginInstanceInit(instanceData);
  if (err != NPERR_NO_ERROR) {
    NPN_ReleaseObject(scriptableObject);
    free(instanceData);
    return err;
  }
  ....
}

Предупреждение PVS-Studio: V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'instanceData' variable. nptest.cpp 1029

Память выделяется с помощью оператора 'new', а освобождается вызовом функции 'free'. Результат - неопределённое поведение программы. Впрочем, это не страшно, так как код относится к тестам.

Невнимательность N4

Ещё один фрагмент кода, относящийся к тестам. Переменная 'device' может остаться неинициализированной:

static ID3D10Device1* getD3D10Device()
{
  ID3D10Device1 *device;
  ....
  if (createDXGIFactory1)
  {
    ....
    hr = createD3DDevice(...., &device);
    ....
  }
  return device;
}

Предупреждение PVS-Studio: V614 Potentially uninitialized pointer 'device' used. nptest_windows.cpp 164

Более тщательная проверка

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

Цель статьи показать возможности статического анализа, и привлечь внимание людей к нашему инструменту. Более тщательный анализ Firefox могут осуществить сами разработчики. Им будем намного легче понять, является что-то ошибкой или нет.

Примечание для разработчиков Firefox. Проект весьма большой, поэтому анализатор PVS-Studio выдаёт много ложных срабатываний. Однако, большинство из них относятся к специфическим макросам. Можно очень быстро сократить количество ложных предупреждений в несколько раз, расставив соответствующие комментарии в коде. Как подавить предупреждения, относящиеся к определённым макросам, описано в документации (см. раздел "Подавление ложных предупреждений"). Если разработчики Firefox заинтересуются приобретением лицензии на PVS-Studio, мы со своей стороны также готовы принять участие в сокращении ложных срабатываний.

Заключение

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

Желаю всем успехов в программировании и поменьше ошибок.

Дополнительные ссылки

Анализатор PVS-Studio. Найди массу глупых ошибок в процессе написания кода. Сэкономь время команды. У вас не делают глупых ошибок? Ха-ха!
Приглашаем вас подписаться на наш твиттер: @Code_Analysis. Регулярно публикуем ссылки на интересные статьи по программированию и о проверке новых проектов.

#Cpp