Проект ReactOS активно продолжает развиваться. Один из разработчиков, участвующий в этом проекте, предложил вновь проверить исходный код, так как кодовая база быстро увеличивается. С удовольствием сделаем это. Нам симпатичен этот проект. Мы будем рады, если статья поможет исправить ряд недочетов. Для проверки используется анализатор кода PVS-Studio версии 5.02.
Напомним, что такое ReactOS. Это свободная и открытая операционная система, основанная на принципах архитектуры Windows NT. Система была разработана с нуля и таким образом не основана на Linux и не имеет ничего общего с архитектурой UNIX. Основной целью проекта ReactOS является создание бинарно-совместимой с Windows операционной системы. Такая система позволяет выполнять Windows-совместимые приложения и драйвера так, как если бы они выполнялись в самой Windows.
Ранее мы уже проверяли этот проект. Результаты этой проверки описаны в заметке "PVS-Studio: анализируем код операционной системы ReactOS". Вновь проверив проект, мы нашли много новых ошибок и подозрительных участков кода. Это хорошо демонстрирует, что статический анализ кода должен выполняться регулярно, а не от случая к случаю! Это существенно сократит количество ошибок ещё на этапе кодирования. А значит, существенно сократится время на ликвидацию обнаруженных ошибок.
Хочу предупредить, я опишу далеко не все места в проекте, на которые стоит обратить внимание. ReactOS стал большим мальчиком. В состав решения (solution) входит 803 проекта. Для них анализатор PVS-Studio выдал множество предупреждений общего назначения:
Естественно, у меня нет сил взять и внимательно изучить все эти предупреждения. Я укажу только на наиболее подозрительные места, на которых остановился мой взгляд. Наверняка, есть другие предупреждения, которые заслуживают не меньшего внимания. А ещё есть диагностики, связанные с 64-битными ошибками и микро-оптимизациями. Их я вообще не изучал.
Демонстрационной версии PVS-Studio будет недостаточно, чтобы изучить 4887 предупреждений. Однако, мы дружелюбно относимся к открытым проектам. Если разработчики ReactOS обратятся к нам, мы бесплатно предоставим на время им наш инструмент.
PVS-Studio хорошо выявляет разнообразные опечатки. Можно сказать, это его конёк. Это очень полезная функциональность, так как опечатки есть в любом проекте. Давайте посмотрим, что на эту тему есть в ReactOS.
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;. Но я, конечно, не ручаюсь, так как не знаю, как работает этот код.
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' должен сравниваться с другой константой. Или это сравнение лишнее.
Рассмотрим аналогичную ошибку:
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[i].type' to the left and to the right of the '!=' operator. translate_generic.c 776
Правильная проверка по всей видимости должна выглядеть так: src->channel[i].type != dst->channel[i].type.
И ещё одна аналогичная ошибка:
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
И ещё:
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, то ошибка спокойно существует в коде как минимум пару лет.
И ещё:
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
Думаю, ошибка хорошо заметна. Как надо исправить код, я не знаю.
Потерпите. Но у меня есть и другие ошибки близнецы. Что делать... Это очень типичные ошибки в программах.
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
Наконец, мы добрались до другой разновидности опечаток. Это лишняя точка с запятой ';', которая всё портит.
int BlockEnvToEnvironA(void)
{
....
for (envptr--; envptr >= _environ; envptr--);
free(*envptr);
....
}V529 Odd semicolon ';' after 'for' operator. environ.c 67
Обратите внимание на ';' после оператора 'for'. Как результат функция free() будет вызвана только один раз. Это приведет к утечкам памяти. Также произойдет освобождение участка памяти, который не планировалось освобождать. Ошибочный код сейчас работает так:
free(envptr >= _environ ? _environ[-1] : envptr);Аналогичные точки с запятой можно увидеть здесь:
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
Видимо, имя одного из операндов в выражении задано неправильно.
А вот опечатка, из-за которой неправильно будет вычислен размер прямоугольника.
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);". Если это вдруг не так, то здесь бы не помешал бы комментарий.
Фрагмент кода, где переменная присваивается сама себе:
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.
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' используется и в других местах:
Другой фрагмент кода, где переменная к моменту её использования остаётся равной нулю:
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:
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():
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() тоже пострадала:
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 будет равен нулю.
Очень много фрагментов кода, где вначале указатель разыменовывается, а только потом проверяется на равенство нулю. Это не всегда ошибка. Возможно, указатель не может быть равен нулю и проверка просто лишняя. Но, как правило, такой код появляется из-за невнимательности и он не верен. И работает он только до того момента, пока злосчастный указатель, из-за стечения обстоятельств, вдруг не станет равен нулю.
Я рассмотрю здесь только один простой пример:
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() и ограничился изготовлением говномакроса. Этот макрос выглядит так:
#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:
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;". Остальные строки кода будут выполнены всегда!
Другие места, где используется этот неправильный макрос:
Рассмотрим случай, когда всегда истинное условие может привести к бесконечному циклу.
#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 является знаковой переменной. Это не так. Вернее это зависит от реализации библиотеки.
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.
Аналогичные ошибки при работе с сокетами находятся здесь:
Неправильные проверки могут привести к переполнению буфера и, как следствие, к неопределенному поведению программы. Рассмотрим пример, где защита не срабатывает.
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' следовало сделать знаковой.
Нередко, из-за беззнаковых переменных неправильно проверяется значения, возвращаемые функциями. Рассмотрим такой код:
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' не имеет смысла.
В следующем фрагменте кода неправильно проверяется параметр функции.
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 не интересно. Приведу список оставшихся мест, на которые я обратил внимание:
Нельзя сдвигать отрицательные числа. Нельзя даже в том случае, если вам кажется, что такой код давно и успешно работает. Он неверен. Он приводит к неопределенному или к неуточненному поведению. Проблема может проявить себя при смене платформы, при смене компилятора или при изменении ключей оптимизации. Подробно о сдвигах я писал в статье "Не зная брода, не лезь в воду. Часть третья".
Неправильный код выглядит так:
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) неизвестно. Аналогичные хлипкие места можно наблюдать здесь:
Рассмотрим несколько неправильных способов использования функций с переменным количеством аргументов для распечатки значений переменных.
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".
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'. Рассмотрим пример неправильного кода:
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
Аналогичные ошибки можно найти здесь:
Я нашел несколько ошибок, связанных с путаницей в приоритетах операций.
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. Функция вернёт неправильный статус.
Другая очень похожая ошибка:
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'.
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 раза больше, чем нужно. При записи терминального нуля произойдет выход за границу массива со всеми вытекающими последствиями.
Правильный код, должен был выглядеть так:
FF_T_WCHAR *lastPtr = pDirent->FileName +
sizeof(pDirent->FileName) / sizeof(pDirent->FileName[0]);Весьма опасна в плане выхода за границу массива функция strncat(). Дело в том, что последний аргумент должен указывать не общий размер буфера, а сколько ещё символов в него можно записать. Из-за этого непонимания возникает опасный код:
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
Повторы связаны с условиями и бывают двух видов.
Вид первый. В не зависимости от условия выполняются одни и те же действия. Пример:
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
Другой пример:
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
Вид второй. Повторяется условие. Получается, что второе условие никогда не выполнится. Пример:
#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
Как мне кажется, вторая проверка должна была выглядеть так:
if (time_data[i]==LOCALE_SLONGDATE && !lcid[LC_TIME])Аналогичные повторные проверки можно найти находятся здесь:
Рассмотрим теперь разносортные ошибки.
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)'. Аналогичные ошибки присутствуют здесь:
А вот другой случай неправильного вычисления количества символов в строках. Вместо деления, написано умножение:
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() считает, что размер буфера больше, чем он есть на самом деле. Потенциально это может привести к переполнению буфера.
#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
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' ещё не инициализирована.
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'. Это значит, что тело циклов выполняется только один раз. Рассмотрим пример такого цикла.
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
Аналогичные странные циклы:
Есть фрагменты кода, которые, наверное, не являются ошибкой. Они просто очень странные. Приведу пример странного места:
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 в борьбе с багами. Там мы также публикуем ссылки на интересные статьи по тематике программирования на языке Си/Си++ и смежным темам.