Продолжение читать тутПроверив код ReactOS, я смог исполнить сразу три своих желания. Во-первых, давно хотелось написать статью об обыкновенном проекте. Не интересно проверять код таких проектов, как Chromium. Он слишком качественен и, на поддержание этого качества тратятся ресурсы, недоступные в обыкновенных проектах. Во-вторых, появился хороший пример, на котором можно показать, как необходим статический анализ в большом проекте, особенно если он разрабатывается разнородным распределенным коллективом. В-третьих, я получил подтверждение, что PVS-Studio становится всё лучше и полезнее...
ReactOS and PVS-Studio code analyzer
-
- Posts: 706
- Joined: Sun Mar 16, 2008 11:26 am
- Location: Russia, Stavropol
- Contact:
ReactOS and PVS-Studio code analyzer
-
- Posts: 66
- Joined: Wed Jan 20, 2010 9:46 am
Re: Анализ кода ReactOS в PVS-Studio
-
- Posts: 3
- Joined: Thu Sep 01, 2011 1:28 pm
Re: Анализ кода ReactOS в PVS-Studio
Евгений Рыжков
ООО "СиПроВер"
Я не видел результатов анализа ReactOS с помощью Coverity, но подозреваю, что ошибок там обнаружено вовсе не несколько, а тысячи…Но, тем не менее, там, где благодаря Coverity «было найдено несколько новых ошибок», PVS-Studio находит их целый вагон и маленькую тележку.
Даже тот статический анализатор, что встроен в GCC, выдаёт множество предупреждений. Только вот мало желающих заниматься таким "негламурным" занятием, как исправление ошибок в существующем коде.
Полностью согласен; как уже писал в ветке про "бесплатный анал", я думаю, что разработчики должны быть more anal насчёт качества кода. :)Считаю, что статический анализ кода должен являться обязательным элементом цикла разработки ReactOS и других крупных проектов.
-
- Posts: 3
- Joined: Thu Sep 01, 2011 1:28 pm
Re: Анализ кода ReactOS в PVS-Studio
Евгений Рыжков
ООО "СиПроВер"
Re: Анализ кода ReactOS в PVS-Studio
Как оказалось, не все программы статического анализа одинаково полезны, большинство из коммитов, сделанных Lentin'ом по приведённому на Хабре списку (созданному в PVS) были ревертануты. Это видно здесь, здесь, здесь, и здесь. Может люди, сведущие в программировании прояснят ситуацию? С Coverity по крайней мере таких проблем видно не было...EvgeniyRyzhkov wrote:Перевод статьи ещё не готов. Однако, если кому-то из иностранных товарищей хочется поработать со списком ошибок, то я подготовил для них вот этот http://www.viva64.com/external-pictures ... nicode.txt . В нём переведены разъяснения к ошибкам.
-
- Posts: 3
- Joined: Thu Sep 01, 2011 1:28 pm
Re: Анализ кода ReactOS в PVS-Studio
В первом случае, это мы ввели в заблуждение. Невнимательно посмотрели текст сообщения:
Код: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);
Почему не понравилась вторая правка - не понятно. Дело в том, что sizeof(dest) возвращает размер указателя, а не массива.
Вот этот код:
Code: Select all
Mapdesc::identify( REAL dest[MAXCOORDS][MAXCOORDS] )
{
memset( dest, 0, sizeof(dest));
Code: Select all
memset( dest, 0, sizeof(void *));
Третий случай. Опять не понятно, что не понравилось.
Думаю, дело было так. Увидели, что пользователем внесен ряд правок. И самая первая – неверная. И от греха подальше отклонили его правки. И правильно сделали. Надо подходить к правкам с головой и не так. Мы изучали проект поверхностно. И к каждой правке надо подходить с вниманием. Множество диагностик мы вообще не привели в списке, чтобы снизить возможность вот таких вот ложных советов.
-
- Posts: 11
- Joined: Fri Sep 16, 2011 6:28 pm
PVS-Studio: анализируем код операционной системы ReactOS
Аннотация
Проверив код 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.
- Wikipedia. ReactOS.
- Статья в Компьютерре. ReactOS: догнать и перегнать.
- Lurkmore. ReactOS.
Теперь поговорим о вагоне ошибок, которые повстречались мне в коде ReactOS. Конечно, все они в статью не войдут. Здесь я выложил текстовый файл с описанием ошибок, которые я заметил в ходе анализа. Файл содержит диагностические сообщения с именами файлов и номерами строк. Также я выделил ошибки в короткий код и дал некоторые комментарии. Поэтому тем, кто захочет сделать правки в ReactOS, лучше руководствоваться этим файлом, а не статьёй.
А еще лучше скачать и самим проверить проект с помощью PVS-Studio. Ведь я не знаком с проектом и выписывал только те ошибки, которые мне понятны. По поводу многих фрагментов кода я не знаю, содержат они ошибки или нет. Так что мой анализ достаточно поверхностен. Ключ для проверки выделим.
Ошибки, которые можно встретить в ReactOS разнообразнейшие. Просто зоопарк. Есть опечатки из одного символа.
Code: Select all
BOOL WINAPI GetMenuItemInfoA(...)
{
...
mii->cch = mii->cch;
...
}
А вот другая опечатка в один символ. В этот раз некорректно работает сравнение двух имён.
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))
...
}
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)) {
...
}
Кстати, в 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))
{
...
}
Code: Select all
(ErrorCode == 10065) && (ErrorCode == 10051)
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);
...
}
Встречаются просто странные фрагменты. Скорее всего, это недописанные и забытые участки кода.
Code: Select all
if (ERROR_SUCCESS == hres)
{
Names[count] = HeapAlloc(GetProcessHeap(), 0, strlenW(szValue) + 1);
if (Names[count])
strcmpW(Names[count], szValue);
}
Имеются ошибки, связанные с приоритетом операций.
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;
Code: Select all
BOOLEAN
CTEScheduleEvent(PCTE_DELAYED_EVENT Event,
PVOID Context)
{
...
if (!Event->Queued);
{
Event->Queued = TRUE;
Event->Context = Context;
ExQueueWorkItem(&Event->WorkItem, CriticalWorkQueue);
}
...
}
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);
...
}
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))
Вот только знание этой практики не мешает другому человеку ошибиться альтернативным способом.
Code: Select all
static DWORD CALLBACK
RegistrationProc(LPVOID Parameter)
{
...
if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR,
UnknownError,
sizeof(UnknownError) /
sizeof(UnknownError[0] - 20)))
...
}
К чему я все эти примеры? А к тому, что никто из нас программистов не идеален. И ни стандарт кодирования, ни технологии программирования, ни самоконтроль не гарантируют отсутствие ошибок в коде.
В крупных проектах просто необходимы такие вспомогательные технологии, как динамический и статический анализ. Подчеркну:
Считаю, что статический анализ кода должен являться обязательным элементом цикла разработки 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);
...
}
Другим хорошим примером может служить большое количество ошибок очистки памяти, которые я увидел 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));
...
}
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);
...
}
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();
...
[ external image ]
[ external image ]
[ external 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 == ' '))
...
}
Примечание для сторонников табуляции. Не надо мне вновь рассказывать, как правильно использовать табуляции. И это не мой код. Смотрите, вот есть вполне конкретный проект ReactOS. В нем есть плохо отформатированный код. Подумайте, какие действия позволят сделать так, чтобы новый программист мог открыть код проекта и не гадать, какой размер символа табуляции ему необходимо установить в настройках редактора. Мысли в духе "нужно сразу было писать правильно" практической ценности не имеют.
Библиографический список
- Выпуск новостей ReactOS N79. Статический анализ в Coverity. http://www.viva64.com/go.php?url=722
- Пора завязывать использовать пробелы вместо табуляции в коде. http://www.viva64.com/go.php?url=725
- Пора завязывать использовать символы табуляции в коде. http://www.viva64.com/go.php?url=726
- ReactOS. Coding Style. http://www.viva64.com/go.php?url=724
- Google C++ Style Guide. http://www.viva64.com/go.php?url=679
Re: PVS-Studio: анализируем код операционной системы ReactOS
Здравствуйте Андрей,
Ваша компания готова бесплатно предоставить PVS Studio для анализа исходного когда открытого некоммерческого проекта ReactOS?
Coverity предоставляет такие услуги открытым проектам бесплатно. Мы не требуем и никогда не будем требовать какие-либо деньги за ReactOS, поэтому подавляющее большинство коммерческих компаний предлагает свои услуги открытым проектам бесплатно. То же касается участия в выставках и конференциях.
С уважением,
Президент Фонда Реактос
Брагин А.В.
ReactOS Project Lead
-
- Posts: 11
- Joined: Fri Sep 16, 2011 6:28 pm
Re: PVS-Studio: анализируем код операционной системы ReactOS
Мы давно предоставляем и продолжаем предоставлять возможность проверить бесплатные open-source проекты. Да, эта возможность ограничена по времени. Мы не видим смысла просто так раздавать ключи на долгий период. Однако, если такие пользователи сообщают об ошибках или предлагают новые диагностические правила, то мы с радостью продлеваем им ключи и общаемся с ними. Приятным примером такого взаимодействия является проект FlylinkDC++.
P.S. Только прошу не надо постоянно писать про чистый альтруизм и желание сделать мир лучше. Проекты подобного размера могут существовать только тогда, когда это кому то надо. Это действует на студентов, но никак не на профессиональных разработчиков. WooS тоже будет всегда бесплатен?
----
С уважением, Андрей Карпов
к.ф.-м.н., Технический директор
ООО "Системы программной верификации"
Сайт: http://www.viva64.com/ru/
E-Mail: karpov[[@]]viva64.com
Тел.: +7 (4872) 38-59-95 (GMT + 03:00)
-
- Posts: 11
- Joined: Fri Sep 16, 2011 6:28 pm
Большой отчет о повторной проверке проекта ReactOS
[ external image ]
Напомним, что такое ReactOS. Это свободная и открытая операционная система, основанная на принципах архитектуры Windows NT. Система была разработана с нуля и таким образом не основана на Linux и не имеет ничего общего с архитектурой UNIX. Основной целью проекта ReactOS является создание бинарно-совместимой с Windows операционной системы. Такая система позволяет выполнять Windows-совместимые приложения и драйвера так, как если бы они выполнялись в самой Windows.
Ранее мы уже проверяли этот проект. Результаты этой проверки описаны в заметке "PVS-Studio: анализируем код операционной системы ReactOS". Вновь проверив проект, мы нашли много новых ошибок и подозрительных участков кода. Это хорошо демонстрирует, чтостатический анализ кода должен выполняться регулярно, а не от случая к случаю! Это существенно сократит количество ошибок ещё на этапе кодирования. А значит, существенно сократится время на ликвидацию обнаруженных ошибок.
Хочу предупредить, я опишу далеко не все места в проекте, на которые стоит обратить внимание. ReactOS стал большим мальчиком. В состав решения (solution) входит 803 проекта. Для них анализатор PVS-Studio выдал множество предупреждений общего назначения:
- 1320 предупреждений первого уровня;
- 814 предупреждений второго уровня;
- 2753 предупреждений третьего уровня.
Демонстрационной версии 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;
....
}
Присваивание затирает предыдущее значение члена 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))
....
}
Два раза выполняется проверка "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;
}
....
}
Правильная проверка по всей видимости должна выглядеть так: 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;
}
....
}
И ещё:
Code: Select all
static inline BOOL is_unc_path(const WCHAR *str) {
return (str[0] == '\\' && str[0] == '\\');
}
Кстати, это ошибка не исправлена с прошлого раза. В предыдущей статье я не описал её, хотя она записана у меня в базу примеров ошибок. Уже не помню почему. Наверное, не хотел, чтобы статья была слишком большой. А поскольку сами разработчики, видимо, так и не запускали PVS-Studio, то ошибка спокойно существует в коде как минимум пару лет.
И ещё:
Code: Select all
VOID NTAPI UniAtaReadLunConfig(....)
{
if(!LunExt->IdentifyData.SectorsPerTrack ||
!LunExt->IdentifyData.NumberOfCylinders ||
!LunExt->IdentifyData.SectorsPerTrack)
....
}
Думаю, ошибка хорошо заметна. Как надо исправить код, я не знаю.
Потерпите. Но у меня есть и другие ошибки близнецы. Что делать... Это очень типичные ошибки в программах.
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)) {
....
}
Здесь одно из сравнений "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);
....
}
Обратите внимание на ';' после оператора '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;
}
Видимо, имя одного из операндов в выражении задано неправильно.
А вот опечатка, из-за которой неправильно будет вычислен размер прямоугольника.
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;
}
Я почти уверен, что здесь должно быть написано: "rect->Y = gdip_round(rectf.Y);". Если это вдруг не так, то здесь бы не помешал бы комментарий.
Фрагмент кода, где переменная присваивается сама себе:
Code: Select all
DWORD WINAPI
DdGetDriverInfo(LPDDHAL_GETDRIVERINFODATA pData)
{
....
pUserColorControl->dwFlags = pUserColorControl->dwFlags;
....
}
Присваивание не имеет смысла. Скорее всего, выражение не дописано или что-то перепутано. Аналогичная ошибка здесь:
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;
....
}
}
Если в операторе '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;
}
....
}
В момент вызова функции 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);
}
....
}
Функция wcscpy() вызывается только в том случае, если переменная 'lpMsgBuf' равна нулю. Эта переменная передаётся как аргумент в функцию 'wcscpy'. Передавать ноль в функцию 'wcscpy' это хулиганство.
А вот другой хулиган издевается над кошкой функцией strstr():
Code: Select all
VOID WinLdrSetupEms(IN PCHAR BootOptions)
{
PCHAR RedirectPort;
....
if (RedirectPort)
{
....
}
else
{
RedirectPort = strstr(RedirectPort, "usebiossettings");
....
}
За компанию, функция _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;
}
}
....
}
К моменту вызова функции _wcsicmp() указатель tmpPrefix будет равен нулю.
Разыменовывание потенциально нулевого указателя
Очень много фрагментов кода, где вначале указатель разыменовывается, а только потом проверяется на равенство нулю. Это не всегда ошибка. Возможно, указатель не может быть равен нулю и проверка просто лишняя. Но, как правило, такой код появляется из-за невнимательности и он не верен. И работает он только до того момента, пока злосчастный указатель, из-за стечения обстоятельств, вдруг не станет равен нулю.
Я рассмотрю здесь только один простой пример:
Code: Select all
static BOOL LookupSidInformation(....)
{
....
DomainName = &PolicyAccountDomainInfo->DomainName;
SidNameUse = (PolicyAccountDomainInfo != NULL ?
SidTypeGroup : SidTypeUser);
....
}
Смотрите, в начале указатель '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;
}
Другие места, где используется этот неправильный макрос:
- 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;
}
....
}
Цикл предназначен для поиска в массиве 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; }
....
}
Переменная '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;
}
....
}
По всей видимости, переменную '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)
....
}
Функция 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 не интересно. Приведу список оставшихся мест, на которые я обратил внимание:
- 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))
....
}
Чему будет равно выражение (-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);
....
}
Чтобы распечатать 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);
....
}
Очень распространены ошибки, когда совместно используются юникодные и неюникодные строки. Например, чтобы распечатать юникодный символ в функции 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 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;
}
....
}
Согласно приоритету операций в языке Си/Си++ в начале выполняется сравнение "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))
....
}
Код читается плохо. Но если присмотреться, то можно заметить, что в начале результат работы функции 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';
....
}
Программист планировал, что '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 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);
....
}
Другой пример:
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. 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 {
....
}
Как мне кажется, вторая проверка должна была выглядеть так:
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;
....
}
Как мне кажется выражение "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));
....
}
Результат работы функции не используется
Code: Select all
#define strcmpW(s1,s2) wcscmp((s1),(s2))
static HRESULT WINAPI IEnumDMO_fnNext(....)
{
....
if (Names[count])
strcmpW(Names[count], szValue);
....
}
Неинициализированная переменная
Code: Select all
HRESULT WINAPI
INetCfgComponentControl_fnApplyRegistryChanges(
INetCfgComponentControl * iface)
{
HKEY hKey;
....
if (RegCreateKeyExW(hKey,
L"SYSTEM\\CurrentControlSet....",
....) == ERROR_SUCCESS)
....
}
В момент вызова функции 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, чтобы понять суть проблемы. Если кратко, то беда в том, что функция 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. 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;
....
}
Переменной 'MinLength' два раза подряд присваиваются разные значения. Возможно, это помогает при отладке. Не знаю. Я бы посчитал это ошибкой, но таких участков кода много. Перечислять их не буду, так как заметка и без того получилась огромной.
Заключение
Я затрудняюсь сделать глубокомысленные выводы. ReactOS это быстрорастущий и развивающийся проект. Как следствие, он содержит большое количество ошибок. Как видно из этой заметки, статический анализ может выявить в таком проекте большое количество ошибок. А если бы его использовать регулярно, то польза была просто неоценима.
Следите в нашем твиттере за новыми интересными подвигами PVS-Studio в борьбе с багами. Там мы также публикуем ссылки на интересные статьи по тематике программирования на языке Си/Си++ и смежным темам.
Отчеты о проверке исходников ReactOS на ошибки PVS-Studio
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
Re: Отчеты о проверке исходников ReactOS на ошибки PVS-Studi
Re: Отчеты о проверке исходников ReactOS на ошибки PVS-Studi
Who is online
Users browsing this forum: No registered users and 2 guests