ReactOS and PVS-Studio code analyzer

Обсуждаем ReactOS по-русски

Moderators: fog, fireball

Jedi-to-be
Posts: 698
Joined: Sun Mar 16, 2008 11:26 am
Location: Russia, Stavropol
Contact:

ReactOS and PVS-Studio code analyzer

Post by Jedi-to-be » Thu Sep 01, 2011 9:46 am

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

Control Frik
Posts: 64
Joined: Wed Jan 20, 2010 9:46 am

Re: Анализ кода ReactOS в PVS-Studio

Post by Control Frik » Thu Sep 01, 2011 11:30 am

Jedi, ты ссылку сразу на IRC закинь и Брагину. А ещё лучше сделать хороший превод, потому что большая часть разрабтчиков по русски не понимэ.

EvgeniyRyzhkov
Posts: 3
Joined: Thu Sep 01, 2011 1:28 pm

Re: Анализ кода ReactOS в PVS-Studio

Post by EvgeniyRyzhkov » Thu Sep 01, 2011 1:35 pm

Терпение, мы скоро переведем и выложим статью на английском. Кстати, если есть вопросы - готов пообщаться.

Евгений Рыжков
ООО "СиПроВер"

hto
Developer
Posts: 2193
Joined: Sun Oct 01, 2006 3:43 pm

Post by hto » Thu Sep 01, 2011 4:19 pm

Но, тем не менее, там, где благодаря Coverity «было найдено несколько новых ошибок», PVS-Studio находит их целый вагон и маленькую тележку.
Я не видел результатов анализа ReactOS с помощью Coverity, но подозреваю, что ошибок там обнаружено вовсе не несколько, а тысячи…

Даже тот статический анализатор, что встроен в GCC, выдаёт множество предупреждений. Только вот мало желающих заниматься таким "негламурным" занятием, как исправление ошибок в существующем коде.
Считаю, что статический анализ кода должен являться обязательным элементом цикла разработки ReactOS и других крупных проектов.
Полностью согласен; как уже писал в ветке про "бесплатный анал", я думаю, что разработчики должны быть more anal насчёт качества кода. :)

EvgeniyRyzhkov
Posts: 3
Joined: Thu Sep 01, 2011 1:28 pm

Re: Анализ кода ReactOS в PVS-Studio

Post by EvgeniyRyzhkov » Fri Sep 02, 2011 12:30 pm

Перевод статьи ещё не готов. Однако, если кому-то из иностранных товарищей хочется поработать со списком ошибок, то я подготовил для них вот этот http://www.viva64.com/external-pictures ... nicode.txt . В нём переведены разъяснения к ошибкам.

Евгений Рыжков
ООО "СиПроВер"

evilslon
Posts: 261
Joined: Sat Apr 11, 2009 7:39 pm
Location: Russia, Ivanovo
Contact:

Re: Анализ кода ReactOS в PVS-Studio

Post by evilslon » Sat Sep 03, 2011 2:50 pm

EvgeniyRyzhkov wrote:Перевод статьи ещё не готов. Однако, если кому-то из иностранных товарищей хочется поработать со списком ошибок, то я подготовил для них вот этот http://www.viva64.com/external-pictures ... nicode.txt . В нём переведены разъяснения к ошибкам.
Как оказалось, не все программы статического анализа одинаково полезны, большинство из коммитов, сделанных Lentin'ом по приведённому на Хабре списку (созданному в PVS) были ревертануты. Это видно здесь, здесь, здесь, и здесь. Может люди, сведущие в программировании прояснят ситуацию? С Coverity по крайней мере таких проблем видно не было...

hto
Developer
Posts: 2193
Joined: Sun Oct 01, 2006 3:43 pm

Post by hto » Mon Sep 05, 2011 6:38 pm

Заметил ли PVS-Studio, что в Device_SaveCurrentSettings (в drivers/bus/pcix/device.c) есть выход за границы массива?

EvgeniyRyzhkov
Posts: 3
Joined: Thu Sep 01, 2011 1:28 pm

Re: Анализ кода ReactOS в PVS-Studio

Post by EvgeniyRyzhkov » Wed Sep 14, 2011 9:13 am

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

В первом случае, это мы ввели в заблуждение. Невнимательно посмотрели текст сообщения:
V575 The 'memset' function processes value '4294967295'. Inspect the second argument. mkhive bitmap.c 342
Код:

Code: Select all

#define RtlFillMemoryUlong(dst, len, val) memset(dst, val, len)

RtlFillMemoryUlong(Buffer, NumberToSet >> 3, MAXULONG);
Здесь просто странно, передавать 0xFFFFFFFF в memset, так как используется только младшие 8 бит. В общем то, это ложное сообщение. Мы были невнимательны и отчего-то показалось, что спутаны аргументы. Бывает.

Почему не понравилась вторая правка - не понятно. Дело в том, что sizeof(dest) возвращает размер указателя, а не массива.
Вот этот код:

Code: Select all

Mapdesc::identify( REAL dest[MAXCOORDS][MAXCOORDS] )
{
memset( dest, 0, sizeof(dest));
Эквивалентен:

Code: Select all

memset( dest, 0, sizeof(void *));
И вряд ли хотелось очистить только часть матрицы.

Третий случай. Опять не понятно, что не понравилось.

Думаю, дело было так. Увидели, что пользователем внесен ряд правок. И самая первая – неверная. И от греха подальше отклонили его правки. И правильно сделали. Надо подходить к правкам с головой и не так. Мы изучали проект поверхностно. И к каждой правке надо подходить с вниманием. Множество диагностик мы вообще не привели в списке, чтобы снизить возможность вот таких вот ложных советов.

Andrey_Karpov
Posts: 11
Joined: Fri Sep 16, 2011 6:28 pm

PVS-Studio: анализируем код операционной системы ReactOS

Post by Andrey_Karpov » Fri Sep 16, 2011 6:40 pm

Image

Аннотация

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

PVS-Studio становится все лучше и лучше

Начну с последнего момента, по поводу пользы инструмента PVS-Studio. ReactOS косвенно подтверждает, что PVS-Studio развивается в правильном направлении. Вот новость о проверке ReactOS с помощью такого тяжеловеса, как Coverity - "Статический анализ в Coverity" [1]. Я, конечно, знаю и понимаю, что до возможностей Coverity нам далеко. Но, тем не менее, там, где благодаря Coverity "было найдено несколько новых ошибок", PVS-Studio находит их целый вагон и маленькую тележку. При этом никакой код никуда отправлять не надо. Можно просто взять и проверить проект. Значит мы на верном пути.

Что такое ReactOS?

ReactOS - это современная, свободная и открытая операционная система, основанная на архитектуре Windows XP/2003. Система была написана с нуля и имеет своей целью повторение архитектуры Windows-NT, созданной Microsoft, от аппаратного до прикладного уровня. Объем исходного кода на языке Си, Си++ и ассемблере составляет порядка 220 мегабайт.

Различные ссылки:
Ошибки в ReactOS

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

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

Ошибки, которые можно встретить в ReactOS разнообразнейшие. Просто зоопарк. Есть опечатки из одного символа.

Code: Select all

BOOL WINAPI GetMenuItemInfoA(...)
{
  ...
  mii->cch = mii->cch;
  ...
}
На самом деле должно быть написано вот так: "mii->cch = miiW->cch;". Потеряли букву 'W'. Как результат, программы уже не могут доверять функции GetMenuItemInfoA.

А вот другая опечатка в один символ. В этот раз некорректно работает сравнение двух имён.

Code: Select all

static void _Stl_loc_combine_names(_Locale_impl* L,
  const char* name1, const char* name2,
  locale::category c)
{
  if ((c & locale::all) == 0 || strcmp(name1, name1) == 0)
  ...
}
Есть путаница между оператором && и &. Очень распространенная ошибка. Встречаю практически в каждом проекте, где работают с битами или атрибутами файлов.

Code: Select all

static LRESULT APIENTRY ACEditSubclassProc()
{
  ...
  if ((This->options && ACO_AUTOSUGGEST) &&
      ((HWND)wParam != This->hwndListBox))
  ...
}
Корректный код должен выглядеть так "(This->options & ACO_AUTOSUGGEST)". Пример ниже содержит родственную ей ошибку, из-за которой всё условие всегда ложно.

Code: Select all

void adns__querysend_tcp(adns_query qu, struct timeval now) {
  ...
    if (!(errno == EAGAIN || EWOULDBLOCK || errno == EINTR ||
        errno == ENOSPC || errno == ENOBUFS || errno == ENOMEM)) {
  ...
}
Если присмотреться, то можно заметить коварный фрагмент: "|| EWOULDBLOCK ||".

Кстати, в ReactOS нашлось достаточно много условий, которые всегда истинны или ложны. Некоторые нестрашные, так как, например, располагаются в макросе assert(). Но есть, на мой взгляд, и критичные.

Code: Select all

INT WSAAPI
connect(IN SOCKET s,
        IN CONST struct sockaddr *name,
        IN INT namelen)
{
  ...
  /* Check if error code was due to the host not being found */
  if ((Status == SOCKET_ERROR) &&
      (ErrorCode == WSAEHOSTUNREACH) &&
      (ErrorCode == WSAENETUNREACH))
  {
  ...
}
Согласитесь, что реализация таких функций как "connect" должна быть протестирована максимально полно. А здесь мы имеем условие, которое всегда ложно. Быстро заметить дефект не так просто, поэтому выделю суть ошибки:

Code: Select all

(ErrorCode == 10065) && (ErrorCode == 10051)
Кстати, часть, связанная с сокетами, вообще выглядит сырой. Возможно, это связано с тем, что в Linux мире SOCKET принято объявлять как знаковый тип. А в Windows он беззнаковый:

Code: Select all

typedef UINT_PTR SOCKET;
Как результат имеем разнообразные ошибки в операциях сравнения:

Code: Select all

void adns_finish(adns_state ads) {
  ...
  if (ads->tcpsocket >= 0) adns_socket_close(ads->tcpsocket);
  ...
}
Выражение "ads->tcpsocket >= 0" не имеет смысла, так как всегда истинно.

Встречаются просто странные фрагменты. Скорее всего, это недописанные и забытые участки кода.

Code: Select all

if (ERROR_SUCCESS == hres)
{
  Names[count] = HeapAlloc(GetProcessHeap(), 0, strlenW(szValue) + 1);
  if (Names[count])
     strcmpW(Names[count], szValue);
}
Зачем вызывать "strcmpW", если результат никак не используется?

Имеются ошибки, связанные с приоритетом операций.

Code: Select all

VOID NTAPI
AtapiDmaInit(...)
{
  ...
  ULONG treg = 0x54 + (dev < 3) ? (dev << 1) : 7;
  ...
}
Я расставлю скобки, чтобы стало ясно, как работает это выражение на самом деле:

Code: Select all

ULONG treg = (0x54 + (dev < 3)) ? (dev << 1) : 7;
Следующая ошибка обязательно встречается в любом большом проекте. Есть парочка и в ReactOS. Речь идет о лишней точке с запятой - ';'.

Code: Select all

BOOLEAN
CTEScheduleEvent(PCTE_DELAYED_EVENT Event,
                 PVOID Context)
{
  ...
  if (!Event->Queued);
  {
    Event->Queued = TRUE;
    Event->Context = Context;
    ExQueueWorkItem(&Event->WorkItem, CriticalWorkQueue);
  }
  ...
}
Ещё мне нравятся ошибки с инициализацией элементов массива. Не знаю почему. Они трогательны. Возможно, я вспоминаю свои первые эксперименты с массивами на языке Basic.

Code: Select all

HPALETTE CardWindow::CreateCardPalette()
{
  ...
  //include button text colours
  cols[0] = RGB(0, 0, 0);
  cols[1] = RGB(255, 255, 255);

  //include the base background colour
  cols[1] = crBackgnd;

  //include the standard button colours...
  cols[3] = CardButton::GetHighlight(crBackgnd);
  cols[4] = CardButton::GetShadow(crBackgnd);
  ...
}
Можно продолжать приводить разные интересные участки кода. К сожалению, тогда статья станет слишком длинной и нужно останавливаться. Напомню, что посмотреть на другие ошибки, найденные мной в ReactOS, можно вот в этом файле. На сладкое только приведу вот этот кусочек кода:

Code: Select all

#define SWAP(a,b,c)  c = a;\
                     a = b;\
                     a = c
Пример использования:

Code: Select all

BOOL FASTCALL
IntEngGradientFillTriangle(...)
{
  ...
  SWAP(v2,v3,t);
  ...
}
Это – шедевр.

Статический анализ кода

Я считаю ReactOS очень хорошим примером проекта, где регулярный статический анализ кода просто необходим. И дело здесь не в квалификации разработчиков. Проект большой и содержит разнообразные подсистемы. Это значит, что над таким проектом всегда трудится большое количество людей. А в большом коллективе всегда кто-то программирует хуже, кто-то лучше. Кто-то использует один стиль, кто-то другой. Но никто не застрахован от ошибок. Вот смотрите.

Один человек взял и написал в ReactOS вот так:

Code: Select all

if ((res = setsockopt(....) == -1))
Код делает не то, что хочется. Корректный вариант: if ((res = setsockopt(....)) == -1). Если придерживаться практики писать константу в начале, то случайно никогда не сделаешь ошибочное присваивание внутри оператора "if". У нас здесь другой тип ошибки. Но если писать код по приведенному правилу, то не получится сделать ошибку и в рассматриваемом выражении: "if (-1 == res = setsockopt(....))".

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

Code: Select all

static DWORD CALLBACK
RegistrationProc(LPVOID Parameter)
{
  ...
  if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR,
                        UnknownError,
                        sizeof(UnknownError) /
                        sizeof(UnknownError[0] - 20)))
  ...
}
Здесь вначале красиво написана константа 0. Вот только круглая скобка закрывается не там, где надо. Обыкновенная опечатка.

К чему я все эти примеры? А к тому, что никто из нас программистов не идеален. И ни стандарт кодирования, ни технологии программирования, ни самоконтроль не гарантируют отсутствие ошибок в коде.

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

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

Поясню своё утверждение. В подобных системах невозможно приблизиться к 100% покрытию кода при тестировании с помощью юнит-тестов или регрессионных тестов. Немного уточню. Можно конечно, но затраты на создание и поддержание таких тестов становятся недопустимо высоки.

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

Пример проверки кода, редко получающего управление:

Code: Select all

static HRESULT STDMETHODCALLTYPE
CBindStatusCallback_OnProgress(...)
{
  ...
  if (This->szMimeType[0] != _T('\0'))
    _tprintf(_T("Length: %I64u [%s]\n"), This->Size, 
             This->szMimeType);
  else
    _tprintf(_T("Length: %ull\n"), This->Size);
  ...
}
Скорее всего, изначально код был написан неправильно. Потом кто-то заметил, что сообщение формируется неправильно и внес исправление, написав "%I64u". А вот на код по соседству он не обратил внимание. И там по-прежнему имеется некорректный формат "%ull". Видимо ветка вызывается крайне редко. Статический анализ такое не пропустит. Собственно он и не пропустил, раз я могу продемонстрировать этот пример.

Другим хорошим примером может служить большое количество ошибок очистки памяти, которые я увидел ReactOS. Почему их так много, мне понятно. Никто не тестирует, заполнилась память или нет. Во-первых, сложно осознать, что в этих простых местах можно ошибиться. А во-вторых, не так просто проверить очистился внутри функции какой-то временный буфер или нет. Здесь статический анализ вновь на высоте. Приведу только пару примеров. На самом деле я насчитал как минимум 13 ошибок заполнения массивов константным значением.

Code: Select all

#define MEMSET_BZERO(p,l) memset((p), 0, (l))

char *SHA384_End(SHA384_CTX* context, char buffer[]) {
  ...
  MEMSET_BZERO(context, sizeof(context));
  ...
}
Очищаем только первые байты массива, так как sizeof(context) возвращает размер указателя, а не структуры.

Code: Select all

#define RtlFillMemory(Destination, Length, Fill) \
  memset(Destination, Fill, Length)

#define IOPM_FULL_SIZE          8196

HalpRestoreIopm(VOID)
{
  ...
  RtlFillMemory(HalpSavedIoMap, 0xFF, IOPM_FULL_SIZE);
  ...
}
Перепутаны аргументы при использовании макроса RtlFillMemory. Вызов должен быть таким:

Code: Select all

RtlFillMemory(HalpSavedIoMap, IOPM_FULL_SIZE, 0xFF);
Снова о табуляциях и пробелах

Заранее хочу попросить не начинать в комментариях новую бурную дискуссию на эту тему. Я просто выскажу своё мнение. С ним можно быть согласным или нет. Но обсуждать не стоит.

Есть два непримиримых лагеря. Одни за использование табуляции в коде, так как это позволяет подстраивать отображение кода под себя [2]. Другие говорят, что это всё равно не работает и, что нет разумных причин использовать символы табуляции. От табуляции только вред и разъезжающееся форматирование. К их числу отношусь и я [3].

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

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

В стандарте кодирования ReactOS [4] написано хорошее правило с теоретической точки зрения:

Generic note about TABs usage: Don't use TABs for formatting; use TABs for indenting only and use only spaces for formatting.

Code: Select all

Example: 
NTSTATUS
SomeApi(IN Type Param1,
[spaces]IN Type Param2)
{
[TAB]ULONG MyVar;
[TAB]MyVar = 0;
[TAB]if ((MyVar == 3) &&
[TAB][sp](Param1 == TRUE))
[TAB]{
[TAB][TAB]CallSomeFunc();
...
Поклонники табуляции довольны. Однако я открываю исходные коды ReactOS и наблюдаю во многих местах испорченное форматирование. Почему?

Image

Image

Image

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

Это не шаг назад в оформлении кода! Это шаг вперёд! Это следующий уровень осознания. Теоретическая красота настраиваемых отступов не сочетается с практикой. В первую очередь важно обеспечить однозначное представление кода и легкость разработки в большой команде. В компании Google это понимают. И в их стандарте для форматирования используются только пробелы [5]. Тем, кто ратует за использование табуляции, еще раз рекомендую задумываться, почему распределенная команда высококлассных профессионалов, которые разрабатывают Chromium, выбрала именно пробелы.

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

Рекомендую команде разрабатывающей ReactOS модифицировать их стандарт и отказаться от использования табуляции. Любая табуляция должна расцениваться, как ошибка и быть устранена из кода.

Кстати, подобная практика позволит находить вот такие вот безобразия в коде ReactOS:

Code: Select all

BOOLEAN
KdInitSystem(IN ULONG BootPhase,
             IN PLOADER_PARAMETER_BLOCK LoaderBlock)
{
  ...
  /* Check if this is a comma, a space or a tab */
  if ((*DebugOptionEnd == ',') ||
      (*DebugOptionEnd == ' ') ||
      (*DebugOptionEnd == ' '))
  ...
}
Последнее сравнение, это сравнение с символом табуляции, а не с пробелом, как может показаться. Нормальный код должен выглядеть так: "(*DebugOptionEnd == '\t')".

Примечание для сторонников табуляции. Не надо мне вновь рассказывать, как правильно использовать табуляции. И это не мой код. Смотрите, вот есть вполне конкретный проект ReactOS. В нем есть плохо отформатированный код. Подумайте, какие действия позволят сделать так, чтобы новый программист мог открыть код проекта и не гадать, какой размер символа табуляции ему необходимо установить в настройках редактора. Мысли в духе "нужно сразу было писать правильно" практической ценности не имеют.

Библиографический список
  1. Выпуск новостей ReactOS N79. Статический анализ в Coverity. http://www.viva64.com/go.php?url=722
  2. Пора завязывать использовать пробелы вместо табуляции в коде. http://www.viva64.com/go.php?url=725
  3. Пора завязывать использовать символы табуляции в коде. http://www.viva64.com/go.php?url=726
  4. ReactOS. Coding Style. http://www.viva64.com/go.php?url=724
  5. Google C++ Style Guide. http://www.viva64.com/go.php?url=679

fireball
Developer
Posts: 358
Joined: Tue Nov 30, 2004 10:40 pm
Location: Moscow, Russia
Contact:

Re: PVS-Studio: анализируем код операционной системы ReactOS

Post by fireball » Tue Sep 27, 2011 6:43 pm

Я приведу здесь письмо, которое служит ответом на поступившее мне предложение приобрести лицензию PVS-STUDIO со скидкой:

Здравствуйте Андрей,

Ваша компания готова бесплатно предоставить PVS Studio для анализа исходного когда открытого некоммерческого проекта ReactOS?

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


С уважением,
Президент Фонда Реактос
Брагин А.В.
Aleksey Bragin,
ReactOS Project Lead

Andrey_Karpov
Posts: 11
Joined: Fri Sep 16, 2011 6:28 pm

Re: PVS-Studio: анализируем код операционной системы ReactOS

Post by Andrey_Karpov » Wed Sep 28, 2011 7:29 am

Здравствуйте, Алексей.

Мы давно предоставляем и продолжаем предоставлять возможность проверить бесплатные open-source проекты. Да, эта возможность ограничена по времени. Мы не видим смысла просто так раздавать ключи на долгий период. Однако, если такие пользователи сообщают об ошибках или предлагают новые диагностические правила, то мы с радостью продлеваем им ключи и общаемся с ними. Приятным примером такого взаимодействия является проект FlylinkDC++.

P.S. Только прошу не надо постоянно писать про чистый альтруизм и желание сделать мир лучше. Проекты подобного размера могут существовать только тогда, когда это кому то надо. Это действует на студентов, но никак не на профессиональных разработчиков. WooS тоже будет всегда бесплатен? :)
----

С уважением, Андрей Карпов
к.ф.-м.н., Технический директор
ООО "Системы программной верификации"
Сайт: http://www.viva64.com/ru/
E-Mail: karpov[[@]]viva64.com
Тел.: +7 (4872) 38-59-95 (GMT + 03:00)

Andrey_Karpov
Posts: 11
Joined: Fri Sep 16, 2011 6:28 pm

Большой отчет о повторной проверке проекта ReactOS

Post by Andrey_Karpov » Tue Apr 02, 2013 9:04 am

Проект ReactOS активно продолжает развиваться. Один из разработчиков, участвующий в этом проекте, предложил вновь проверить исходный код, так как кодовая база быстро увеличивается. С удовольствием сделаем это. Нам симпатичен этот проект. Мы будем рады, если статья поможет исправить ряд недочетов. Для проверки используется анализатор кода PVS-Studio версии 5.02.

Image

Напомним, что такое ReactOS. Это свободная и открытая операционная система, основанная на принципах архитектуры Windows NT. Система была разработана с нуля и таким образом не основана на Linux и не имеет ничего общего с архитектурой UNIX. Основной целью проекта ReactOS является создание бинарно-совместимой с Windows операционной системы. Такая система позволяет выполнять Windows-совместимые приложения и драйвера так, как если бы они выполнялись в самой Windows.

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

Хочу предупредить, я опишу далеко не все места в проекте, на которые стоит обратить внимание. ReactOS стал большим мальчиком. В состав решения (solution) входит 803 проекта. Для них анализатор PVS-Studio выдал множество предупреждений общего назначения:
  1. 1320 предупреждений первого уровня;
  2. 814 предупреждений второго уровня;
  3. 2753 предупреждений третьего уровня.
Естественно, у меня нет сил взять и внимательно изучить все эти предупреждения. Я укажу только на наиболее подозрительные места, на которых остановился мой взгляд. Наверняка, есть другие предупреждения, которые заслуживают не меньшего внимания. А ещё есть диагностики, связанные с 64-битными ошибками и микро-оптимизациями. Их я вообще не изучал.

Демонстрационной версии PVS-Studio будет недостаточно, чтобы изучить 4887 предупреждений. Однако, мы дружелюбно относимся к открытым проектам. Если разработчики ReactOS обратятся к нам, мы бесплатно предоставим на время им наш инструмент.

Опечатки

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

Затирание значения переменной

Code: Select all

NTSTATUS NTAPI CreateCdRomDeviceObject(....)
{
  ....
  cddata->XAFlags &= ~XA_USE_6_BYTE;
  cddata->XAFlags = XA_USE_READ_CD | XA_USE_10_BYTE;
  ....
}
V519 The 'cddata->XAFlags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1290, 1291. cdrom.c 1291

Присваивание затирает предыдущее значение члена XAFlags. Скорее всего, здесь должно быть написано так: cddata->XAFlags |= XA_USE_READ_CD | XA_USE_10_BYTE;. Но я, конечно, не ручаюсь, так как не знаю, как работает этот код.

Повтор в условии

Code: Select all

void util_blit_pixels_writemask(....)
{
  ....
  if ((src_tex == dst_surface->texture &&
      dst_surface->u.tex.level == src_level &&
      dst_surface->u.tex.first_layer == srcZ0) ||
      (src_tex->target != PIPE_TEXTURE_2D &&
      src_tex->target != PIPE_TEXTURE_2D &&
      src_tex->target != PIPE_TEXTURE_RECT))
  ....
}
V501 There are identical sub-expressions 'src_tex->target != PIPE_TEXTURE_2D' to the left and to the right of the '&&' operator. u_blit.c 421

Два раза выполняется проверка "src_tex->target != PIPE_TEXTURE_2D". Второй раз член 'target' должен сравниваться с другой константой. Или это сравнение лишнее.

Рассмотрим аналогичную ошибку:

Code: Select all

static boolean is_legal_int_format_combo(
  const struct util_format_description *src,
  const struct util_format_description *dst )
{
  ....
  for (i = 0; i < nr; i++) {
    /* The signs must match. */
    if (src->channel[i].type != src->channel[i].type) {
      return FALSE;
    }
  ....
}
V501 There are identical sub-expressions 'src->channel.type' to the left and to the right of the '!=' operator. translate_generic.c 776

Правильная проверка по всей видимости должна выглядеть так: src->channel.type != dst->channel.type.



И ещё одна аналогичная ошибка:

Code: Select all

static GpStatus draw_poly(....)
{
  ....
  if((i + 2 >= count) ||
     !(types[i + 1] & PathPointTypeBezier) ||
     !(types[i + 1] & PathPointTypeBezier))
  {
    ERR("Bad bezier points\n");
    goto end;
  }
  ....
}
V501 There are identical sub-expressions '!(types[i + 1] & PathPointTypeBezier)' to the left and to the right of the '||' operator. graphics.c 1912



И ещё:

Code: Select all

static inline BOOL is_unc_path(const WCHAR *str) {
  return (str[0] == '\\' && str[0] == '\\');
}
V501 There are identical sub-expressions to the left and to the right of the '&&' operator: str[0] == '\\' && str[0] == '\\' uri.c 273

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



И ещё:

Code: Select all

VOID NTAPI UniAtaReadLunConfig(....)
{
  if(!LunExt->IdentifyData.SectorsPerTrack ||
     !LunExt->IdentifyData.NumberOfCylinders ||
     !LunExt->IdentifyData.SectorsPerTrack)
    ....
}
V501 There are identical sub-expressions '!LunExt->IdentifyData.SectorsPerTrack' to the left and to the right of the '||' operator. id_init.cpp 1528

Думаю, ошибка хорошо заметна. Как надо исправить код, я не знаю.



Потерпите. Но у меня есть и другие ошибки близнецы. Что делать... Это очень типичные ошибки в программах.

Code: Select all

ir_visitor_status
ir_validate::visit_leave(ir_loop *ir)
{
  if (ir->counter != NULL) {
    if ((ir->from == NULL) || (ir->from == NULL) ||
        (ir->increment == NULL)) {
  ....
}
V501 There are identical sub-expressions to the left and to the right of the '||' operator: (ir->from == 0) || (ir->from == 0) ir_validate.cpp 123

Здесь одно из сравнений "ir->from == 0" должно быть заменено на "ir->to == NULL".

Эту же ошибку, благодаря технологии copy-paste, можно наблюдать здесь: V501 There are identical sub-expressions to the left and to the right of the '||' operator: (ir->from != 0) || (ir->from != 0) ir_validate.cpp 139

Лишняя точка с запятой

Наконец, мы добрались до другой разновидности опечаток. Это лишняя точка с запятой ';', которая всё портит.

Code: Select all

int BlockEnvToEnvironA(void)
{
  ....
  for (envptr--; envptr >= _environ; envptr--);
    free(*envptr);
  ....
}
V529 Odd semicolon ';' after 'for' operator. environ.c 67

Обратите внимание на ';' после оператора 'for'. Как результат функция free() будет вызвана только один раз. Это приведет к утечкам памяти. Также произойдет освобождение участка памяти, который не планировалось освобождать. Ошибочный код сейчас работает так:

Code: Select all

free(envptr >= _environ ? _environ[-1] : envptr);
Аналогичные точки с запятой можно увидеть здесь:
  • V529 Odd semicolon ';' after 'for' operator. environ.c 119
  • V529 Odd semicolon ';' after 'for' operator. environ.c 171


Неправильное выражение

Code: Select all

static HRESULT WINAPI JScriptSafety_SetInterfaceSafetyOptions(
  ...., DWORD dwEnabledOptions)
{
  ....
  This->safeopt = dwEnabledOptions & dwEnabledOptions;
  return S_OK;
}
V501 There are identical sub-expressions to the left and to the right of the '&' operator: dwEnabledOptions & dwEnabledOptions jscript.c 905

Видимо, имя одного из операндов в выражении задано неправильно.



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

Code: Select all

GpStatus WINGDIPAPI GdipGetRegionBoundsI(....)
{
  ....
  status = GdipGetRegionBounds(region, graphics, &rectf);
  if (status == Ok){
    rect->X = gdip_round(rectf.X);
    rect->Y = gdip_round(rectf.X);
    rect->Width  = gdip_round(rectf.Width);
    rect->Height = gdip_round(rectf.Height);
  }
  return status;
}
V656 Variables 'rect->X', 'rect->Y' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'gdip_round(rectf.X)' expression. Check lines: 718, 719. region.c 719

Я почти уверен, что здесь должно быть написано: "rect->Y = gdip_round(rectf.Y);". Если это вдруг не так, то здесь бы не помешал бы комментарий.



Фрагмент кода, где переменная присваивается сама себе:

Code: Select all

DWORD WINAPI
DdGetDriverInfo(LPDDHAL_GETDRIVERINFODATA pData)
{
  ....
  pUserColorControl->dwFlags = pUserColorControl->dwFlags;
  ....
}
V570 The 'pUserColorControl->dwFlags' variable is assigned to itself. gdientry.c 1029

Присваивание не имеет смысла. Скорее всего, выражение не дописано или что-то перепутано. Аналогичная ошибка здесь:

V570 The 'Irp->IoStatus.Information' variable is assigned to itself. hidclass.c 461

Поговорим о нулевых указателях

Где программа на Си/Си++, там и проблемы с указателями. Это плата за эффективность языка. Впрочем, Си++ и особенно Си++11 предлагает массу способов избегать работы с дикими указателями. Но это тема отдельной статьи.

Посмотрим, где могут быть проблемы в ReactOS.

Разыменовывание нулевого указателя

Code: Select all

static void acpi_bus_notify (....)
{
  struct acpi_device *device = NULL;
  ....
  switch (type) {
    ....
    case ACPI_NOTIFY_EJECT_REQUEST:
      DPRINT1("Received EJECT REQUEST "
              "notification for device [%s]\n", 
              device->pnp.bus_id);
      break;
    ....
  }
}
V522 Dereferencing of the null pointer 'device' might take place. bus.c 762

Если в операторе 'switch' будет выбрана ветка "case ACPI_NOTIFY_EJECT_REQUEST:", то указатель 'device' в этот момент будет по прежнему равен нулю. Его разыменование в выражении "device->pnp.bus_id" приведёт к неприятным последствиям.

Таким нехорошим способом переменная 'device' используется и в других местах:
  • V522 Dereferencing of the null pointer 'device' might take place. bus.c 768
  • V522 Dereferencing of the null pointer 'device' might take place. bus.c 774
  • V522 Dereferencing of the null pointer 'device' might take place. bus.c 780
  • V522 Dereferencing of the null pointer 'device' might take place. bus.c 786


Другой фрагмент кода, где переменная к моменту её использования остаётся равной нулю:

Code: Select all

ir_texture *ir_reader::read_texture(s_expression *expr)
{
  s_symbol *tag = NULL;
  ....
  } else if (MATCH(expr, other_pattern)) {
    op = ir_texture::get_opcode(tag->value());
    if (op == -1)
      return NULL;
  }
  ....
}
V522 Dereferencing of the null pointer 'tag' might take place. ir_reader.cpp 904

В момент вызова функции value() переменная 'tag' будет всё ещё равна нулю. Это нехорошо. Можно назвать и другие аналогичные ошибки разыменования нулевого указателя в ReactOS:
  • V522 Dereferencing of the null pointer 's_shadow' might take place. ir_reader.cpp 964
  • V522 Dereferencing of the null pointer 'BootSectorInfo' might take place. disksup.c 1750
  • V522 Dereferencing of the null pointer 'BootSectorInfo' might take place. disksup.c 1751
  • V522 Dereferencing of the null pointer 'BootSectorInfo' might take place. disksup.c 1754
Передача нулевого указателя в функцию

Code: Select all

BOOL GetEventCategory(....)
{
  ....
  if (lpMsgBuf)
  {
    ....
  }
  else
  {
    wcscpy(CategoryName, (LPCWSTR)lpMsgBuf);
  }
  ....
}
V575 The null pointer is passed into 'wcscpy' function. Inspect the second argument. eventvwr.c 270

Функция wcscpy() вызывается только в том случае, если переменная 'lpMsgBuf' равна нулю. Эта переменная передаётся как аргумент в функцию 'wcscpy'. Передавать ноль в функцию 'wcscpy' это хулиганство.

А вот другой хулиган издевается над кошкой функцией strstr():

Code: Select all

VOID WinLdrSetupEms(IN PCHAR BootOptions)
{
  PCHAR RedirectPort;
  ....
  if (RedirectPort)
  {
    ....
  }
  else
  {
    RedirectPort = strstr(RedirectPort, "usebiossettings");
  ....
}
V575 The null pointer is passed into 'strstr' function. Inspect the first argument. headless.c 263



За компанию, функция _wcsicmp() тоже пострадала:

Code: Select all

DWORD ParseReasonCode(LPCWSTR code)
{
  LPWSTR tmpPrefix = NULL;
  ....
  for (reasonptr = shutdownReason ; reasonptr->prefix ; reasonptr++)
  {
    if ((majorCode == reasonptr->major) &&
        (minorCode == reasonptr->minor) &&
        (_wcsicmp(tmpPrefix, reasonptr->prefix) != 0))
    {
      return reasonptr->flag;
    }
  }
  ....
}
V575 The null pointer is passed into '_wcsicmp' function. Inspect the first argument. misc.c 150

К моменту вызова функции _wcsicmp() указатель tmpPrefix будет равен нулю.



Разыменовывание потенциально нулевого указателя

Очень много фрагментов кода, где вначале указатель разыменовывается, а только потом проверяется на равенство нулю. Это не всегда ошибка. Возможно, указатель не может быть равен нулю и проверка просто лишняя. Но, как правило, такой код появляется из-за невнимательности и он не верен. И работает он только до того момента, пока злосчастный указатель, из-за стечения обстоятельств, вдруг не станет равен нулю.

Я рассмотрю здесь только один простой пример:

Code: Select all

static BOOL LookupSidInformation(....)
{
  ....
  DomainName = &PolicyAccountDomainInfo->DomainName;
  SidNameUse = (PolicyAccountDomainInfo != NULL ?
                SidTypeGroup : SidTypeUser);
  ....
}
V595 The 'PolicyAccountDomainInfo' pointer was utilized before it was verified against nullptr. Check lines: 254, 257. sidcache.c 254

Смотрите, в начале указатель 'PolicyAccountDomainInfo' разыменовывается. А потом вдруг проверяется на равенство нулю. Часто такой код появляется в ходе стремительного рефакторинга. Использование переменных перемещается в коде в то место, где эта переменная ещё не проверена.

Я рассматриваю только один такой фрагмент кода по той причине, что все они очень похожи. И потому, что ИХ ОЧЕНЬ МНОГО. Мне не интересно разбираться и описывать каждый случай. Да и в статью все равно их поместить нет никакой возможности. Это уже будет не статья, а справочник. Ограничусь только диагностическими сообщениями:

СПИСОК

Макросы

Макросы это плохо. Это остается моим твердым мнением. Делайте нормальные функции везде, где это возможно.

В ReactOS кто-то поленился сделать полноценную функцию stat64_to_stat() и ограничился изготовлением говномакроса. Этот макрос выглядит так:

Code: Select all

#define stat64_to_stat(buf64, buf)   \
    buf->st_dev   = (buf64)->st_dev;   \
    buf->st_ino   = (buf64)->st_ino;   \
    buf->st_mode  = (buf64)->st_mode;  \
    buf->st_nlink = (buf64)->st_nlink; \
    buf->st_uid   = (buf64)->st_uid;   \
    buf->st_gid   = (buf64)->st_gid;   \
    buf->st_rdev  = (buf64)->st_rdev;  \
    buf->st_size  = (_off_t)(buf64)->st_size;  \
    buf->st_atime = (time_t)(buf64)->st_atime; \
    buf->st_mtime = (time_t)(buf64)->st_mtime; \
    buf->st_ctime = (time_t)(buf64)->st_ctime; \


Посмотрим теперь, как этот макрос используется в функции _tstat:

Code: Select all

int CDECL _tstat(const _TCHAR* path, struct _stat * buf)
{
  int ret;
  struct __stat64 buf64;

  ret = _tstat64(path, &buf64);
  if (!ret)
    stat64_to_stat(&buf64, buf);
  return ret;
}
Вы думаете, что макрос 'stat64_to_stat' выполнится в том случае, если переменная 'ret' равна нулю? Ничего подобного. Макрос раскрывается в набор отдельных строк. Поэтому к оператору 'if' относится только строчка "buf->st_dev = (buf64)->st_dev;". Остальные строки кода будут выполнены всегда!

Другие места, где используется этот неправильный макрос:
  • V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. stat.c 35
  • V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. stat.c 47
  • V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. stat.c 58
Условия, которые всегда истинны или ложны

Рассмотрим случай, когда всегда истинное условие может привести к бесконечному циклу.

Code: Select all

#define DISKREADBUFFER_SIZE HEX(10000)
typedef unsigned short USHORT, *PUSHORT;
static VOID DetectBiosDisks(....)
{
  USHORT i;
  ....
  Changed = FALSE;
  for (i = 0; ! Changed && i < DISKREADBUFFER_SIZE; i++)
  {
    Changed = ((PUCHAR)DISKREADBUFFER)[i] != 0xcd;
  }
  ....
}
V547 Expression 'i < 0x10000' is always true. The value range of unsigned short type: [0, 65535]. xboxhw.c 358

Цикл предназначен для поиска в массиве DISKREADBUFFER байта, значение которого не равно'0xCD'. Если такого байта нет, то переменная 'Changed' всегда имеет значение FALSE. В этом случае условием остановки цикла является выражение "i < DISKREADBUFFER_SIZE". Это выражение всегда истинно и программа зациклится.

Ошибка кроется в том, что переменная 'i' имеет тип 'unsigned short'. Она может принимать значения в диапазоне от 0 до 65535. Эти значения всегда меньше, чем '0x10000'.



Типовой ошибкой, которую я вижу во многих проектах, является предположение, что SOCKET является знаковой переменной. Это не так. Вернее это зависит от реализации библиотеки.

Code: Select all

typedef UINT_PTR SOCKET;
#define ADNS_SOCKET SOCKET
struct adns__state {
  ....
  ADNS_SOCKET udpsocket, tcpsocket;
  ....
};

static int init_finish(adns_state ads) {
  ....
  if (ads->udpsocket<0) { r= errno; goto x_free; }
  ....
}
V547 Expression 'ads->udpsocket < 0' is always false. Unsigned type value is never < 0. setup.c 539

Переменная 'udpsocket' является беззнаковой, а значит условие 'ads->udpsocket < 0' всегда ложное. Чтобы понять, что произошла ошибка, нужно использовать константу SOCKET_ERROR.

Аналогичные ошибки при работе с сокетами находятся здесь:
  • V547 Expression 'fd < 0' is always false. Unsigned type value is never < 0. event.c 117
  • V547 Expression 'ads->udpsocket >= 0' is always true. Unsigned type value is always >= 0. check.c 105
  • V547 Expression 'ads->tcpsocket >= 0' is always true. Unsigned type value is always >= 0. check.c 114
  • V547 Expression 'ads->tcpsocket >= 0' is always true. Unsigned type value is always >= 0. check.c 123


Неправильные проверки могут привести к переполнению буфера и, как следствие, к неопределенному поведению программы. Рассмотрим пример, где защита не срабатывает.

Code: Select all

BOOL PrepareService(LPCTSTR ServiceName)
{
  DWORD LeftOfBuffer = sizeof(ServiceKeyBuffer) /
                       sizeof(ServiceKeyBuffer[0]);
  ....
  LeftOfBuffer -= _tcslen(SERVICE_KEY);
  ....
  LeftOfBuffer -= _tcslen(ServiceName);
  ....
  LeftOfBuffer -= _tcslen(PARAMETERS_KEY);
  ....
  
  if (LeftOfBuffer < 0)
  {
    DPRINT1("Buffer overflow for service name: '%s'\n",
            ServiceName);
    return FALSE;
  }  
  ....
}
V547 Expression 'LeftOfBuffer < 0' is always false. Unsigned type value is never < 0. svchost.c 51

По всей видимости, переменную 'LeftOfBuffer' следовало сделать знаковой.



Нередко, из-за беззнаковых переменных неправильно проверяется значения, возвращаемые функциями. Рассмотрим такой код:

Code: Select all

static INT FASTCALL
MenuButtonUp(MTRACKER *Mt, HMENU PtMenu, UINT Flags)
{
  UINT Id;
  ....
  Id = NtUserMenuItemFromPoint(....);
  ....
  if (0 <= Id &&
      MenuGetRosMenuItemInfo(MenuInfo.Self, Id, &ItemInfo) &&
      MenuInfo.FocusedItem == Id)
  ....
}
V547 Expression '0 <= Id' is always true. Unsigned type value is always >= 0. menu.c 2663

Функция NtUserMenuItemFromPoint() может возвращать отрицательное значение (-1). Ошибка возникает из-за того, что переменная 'Id' беззнаковая. Как следствие проверка '0 <= Id' не имеет смысла.



В следующем фрагменте кода неправильно проверяется параметр функции.

Code: Select all

typedef unsigned int GLuint;

const GLubyte *_mesa_get_enabled_extension(
  struct gl_context *ctx, GLuint index)
{
  const GLboolean *base;
  size_t n;
  const struct extension *i;
  if (index < 0)
    return NULL;
  ....
}  
V547 Expression 'index < 0' is always false. Unsigned type value is never < 0. extensions.c 936



Дальше рассматривать предупреждения V547 не интересно. Приведу список оставшихся мест, на которые я обратил внимание:
  • V547 Expression 'index >= 0' is always true. Unsigned type value is always >= 0. st_glsl_to_tgsi.cpp 4013
  • V547 Expression 'index >= 0' is always true. Unsigned type value is always >= 0. st_glsl_to_tgsi.cpp 4023
  • V547 Expression 'index < 0' is always false. Unsigned type value is never < 0. st_glsl_to_tgsi.cpp 4027
  • V547 Expression '(src) < (0)' is always false. Unsigned type value is never < 0. texstore.c 3692
  • V547 Expression '(src) < (0)' is always false. Unsigned type value is never < 0. texstore.c 3759
  • V547 Expression 'CommitReduction >= 0' is always true. Unsigned type value is always >= 0. virtual.c 4784
  • V547 Expression 'Info->nPage < 0' is always false. Unsigned type value is never < 0. scrollbar.c 428
  • V547 Expression 'Entry->Id <= 0xffff' is always true. The value range of unsigned short type: [0, 65535]. res.c 312




Undefined behavior и Unspecified behavior.

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

Неправильный код выглядит так:

Code: Select all

static INLINE int wrap(short f, int shift)
{
  ....
  if (f < (-16 << shift))
  ....
}
V610 Undefined behavior. Check the shift operator '<<. The left operand '-16' is negative. vl_mpeg12_bitstream.c 653

Чему будет равно выражение (-16 << shift) неизвестно. Аналогичные хлипкие места можно наблюдать здесь:
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. jdarith.c 460
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. jdhuff.c 930
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. layer1.c 86
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. layer1.c 90
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. layer1.c 97
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. layer1.c 118
  • V610 Unspecified behavior. Check the shift operator '>>. The left operand is negative ('i' = [-4096..4095]). tabinit.c 269
  • V610 Unspecified behavior. Check the shift operator '>>. The left operand is negative ('i' = [-4096..4095]). tabinit.c 274
  • V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. mppc.c 351
Неправильный спецификатор формата

Рассмотрим несколько неправильных способов использования функций с переменным количеством аргументов для распечатки значений переменных.

Code: Select all

UINT64 Size;
static HRESULT STDMETHODCALLTYPE
CBindStatusCallback_OnProgress(....)
{
  ....
  _tprintf(_T("Length: %ull\n"), This->Size);
  ....
}
V576 Incorrect format. Consider checking the second actual argument of the 'wprintf' function. The argument is expected to be not greater than 32-bit. dwnl.c 228

Чтобы распечатать 64-битную переменную следует писать "%llu", а не "%ull".



Ещё неправильно печатать значения указателя, используя "%u". Для этого существует спецификатор "%p". Впрочем, в коде ниже возможно просто опечатались и там должно быть "%s".

Code: Select all

BOOL CALLBACK EnumPickIconResourceProc(
  HMODULE hModule, LPCWSTR lpszType, 
  LPWSTR lpszName, LONG_PTR lParam)
{
  ....
  swprintf(szName, L"%u", lpszName);
  ....
}
V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. To print the value of pointer the '%p' should be used. dialogs.cpp 66



Очень распространены ошибки, когда совместно используются юникодные и неюникодные строки. Например, чтобы распечатать юникодный символ в функции fprintf() правильно использовать '%C', а не '%c'. Рассмотрим пример неправильного кода:

Code: Select all

int WINAPI WinMain(....)
{
  LPWSTR *argvW = NULL;
  ....
  fprintf(stderr,
          "Unknown option \"%c\" in Repair mode\n",
          argvW[i][j]);
  ....
}
V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. The char type argument is expected. msiexec.c 655

Аналогичные ошибки можно найти здесь:
  • V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. The char type argument is expected. msiexec.c 705
  • V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. The pointer to string of wchar_t type symbols is expected. sminit.c 1831
  • V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. The pointer to string of char type symbols is expected. bootsup.c 600
  • V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. The pointer to string of char type symbols is expected. guiconsole.c 328
  • V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. The pointer to string of char type symbols is expected. guiconsole.c 332
  • V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. The pointer to string of char type symbols is expected. guiconsole.c 378
  • V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. The pointer to string of char type symbols is expected. guiconsole.c 382
Приоритеты операций

Я нашел несколько ошибок, связанных с путаницей в приоритетах операций.

Code: Select all

static HRESULT BindStatusCallback_create(....)
{
  HRESULT hr;
  ....
  if ((hr = SafeArrayGetUBound(sa, 1, &size) != S_OK))
  {
    SafeArrayUnaccessData(sa);
    return hr;
  }
  ....
}
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. httprequest.c 692

Согласно приоритету операций в языке Си/Си++ в начале выполняется сравнение "SafeArrayGetUBound(sa, 1, &size) != S_OK", а только потом происходит присваивание. Впрочем, условие отработает правильно. Неправильно то, что вместо статуса в переменной 'hr' будет храниться 0 или 1. Функция вернёт неправильный статус.



Другая очень похожая ошибка:

Code: Select all

static void symt_fill_sym_info(....)
{
  ....
  if (sym->tag != SymTagPublicSymbol ||
      !(dbghelp_options & SYMOPT_UNDNAME) ||
      (sym_info->NameLen =
         UnDecorateSymbolName(name, sym_info->Name,
           sym_info->MaxNameLen, UNDNAME_NAME_ONLY) == 0))
  ....
}
V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. symbol.c 801

Код читается плохо. Но если присмотреться, то можно заметить, что в начале результат работы функции UnDecorateSymbolName() сравнивается с нулём. Затем результат сравнения помещается в переменную 'sym_info->NameLen'.

Выход за границу массива

Code: Select all

FF_T_WCHAR FileName[FF_MAX_FILENAME];
FF_T_UINT32 FF_FindEntryInDir(....) {
  ....
  FF_T_WCHAR *lastPtr = pDirent->FileName + sizeof(pDirent->FileName);
  ....
  lastPtr[-1] = '\0';
  ....
}
V594 The pointer steps out of array's bounds. ff_dir.c 260

Программист планировал, что 'lastPtr' будет указывать на ячейку памяти после последнего символа в строке. Но это не так. Строка состоит из символов типа WCHAR. Это значит, что прибавляется не количество символов, а размер буфера. Это в 2 раза больше, чем нужно. При записи терминального нуля произойдет выход за границу массива со всеми вытекающими последствиями.

Правильный код, должен был выглядеть так:

Code: Select all

FF_T_WCHAR *lastPtr = pDirent->FileName +
  sizeof(pDirent->FileName) / sizeof(pDirent->FileName[0]);


Весьма опасна в плане выхода за границу массива функция strncat(). Дело в том, что последний аргумент должен указывать не общий размер буфера, а сколько ещё символов в него можно записать. Из-за этого непонимания возникает опасный код:

Code: Select all

void shell(int argc, const char *argv[])
{
  char CmdLine[MAX_PATH];
  ....
  strcpy( CmdLine, ShellCmd );

  if (argc > 1)
  {
    strncat(CmdLine, " /C", MAX_PATH);
  }

  for (i=1; i<argc; i++)
  {
    strncat(CmdLine, " ", MAX_PATH);
    strncat(CmdLine, argv[i], MAX_PATH);
  }
  ....
}
V645 The 'strncat' function call could lead to the 'CmdLine' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. cmds.c 1314

V645 The 'strncat' function call could lead to the 'CmdLine' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. cmds.c 1319

V645 The 'strncat' function call could lead to the 'CmdLine' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. cmds.c 1320

Нет гарантии, что не произойдет выход за границу буфера. Подробнее с этим классом ошибок можно познакомиться в документации (диагностика V645).

Аналогичная беда здесь:

V645 The 'wcsncat' function call could lead to the 'szFileName' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. logfile.c 50

Повторы

Повторы связаны с условиями и бывают двух видов.

Вид первый. В не зависимости от условия выполняются одни и те же действия. Пример:

Code: Select all

void CardButton::DrawRect(HDC hdc, RECT *rect, bool fNormal)
{
  ....
  if(fNormal)
    hOld = SelectObject(hdc, hhi);
  else
    hOld = SelectObject(hdc, hhi);
  ....
}
V523 The 'then' statement is equivalent to the 'else' statement. cardbutton.cpp 86



Другой пример:

Code: Select all

NTSTATUS NTAPI 
CPortPinWavePci::HandleKsStream(IN PIRP Irp)
{
  ....
  if (m_Capture)
    m_Position.WriteOffset += Data;
  else
    m_Position.WriteOffset += Data;
  ....
}
V523 The 'then' statement is equivalent to the 'else' statement. pin_wavepci.cpp 562



Ещё повтор большого участка коде есть вот здесь:

V523 The 'then' statement is equivalent to the 'else' statement. tab.c 1043



Вид второй. Повторяется условие. Получается, что второе условие никогда не выполнится. Пример:

Code: Select all

#define LOCALE_SSHORTDATE 31
#define LOCALE_SLONGDATE 32
MSVCRT__locale_t CDECL MSVCRT__create_locale(....)
{
  ....
  if (time_data[i]==
      LOCALE_SSHORTDATE && !lcid[LC_TIME]) {
    size += ....;
  } else if(time_data[i]==
            LOCALE_SSHORTDATE && !lcid[LC_TIME]) {
    size += ....;
  } else {
  ....
}
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1193, 1195. locale.c 1193

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

Code: Select all

if (time_data[i]==LOCALE_SLONGDATE && !lcid[LC_TIME])


Аналогичные повторные проверки можно найти находятся здесь:
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1225, 1228. locale.c 1225
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1241, 1244. locale.c 1241
Разное

Рассмотрим теперь разносортные ошибки.

Неправильное вычисление количества символов

Code: Select all

typedef struct _UNICODE_STRING {
  USHORT Length;
  USHORT MaximumLength;
  PWSTR  Buffer;
} UNICODE_STRING, *PUNICODE_STRING;

UNICODE_STRING DosDevices =
  RTL_CONSTANT_STRING(L"\\DosDevices\\");

NTSTATUS CreateNewDriveLetterName(....)
{
  ....
  DriveLetter->Buffer[
    sizeof(DosDevices.Buffer) / sizeof(WCHAR)] =
    (WCHAR)Letter;
  ....
}
V514 Dividing sizeof a pointer 'sizeof (DosDevices.Buffer)' by another value. There is a probability of logical error presence. mountmgr.c 164

Как мне кажется выражение "sizeof(DosDevices.Buffer) / sizeof(WCHAR)" должно было подсчитать количество символов в строке. Но 'DosDevices.Buffer' это просто указатель. В результате размер указателя делится на 'sizeof(WCHAR)'. Аналогичные ошибки присутствуют здесь:
  • V514 Dividing sizeof a pointer 'sizeof (DosDevices.Buffer)' by another value. There is a probability of logical error presence. mountmgr.c 190
  • V514 Dividing sizeof a pointer 'sizeof (DosDevices.Buffer)' by another value. There is a probability of logical error presence. symlink.c 937


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

Code: Select all

VOID DisplayEvent(HWND hDlg)
{
  WCHAR szEventType[MAX_PATH];
  WCHAR szTime[MAX_PATH];
  WCHAR szDate[MAX_PATH];
  WCHAR szUser[MAX_PATH];
  WCHAR szComputer[MAX_PATH];
  ....
  ListView_GetItemText(...., sizeof(szEventType)*sizeof(WCHAR));
  ListView_GetItemText(...., sizeof(szDate)*sizeof(WCHAR));
  ListView_GetItemText(...., sizeof(szTime)*sizeof(WCHAR));
  ListView_GetItemText(...., sizeof(szSource)*sizeof(WCHAR));
  ListView_GetItemText(...., sizeof(szCategory)*sizeof(WCHAR));
  ListView_GetItemText(...., sizeof(szEventID)*sizeof(WCHAR));
  ListView_GetItemText(...., sizeof(szUser)*sizeof(WCHAR));
  ListView_GetItemText(...., sizeof(szComputer)*sizeof(WCHAR));
  ....
}
В результате, функция ListView_GetItemText() считает, что размер буфера больше, чем он есть на самом деле. Потенциально это может привести к переполнению буфера.



Результат работы функции не используется

Code: Select all

#define strcmpW(s1,s2) wcscmp((s1),(s2))
static HRESULT WINAPI IEnumDMO_fnNext(....)
{
  ....
  if (Names[count])
    strcmpW(Names[count], szValue);
  ....
}
V530 The return value of function 'wcscmp' is required to be utilized. dmoreg.c 621



Неинициализированная переменная

Code: Select all

HRESULT WINAPI
INetCfgComponentControl_fnApplyRegistryChanges(
  INetCfgComponentControl * iface)
{
  HKEY hKey;
  ....
  if (RegCreateKeyExW(hKey,
      L"SYSTEM\\CurrentControlSet....",
      ....) == ERROR_SUCCESS)
    ....
}
V614 Uninitialized pointer 'hKey' used. Consider checking the first actual argument of the 'RegCreateKeyExW' function. tcpipconf_notify.c 3138

В момент вызова функции RegCreateKeyExW() переменная 'hKey' ещё не инициализирована.

Отбрасываются старшие биты, которые могут что-то значить

Code: Select all

HRESULT WINAPI CRecycleBin::CompareIDs(....)
{
  ....
  return MAKE_HRESULT(SEVERITY_SUCCESS, 0,
   (unsigned short)memcmp(pidl1->mkid.abID,
                          pidl2->mkid.abID,
                          pidl1->mkid.cb));
}
V642 Saving the 'memcmp' function result inside the 'unsigned short' type variable is inappropriate. The significant bits could be lost breaking the program's logic. recyclebin.cpp 542

Этот тип ошибки весьма неочевиден. Предлагаю познакомиться с описанием диагностики V642, чтобы понять суть проблемы. Если кратко, то беда в том, что функция memcmp() не обязательно возвращает только значения -1, 0 и 1. Она может вернуть, например, число 0x100000. При приведении этого числа к типу "unsigned short" оно превратится в 0.

"Одноразовые" циклы

Встретилось несколько очень странных циклов. В них нет оператора 'continue'. Но при этом в них есть безусловный 'break'. Это значит, что тело циклов выполняется только один раз. Рассмотрим пример такого цикла.

Code: Select all

VOID NTAPI IKsPin_PinCentricWorker(IN PVOID Parameter)
{
  ....
  do
  {
    DPRINT("IKsPin_PinCentricWorker calling "
           "Pin Process Routine\n");
    Status =
      This->Pin.Descriptor->Dispatch->Process(&This->Pin);
    DPRINT("IKsPin_PinCentricWorker Status %lx, "
           "Offset %lu Length %lu\n", Status,
           This->LeadingEdgeStreamPointer.Offset,
           This->LeadingEdgeStreamPointer.Length);
    break;
  } while(This->IrpCount);
}  
V612 An unconditional 'break' within a loop. pin.c 1839

Аналогичные странные циклы:
  • V612 An unconditional 'break' within a loop. regexp.c 3633
  • V612 An unconditional 'break' within a loop. hlpfile.c 1131


Странное

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

Code: Select all

BOOLEAN NTAPI Ext2MakeNewDirectoryEntry(....)
{
  ....
  MinLength = HeaderLength + NameLength;
  MinLength = (HeaderLength + NameLength + 3) & 0xfffffffc;
  ....
}
V519 The 'MinLength' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 948, 949. metadata.c 949

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

Заключение

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

Следите в нашем твиттере за новыми интересными подвигами PVS-Studio в борьбе с багами. Там мы также публикуем ссылки на интересные статьи по тематике программирования на языке Си/Си++ и смежным темам.

lastar
Posts: 16
Joined: Sat Mar 24, 2012 1:01 pm

Отчеты о проверке исходников ReactOS на ошибки PVS-Studio

Post by lastar » Sat Jun 29, 2013 9:20 am

Отчеты о проверке исходного кода ReactOS на ошибки : : PVS-Studio, Cppcheck, MSVC 2012 Ultimate Code Analysis.

PVS-Studio http://jira.reactos.org/browse/CORE-7322

MSVC 2012 Ultimate Code Analysis http://jira.reactos.org/browse/CORE-7325

Cppcheck http://jira.reactos.org/browse/CORE-7292

lastar
Posts: 16
Joined: Sat Mar 24, 2012 1:01 pm

Re: Отчеты о проверке исходников ReactOS на ошибки PVS-Studi

Post by lastar » Tue Jul 02, 2013 2:58 pm

Вот обнаружил новую статью от разработчиков PVS-Studio в ней есть примеры обнаруженных ошибок в ReactOS http://www.viva64.com/ru/b/0203/

lastar
Posts: 16
Joined: Sat Mar 24, 2012 1:01 pm

Re: Отчеты о проверке исходников ReactOS на ошибки PVS-Studi

Post by lastar » Sat Sep 14, 2013 3:02 pm

Провел свежие проверки исходных кодов во всех трех отчетах в jira .

Post Reply

Who is online

Users browsing this forum: No registered users and 1 guest