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

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



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

Picture 1

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

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

Хочу предупредить, я опишу далеко не все места в проекте, на которые стоит обратить внимание. ReactOS стал большим мальчиком. В состав решения (solution) входит 803 проекта. Для них анализатор PVS-Studio выдал множество предупреждений общего назначения:

  • 1320 предупреждений первого уровня;
  • 814 предупреждений второго уровня;
  • 2753 предупреждений третьего уровня.

Естественно, у меня нет сил взять и внимательно изучить все эти предупреждения. Я укажу только на наиболее подозрительные места, на которых остановился мой взгляд. Наверняка, есть другие предупреждения, которые заслуживают не меньшего внимания. А ещё есть диагностики, связанные с 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);

Аналогичные точки с запятой можно увидеть здесь:

  • V529 Odd semicolon ';' after 'for' operator. environ.c 119
  • V529 Odd semicolon ';' after 'for' operator. environ.c 171

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

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' используется и в других местах:

  • 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

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

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

Передача нулевого указателя в функцию

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' разыменовывается. А потом вдруг проверяется на равенство нулю. Часто такой код появляется в ходе стремительного рефакторинга. Использование переменных перемещается в коде в то место, где эта переменная ещё не проверена.

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

  • V595 The 'oldRelations' pointer was utilized before it was verified against nullptr. Check lines: 216, 246. pnp.c 216
  • V595 The 'Op->Common.Value.Arg' pointer was utilized before it was verified against nullptr. Check lines: 531, 554. dswload.c 531
  • V595 The 'OutOp' pointer was utilized before it was verified against nullptr. Check lines: 325, 346. dswexec.c 325
  • V595 The 'Poll' pointer was utilized before it was verified against nullptr. Check lines: 63, 66. select.c 63
  • V595 The 'pEdit' pointer was utilized before it was verified against nullptr. Check lines: 480, 491. editstream.c 480
  • V595 The 'plpOptions[curStream]' pointer was utilized before it was verified against nullptr. Check lines: 1629, 1630. api.c 1629
  • V595 The 'pThis' pointer was utilized before it was verified against nullptr. Check lines: 454, 461. atlwin.h 454
  • V595 The 'pThis' pointer was utilized before it was verified against nullptr. Check lines: 639, 646. atlwin.h 639
  • V595 The 'DeviceObject' pointer was utilized before it was verified against nullptr. Check lines: 6870, 6877. class.c 6870
  • V595 The 'extName' pointer was utilized before it was verified against nullptr. Check lines: 128, 141. assoc.c 128
  • V595 The 'FileList' pointer was utilized before it was verified against nullptr. Check lines: 775, 791. filecomp.c 775
  • V595 The 'ident' pointer was utilized before it was verified against nullptr. Check lines: 449, 462. set.c 449
  • V595 The 'psp' pointer was utilized before it was verified against nullptr. Check lines: 2705, 2714. propsheet.c 2705
  • V595 The 'lpItem' pointer was utilized before it was verified against nullptr. Check lines: 4256, 4269. listview.c 4256
  • V595 The 'lpFindInfo' pointer was utilized before it was verified against nullptr. Check lines: 6199, 6203. listview.c 6199
  • V595 The 'item' pointer was utilized before it was verified against nullptr. Check lines: 1461, 1463. treeview.c 1461
  • V595 The 'file' pointer was utilized before it was verified against nullptr. Check lines: 2799, 2802. file.c 2799
  • V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 976, 1006. cryptnet_main.c 976
  • V595 The 'advanced' pointer was utilized before it was verified against nullptr. Check lines: 436, 451. main.c 436
  • V595 The 'compiland' pointer was utilized before it was verified against nullptr. Check lines: 389, 396. symbol.c 389
  • V595 The 'func' pointer was utilized before it was verified against nullptr. Check lines: 468, 471. symbol.c 468
  • V595 The 'compiland' pointer was utilized before it was verified against nullptr. Check lines: 589, 594. symbol.c 589
  • V595 The 'pMapper' pointer was utilized before it was verified against nullptr. Check lines: 822, 847. createdevenum.c 822
  • V595 The 'psh.phpage' pointer was utilized before it was verified against nullptr. Check lines: 2475, 2494. advprop.c 2475
  • V595 The 'DevAdvPropInfo' pointer was utilized before it was verified against nullptr. Check lines: 2480, 2508. advprop.c 2480
  • V595 The 'DeviceID' pointer was utilized before it was verified against nullptr. Check lines: 296, 303. enumdevices.c 296
  • V595 The 'DeviceObject' pointer was utilized before it was verified against nullptr. Check lines: 4279, 4284. disk.c 4279
  • V595 The 'device->hwbuf' pointer was utilized before it was verified against nullptr. Check lines: 917, 927. mixer.c 917
  • V595 The 'PtrNewFileObject' pointer was utilized before it was verified against nullptr. Check lines: 306, 322. create.c 306
  • V595 The 'PtrSourceFCB->FCBName' pointer was utilized before it was verified against nullptr. Check lines: 2793, 2812. metadata.c 2793
  • V595 The 'FileObject' pointer was utilized before it was verified against nullptr. Check lines: 54, 60. fastio.c 54
  • V595 The 'FileObject' pointer was utilized before it was verified against nullptr. Check lines: 663, 680. fastio.c 663
  • V595 The 'FileObject' pointer was utilized before it was verified against nullptr. Check lines: 733, 749. fastio.c 733
  • V595 The 'PtrCCB' pointer was utilized before it was verified against nullptr. Check lines: 1018, 1021. fastio.c 1018
  • V595 The 'PtrCCB' pointer was utilized before it was verified against nullptr. Check lines: 1093, 1102. fastio.c 1093
  • V595 The 'pData' pointer was utilized before it was verified against nullptr. Check lines: 330, 340. inode.c 330
  • V595 The 'ext2_bdl' pointer was utilized before it was verified against nullptr. Check lines: 532, 537. inode.c 532
  • V595 The 'ext2_bdl' pointer was utilized before it was verified against nullptr. Check lines: 600, 615. inode.c 600
  • V595 The 'IrpContext' pointer was utilized before it was verified against nullptr. Check lines: 922, 925. finfo.c 922
  • V595 The 'IrpContext' pointer was utilized before it was verified against nullptr. Check lines: 396, 399. volume.c 396
  • V595 The 'rwContext' pointer was utilized before it was verified against nullptr. Check lines: 224, 235. fbtrwr.c 224
  • V595 The 'DataSize' pointer was utilized before it was verified against nullptr. Check lines: 695, 699. registry.c 695
  • V595 The 'DataSize' pointer was utilized before it was verified against nullptr. Check lines: 733, 737. registry.c 733
  • V595 The 'mm' pointer was utilized before it was verified against nullptr. Check lines: 287, 290. pb_bufmgr_mm.c 287
  • V595 The 'pool' pointer was utilized before it was verified against nullptr. Check lines: 315, 320. pb_bufmgr_pool.c 315
  • V595 The 'cache' pointer was utilized before it was verified against nullptr. Check lines: 186, 189. u_cache.c 186
  • V595 The 'cache' pointer was utilized before it was verified against nullptr. Check lines: 221, 224. u_cache.c 221
  • V595 The 'src' pointer was utilized before it was verified against nullptr. Check lines: 163, 166. u_surface.c 163
  • V595 The 'graphics' pointer was utilized before it was verified against nullptr. Check lines: 2239, 2255. graphics.c 2239
  • V595 The 'vlist' pointer was utilized before it was verified against nullptr. Check lines: 69, 73. trimvertpool.cc 69
  • V595 The 'vlist' pointer was utilized before it was verified against nullptr. Check lines: 88, 93. trimvertpool.cc 88
  • V595 The 'LocalItemState' pointer was utilized before it was verified against nullptr. Check lines: 64, 70. parser.c 64
  • V595 The 'sd->zone_mgr' pointer was utilized before it was verified against nullptr. Check lines: 246, 249. security.c 246
  • V595 The 'entitySet' pointer was utilized before it was verified against nullptr. Check lines: 519, 535. ipstats_reactos.c 519
  • V595 The 'disp' pointer was utilized before it was verified against nullptr. Check lines: 509, 515. jscript.c 509
  • V595 The 'a_cBuffer' pointer was utilized before it was verified against nullptr. Check lines: 888, 893. debugger.c 888
  • V595 The 'AutomationTableB' pointer was utilized before it was verified against nullptr. Check lines: 1903, 1905. api.c 1903
  • V595 The 'AutomationTableB' pointer was utilized before it was verified against nullptr. Check lines: 1952, 1954. api.c 1952
  • V595 The 'AutomationTableB' pointer was utilized before it was verified against nullptr. Check lines: 2001, 2003. api.c 2001
  • V595 The 'AutomationTableB' pointer was utilized before it was verified against nullptr. Check lines: 2347, 2350. api.c 2347
  • V595 The 'IoStack->FileObject' pointer was utilized before it was verified against nullptr. Check lines: 611, 622. device.c 611
  • V595 The 'Ctx' pointer was utilized before it was verified against nullptr. Check lines: 425, 430. event.c 425
  • V595 The 'BusDeviceExtension' pointer was utilized before it was verified against nullptr. Check lines: 1388, 1390. swenum.c 1388
  • V595 The 'path' pointer was utilized before it was verified against nullptr. Check lines: 3250, 3254. catalog.c 3250
  • V595 The 'str2' pointer was utilized before it was verified against nullptr. Check lines: 395, 396. c14n.c 395
  • V595 The 'atts' pointer was utilized before it was verified against nullptr. Check lines: 3763, 3775. htmlparser.c 3763
  • V595 The 'ctxt' pointer was utilized before it was verified against nullptr. Check lines: 3674, 3676. htmlparser.c 3674
  • V595 The 'ctxt->input' pointer was utilized before it was verified against nullptr. Check lines: 6693, 6697. htmlparser.c 6693
  • V595 The 'name' pointer was utilized before it was verified against nullptr. Check lines: 123, 131. hash.c 123
  • V595 The 'ctxt->nsTab' pointer was utilized before it was verified against nullptr. Check lines: 1546, 1553. parser.c 1546
  • V595 The 'ctxt->input' pointer was utilized before it was verified against nullptr. Check lines: 6690, 6698. parser.c 6690
  • V595 The 'ctxt->input' pointer was utilized before it was verified against nullptr. Check lines: 6750, 6758. parser.c 6750
  • V595 The 'atts' pointer was utilized before it was verified against nullptr. Check lines: 8477, 8486. parser.c 8477
  • V595 The 'ctxt->input' pointer was utilized before it was verified against nullptr. Check lines: 11113, 11116. parser.c 11113
  • V595 The 'ctx->myDoc' pointer was utilized before it was verified against nullptr. Check lines: 12784, 12787. parser.c 12784
  • V595 The 'ctxt->myDoc' pointer was utilized before it was verified against nullptr. Check lines: 13341, 13357. parser.c 13341
  • V595 The 'oldctxt' pointer was utilized before it was verified against nullptr. Check lines: 13349, 13367. parser.c 13349
  • V595 The 'tmp' pointer was utilized before it was verified against nullptr. Check lines: 1536, 1537. relaxng.c 1536
  • V595 The 'lib' pointer was utilized before it was verified against nullptr. Check lines: 8598, 8604. relaxng.c 8598
  • V595 The 'ctxt->myDoc' pointer was utilized before it was verified against nullptr. Check lines: 984, 986. sax2.c 984
  • V595 The 'ctxt->incTab' pointer was utilized before it was verified against nullptr. Check lines: 392, 400. xinclude.c 392
  • V595 The 'href' pointer was utilized before it was verified against nullptr. Check lines: 518, 529. xinclude.c 518
  • V595 The 'ctxt' pointer was utilized before it was verified against nullptr. Check lines: 1129, 1130. xinclude.c 1129
  • V595 The 'ctxt->incTab[nr]' pointer was utilized before it was verified against nullptr. Check lines: 1419, 1428. xinclude.c 1419
  • V595 The 'p' pointer was utilized before it was verified against nullptr. Check lines: 586, 589. xmlmemory.c 586
  • V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 2819, 2829. xmlschemastypes.c 2819
  • V595 The 'attr' pointer was utilized before it was verified against nullptr. Check lines: 2858, 2862. xmlschemas.c 2858
  • V595 The 'uses' pointer was utilized before it was verified against nullptr. Check lines: 14498, 14508. xmlschemas.c 14498
  • V595 The 'atom->ranges' pointer was utilized before it was verified against nullptr. Check lines: 817, 818. xmlregexp.c 817
  • V595 The 'exec->state' pointer was utilized before it was verified against nullptr. Check lines: 4263, 4273. xmlregexp.c 4263
  • V595 The 'res' pointer was utilized before it was verified against nullptr. Check lines: 14032, 14054. xpath.c 14032
  • V595 The 'msg' pointer was utilized before it was verified against nullptr. Check lines: 361, 363. rostcp.c 361
  • V595 The 'msg' pointer was utilized before it was verified against nullptr. Check lines: 469, 473. rostcp.c 469
  • V595 The 'last_unsent' pointer was utilized before it was verified against nullptr. Check lines: 632, 661. tcp_out.c 632
  • V595 The 'att->Renderbuffer' pointer was utilized before it was verified against nullptr. Check lines: 790, 798. fbobject.c 790
  • V595 The 'sub_prims' pointer was utilized before it was verified against nullptr. Check lines: 852, 859. st_draw.c 852
  • V595 The 'height' pointer was utilized before it was verified against nullptr. Check lines: 2471, 2474. teximage.c 2471
  • V595 The 'WorkItem' pointer was utilized before it was verified against nullptr. Check lines: 745, 753. notify.c 745
  • V595 The 'lpErrno' pointer was utilized before it was verified against nullptr. Check lines: 425, 439. dllmain.c 425
  • V595 The 'pServiceFailureActions' pointer was utilized before it was verified against nullptr. Check lines: 175, 184. srvpage.c 175
  • V595 The 'This->pITextStoreACP' pointer was utilized before it was verified against nullptr. Check lines: 143, 147. context.c 143
  • V595 The 'sidsize' pointer was utilized before it was verified against nullptr. Check lines: 1998, 2002. registry.c 1998
  • V595 The 'locator->pParserCtxt' pointer was utilized before it was verified against nullptr. Check lines: 2290, 2301. saxreader.c 2290
  • V595 The 'DstBuffer' pointer was utilized before it was verified against nullptr. Check lines: 100, 101. buffer.c 100
  • V595 The 'SrcBuffer' pointer was utilized before it was verified against nullptr. Check lines: 159, 160. buffer.c 159
  • V595 The 'DstBuffer' pointer was utilized before it was verified against nullptr. Check lines: 264, 265. buffer.c 264
  • V595 The 'SrcBuffer' pointer was utilized before it was verified against nullptr. Check lines: 275, 276. buffer.c 275
  • V595 The 'DstBuffer' pointer was utilized before it was verified against nullptr. Check lines: 627, 628. buffer.c 627
  • V595 The 'SrcBuffer' pointer was utilized before it was verified against nullptr. Check lines: 638, 639. buffer.c 638
  • V595 The 'DstBuffer' pointer was utilized before it was verified against nullptr. Check lines: 1249, 1250. buffer.c 1249
  • V595 The 'SrcBuffer' pointer was utilized before it was verified against nullptr. Check lines: 1262, 1263. buffer.c 1262
  • V595 The 'SrcBuffer' pointer was utilized before it was verified against nullptr. Check lines: 1047, 1048. 8390.c 1047
  • V595 The 'DstBuffer' pointer was utilized before it was verified against nullptr. Check lines: 899, 900. main.c 899
  • V595 The 'EnumContext' pointer was utilized before it was verified against nullptr. Check lines: 596, 599. local_group.c 596
  • V595 The 'EnumContext' pointer was utilized before it was verified against nullptr. Check lines: 1344, 1347. user.c 1344
  • V595 The '* ptr' pointer was utilized before it was verified against nullptr. Check lines: 85, 88. nbnamecache.c 85
  • V595 The 'wki' pointer was utilized before it was verified against nullptr. Check lines: 129, 133. netid.c 129
  • V595 The 'wki' pointer was utilized before it was verified against nullptr. Check lines: 163, 167. netid.c 163
  • V595 The 'wki' pointer was utilized before it was verified against nullptr. Check lines: 299, 302. netid.c 299
  • V595 The 'SafeParams' pointer was utilized before it was verified against nullptr. Check lines: 608, 624. harderr.c 608
  • V595 The 'ObjectCreateInfo' pointer was utilized before it was verified against nullptr. Check lines: 707, 732. oblife.c 707
  • V595 The 'ListHead' pointer was utilized before it was verified against nullptr. Check lines: 103, 104. pfnlist.c 103
  • V595 The 'ImpersonationInfo' pointer was utilized before it was verified against nullptr. Check lines: 56, 60. security.c 56
  • V595 The 'CapturedPrivileges' pointer was utilized before it was verified against nullptr. Check lines: 2256, 2277. token.c 2256
  • V595 The 'pv' pointer was utilized before it was verified against nullptr. Check lines: 809, 831. variant.c 809
  • V595 The 'result' pointer was utilized before it was verified against nullptr. Check lines: 3394, 3401. variant.c 3394
  • V595 The 'result' pointer was utilized before it was verified against nullptr. Check lines: 3585, 3592. variant.c 3585
  • V595 The 'pVarOut' pointer was utilized before it was verified against nullptr. Check lines: 5248, 5251. variant.c 5248
  • V595 The 'typeInfo' pointer was utilized before it was verified against nullptr. Check lines: 867, 869. typelib.c 867
  • V595 The 'subtypeinfo' pointer was utilized before it was verified against nullptr. Check lines: 4960, 4965. typelib.c 4960
  • V595 The 'pTLib' pointer was utilized before it was verified against nullptr. Check lines: 7082, 7084. typelib.c 7082
  • V595 The 'DeviceObject' pointer was utilized before it was verified against nullptr. Check lines: 612, 624. fdo.c 612
  • V595 The 'Package' pointer was utilized before it was verified against nullptr. Check lines: 170, 187. init.c 170
  • V595 The 'Package' pointer was utilized before it was verified against nullptr. Check lines: 462, 469. init.c 462
  • V595 The 'Adapter' pointer was utilized before it was verified against nullptr. Check lines: 998, 1004. pcnet.c 998
  • V595 The 'm_pInterruptSync' pointer was utilized before it was verified against nullptr. Check lines: 1610, 1627. miniport_dmus.cpp 1610
  • V595 The 'DSImpl->dsbuffer' pointer was utilized before it was verified against nullptr. Check lines: 882, 898. dsoundrender.c 882
  • V595 The 'pcFetched' pointer was utilized before it was verified against nullptr. Check lines: 199, 204. enummedia.c 199
  • V595 The 'pParser' pointer was utilized before it was verified against nullptr. Check lines: 868, 872. filtermapper.c 868
  • V595 The 'pPropBag' pointer was utilized before it was verified against nullptr. Check lines: 920, 928. filtermapper.c 920
  • V595 The 'pPropBagCat' pointer was utilized before it was verified against nullptr. Check lines: 1342, 1366. filtermapper.c 1342
  • V595 The 'pPropBagCat' pointer was utilized before it was verified against nullptr. Check lines: 784, 797. filtergraph.c 784
  • V595 The 'pFM2' pointer was utilized before it was verified against nullptr. Check lines: 644, 654. regsvr.c 644
  • V595 The 'pAlloc' pointer was utilized before it was verified against nullptr. Check lines: 900, 905. pin.c 900
  • V595 The 'pMemConnected' pointer was utilized before it was verified against nullptr. Check lines: 941, 947. pin.c 941
  • V595 The 'pAlloc' pointer was utilized before it was verified against nullptr. Check lines: 970, 972. pin.c 970
  • V595 The 'pAlloc' pointer was utilized before it was verified against nullptr. Check lines: 999, 1001. pin.c 999
  • V595 The 'pAlloc' pointer was utilized before it was verified against nullptr. Check lines: 1028, 1030. pin.c 1028
  • V595 The 'pMemAlloc' pointer was utilized before it was verified against nullptr. Check lines: 1704, 1709. pin.c 1704
  • V595 The 'This->pMemInputPin' pointer was utilized before it was verified against nullptr. Check lines: 1716, 1725. pin.c 1716
  • V595 The 's' pointer was utilized before it was verified against nullptr. Check lines: 778, 799. recyclebin_v5.c 778
  • V595 The 'prbel' pointer was utilized before it was verified against nullptr. Check lines: 230, 248. recyclebin.c 230
  • V595 The 'ppszNames' pointer was utilized before it was verified against nullptr. Check lines: 238, 245. find.c 238
  • V595 The 'ppszNames' pointer was utilized before it was verified against nullptr. Check lines: 464, 485. find.c 464
  • V595 The 'para' pointer was utilized before it was verified against nullptr. Check lines: 211, 213. caret.c 211
  • V595 The 'lpObject' pointer was utilized before it was verified against nullptr. Check lines: 1151, 1173. editor.c 1151
  • V595 The 'lpDataObject' pointer was utilized before it was verified against nullptr. Check lines: 1152, 1176. editor.c 1152
  • V595 The 'lpOleCache' pointer was utilized before it was verified against nullptr. Check lines: 1150, 1177. editor.c 1150
  • V595 The 'argv' pointer was utilized before it was verified against nullptr. Check lines: 354, 358. rundll32.c 354
  • V595 The 'pServiceStatus' pointer was utilized before it was verified against nullptr. Check lines: 131, 144. query.c 131
  • V595 The 'pServiceStatus' pointer was utilized before it was verified against nullptr. Check lines: 191, 197. query.c 191
  • V595 The 'oleobj' pointer was utilized before it was verified against nullptr. Check lines: 357, 365. dochost.c 357
  • V595 The 'urlfile' pointer was utilized before it was verified against nullptr. Check lines: 220, 235. iexplore.c 220
  • V595 The 'urlobj' pointer was utilized before it was verified against nullptr. Check lines: 223, 238. iexplore.c 223
  • V595 The 'pDrvDefExt' pointer was utilized before it was verified against nullptr. Check lines: 161, 177. drive.cpp 161
  • V595 The 'pszList' pointer was utilized before it was verified against nullptr. Check lines: 589, 606. dialogs.cpp 589
  • V595 The 'pFileDefExt' pointer was utilized before it was verified against nullptr. Check lines: 142, 157. fprop.cpp 142
  • V595 The 'pidl' pointer was utilized before it was verified against nullptr. Check lines: 757, 760. pidl.cpp 757
  • V595 The 'pidlInOut' pointer was utilized before it was verified against nullptr. Check lines: 136, 156. shlfolder.cpp 136
  • V595 The 'cm' pointer was utilized before it was verified against nullptr. Check lines: 1296, 1303. shlexec.cpp 1296
  • V595 The 'pcchOut' pointer was utilized before it was verified against nullptr. Check lines: 2238, 2240. url.c 2238
  • V595 The 'NewSubsystem' pointer was utilized before it was verified against nullptr. Check lines: 500, 502. smsubsys.c 500
  • V595 The 'OutputMdl' pointer was utilized before it was verified against nullptr. Check lines: 1382, 1408. dispatch.c 1382
  • V595 The 'DstBuffer' pointer was utilized before it was verified against nullptr. Check lines: 100, 101. buffer.c 100
  • V595 The 'SrcBuffer' pointer was utilized before it was verified against nullptr. Check lines: 159, 160. buffer.c 159
  • V595 The 'DstBuffer' pointer was utilized before it was verified against nullptr. Check lines: 266, 267. buffer.c 266
  • V595 The 'SrcBuffer' pointer was utilized before it was verified against nullptr. Check lines: 277, 278. buffer.c 277
  • V595 The 'lpszTemp' pointer was utilized before it was verified against nullptr. Check lines: 997, 1000. taskmgr.c 997
  • V595 The 'AtaReq' pointer was utilized before it was verified against nullptr. Check lines: 4944, 4972. id_ata.cpp 4944
  • V595 The 'AtaReq' pointer was utilized before it was verified against nullptr. Check lines: 4899, 4925. id_ata.cpp 4899
  • V595 The 'Srb' pointer was utilized before it was verified against nullptr. Check lines: 9664, 9666. id_ata.cpp 9664
  • V595 The 'Srb' pointer was utilized before it was verified against nullptr. Check lines: 9652, 9666. id_ata.cpp 9652
  • V595 The 'builder->uri' pointer was utilized before it was verified against nullptr. Check lines: 5250, 5262. uri.c 5250
  • V595 The 'pInstance' pointer was utilized before it was verified against nullptr. Check lines: 387, 388. ddeserver.c 387
  • V595 The 'current_line' pointer was utilized before it was verified against nullptr. Check lines: 524, 529. edit.c 524
  • V595 The 'es' pointer was utilized before it was verified against nullptr. Check lines: 5195, 5214. edit.c 5195
  • V595 The 'pBSMInfo' pointer was utilized before it was verified against nullptr. Check lines: 3146, 3158. message.c 3146
  • V595 The 'pHwnd' pointer was utilized before it was verified against nullptr. Check lines: 673, 679. window.c 673
  • V595 The 'levels' pointer was utilized before it was verified against nullptr. Check lines: 1572, 1578. usp10.c 1572
  • V595 The '* root' pointer was utilized before it was verified against nullptr. Check lines: 548, 553. check.c 548
  • V595 The 'VbeInfo' pointer was utilized before it was verified against nullptr. Check lines: 206, 207. vbemodes.c 206
  • V595 The 'pClient->hPins' pointer was utilized before it was verified against nullptr. Check lines: 237, 242. entry.c 237
  • V595 The 'Context.ProcessData' pointer was utilized before it was verified against nullptr. Check lines: 853, 861. exitros.c 853
  • V595 The 'pstrLibName' pointer was utilized before it was verified against nullptr. Check lines: 136, 140. callback.c 136
  • V595 The 'psurfColor' pointer was utilized before it was verified against nullptr. Check lines: 1176, 1182. cursoricon.c 1176
  • V595 The 'DestRect' pointer was utilized before it was verified against nullptr. Check lines: 33, 36. copybits.c 33
  • V595 The 'SourcePoint' pointer was utilized before it was verified against nullptr. Check lines: 34, 36. copybits.c 34
  • V595 The 'pdesk->pDeskInfo->spwnd' pointer was utilized before it was verified against nullptr. Check lines: 148, 150. desktop.c 148
  • V595 The 'psurfPattern' pointer was utilized before it was verified against nullptr. Check lines: 342, 365. engbrush.c 342
  • V595 The 'WndPrev' pointer was utilized before it was verified against nullptr. Check lines: 62, 65. focus.c 62
  • V595 The 'Wnd' pointer was utilized before it was verified against nullptr. Check lines: 374, 384. focus.c 374
  • V595 The 'pti->rpdesk' pointer was utilized before it was verified against nullptr. Check lines: 358, 364. input.c 358
  • V595 The 'pStrokes' pointer was utilized before it was verified against nullptr. Check lines: 1941, 1947. path.c 1941
  • V595 The 'dc' pointer was utilized before it was verified against nullptr. Check lines: 933, 958. palette.c 933
  • V595 The 'SourceRect' pointer was utilized before it was verified against nullptr. Check lines: 402, 430. stretchblt.c 402
  • V595 The 'Wnd' pointer was utilized before it was verified against nullptr. Check lines: 390, 403. windc.c 390
  • V595 The 'pPatterns' pointer was utilized before it was verified against nullptr. Check lines: 512, 533. info.c 512
  • V595 The 'converter' pointer was utilized before it was verified against nullptr. Check lines: 2164, 2172. info.c 2164
  • V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 95, 100. texture.c 95
  • V595 The 'Globals.win_list' pointer was utilized before it was verified against nullptr. Check lines: 560, 583. winhelp.c 560
  • V595 The 'lpwh' pointer was utilized before it was verified against nullptr. Check lines: 1400, 1414. ftp.c 1400
  • V595 The 'optval' pointer was utilized before it was verified against nullptr. Check lines: 284, 296. sockctrl.c 284

Макросы

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

В 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;". Остальные строки кода будут выполнены всегда!

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

  • 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

Условия, которые всегда истинны или ложны

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

#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.

Аналогичные ошибки при работе с сокетами находятся здесь:

  • 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

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

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 не интересно. Приведу список оставшихся мест, на которые я обратил внимание:

  • 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[i]) < (0)' is always false. Unsigned type value is never < 0. texstore.c 3692
  • V547 Expression '(src[i]) < (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

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

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

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

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

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

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

Аналогичные ошибки можно найти здесь:

  • 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

Приоритеты операций

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

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])

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

  • 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

Разное

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

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

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

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

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

Аналогичные странные циклы:

  • V612 An unconditional 'break' within a loop. regexp.c 3633
  • V612 An unconditional 'break' within a loop. hlpfile.c 1131

Странное

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

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 в борьбе с багами. Там мы также публикуем ссылки на интересные статьи по тематике программирования на языке Си/Си++ и смежным темам.



Найдите ошибки в своем C, C++, C# и Java коде

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

goto PVS-Studio;

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


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

Проверено проектов
354
Собрано ошибок
13 290

А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++, C#
и Java

goto PVS-Studio;