11 мгновений ReactOS: user mode становится лучше?

• Введение
• Установка PVS-Studio
• Конфигурация сборки и запуск её анализа
• Анализ найденных ошибок
  • Да, Windows делает так же, но точно ли так же?
  • Ваш сертификат безопасности, пожалуйста.
  • Frown The Pointers
  • Никто ничего не видел!
  • Ваш минус ничего не значит
  • Звук неуверенности
  • Взаимоисключающие параграфы
  • В DMA позвонили и сбросили
  • Кастовать нельзя продвинуть
  • То самое, одиннадцатое мгновение ReactOS
• Заключение

ReactOS — это проект, где победу над регрессией, появление новой фичи или её рабочего прототипа празднуют так громко, что FOSS-сообществу приходится отвлекаться от переписывания всего на Rust и полемик о systemd. В последний раз мы проверяли ReactOS в 2013 году, почти одиннадцать лет назад. Проверка была неточной ввиду неполного понимания структуры папок, из-за чего в поле видимости PVS-Studio оставались компоненты Wine. Пришло время освежить память и провести новую проверку, учитывая опыт предыдущей недоработки.

Введение

Напомним, что ReactOS "под капотом" использует не ядро GNU/Linux, а самостоятельно написанное ядро, воспроизводящее поведение Windows Server 2003 R2 в 32-битной редакции и Windows Vista в 64-битной. Использование компонентов Wine, прослойки совместимости Windows API для POSIX-совместимых операционных систем, для реализации пользовательской среды Windows и возможности запуска приложений для Windows совершенно не означает, что операционная система будет использовать ядро GNU/Linux. При установке WineVDM на Windows 11 ядро вашей копии системы не заменится на ядро условного Debian 12, верно? :)

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

Legal notice: If you have seen proprietary Microsoft Windows source code (including but not limited to the leaked Windows NT 3.5, NT 4, 2000 source code and the Windows Research Kernel), your contribution won't be accepted because of potential copyright violation.

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

Установка PVS-Studio

Загрузить PVS-Studio можно перейдя на эту страницу. Чтобы произвести анализ проекта, понадобится лицензия, триальную версию которой можно получить здесь. Процедура установки PVS-Studio не вызовет сложностей: каждый шаг сопровождается легко воспринимаемым пояснением, а при возникновении трудностей всегда поможет справка по быстрому запуску под Windows. Потребуется два компонента: мониторинг компиляторов (C and C++ Compiler Monitoring) и любая интеграция с IDE. Мы будем использовать расширение для Visual Studio 2022. Актуальная на момент написания статьи версия анализатора — 7.30.

Конфигурация сборки и запуск её анализа

ReactOS — это ещё один пример проекта с нестандартной процедурой анализа, наряду с проектами Unreal Engine. ReactOS Build Environment (RosBE) является полноценным набором инструментов для создания установочного или Live-носителя операционной системы и содержит специальные версии утилит CMake, Bison, Flex и Ninja. Анализу будет подвергнута стандартная отладочная сборка, выполненная через GCC с кодом, актуальным на момент коммита 00c4b3d ветки master. Произведём настройку рабочего окружения и рассмотрим сам процесс.

Анализ проекта будет осуществляться через CLMonitor. Что он собой представляет? Это консольная утилита в составе PVS-Studio, которая позволяет производить анализ проектов независимо от их системы сборки. Главное условие — компилятор должен быть в перечне поддерживаемых. Поскольку за компиляцию отвечает GCC, то каждый вызов компилятора будет обнаружен и записан в журнал.

Запускаем RosBE, переходим в папку с исходными текстами из извлечённого репозитория и выполняем сценарий configure.cmd. По окончании конфигурации запускаем CLMonitor (указан путь по умолчанию) в режиме мониторинга:

"C:\Program Files (x86)\PVS-Studio\CLMonitor" monitor

Запускаем сборку любой цели, например, bootcd (только установочный носитель):

cd output-MinGW-i386
ninja bootcd

После завершения сборки закрываем CLMonitor c сохранением результатов:

"C:\Program Files (x86)\PVS-Studio\CLMonitor" analyze -l output.plog

Теперь можно приступить к анализу. Запускаем Visual Studio, в окне приветствия выбираем вариант Continue without code. Далее открываем сохранённый CLMonitor файл журнала. Мы его сохранили в ту же папку, где находятся выходные файлы RosBE.

Чтобы открыть сохранённый файл журнала, выберите пункт Extensions > PVS-Studio > Open/Save > Open Analysis Report.

Анализ найденных ошибок

"В первую очередь — самое важное" — такой фразой приветствует Microsoft Office 2016 при первом запуске, предлагая произвести начальную настройку. Не менее важен первый запуск PVS-Studio в любом проекте, где его ни разу не использовали, так как нам необходимо по максимуму убрать из перечня проверяемых файлов сторонние компоненты: вспомогательные утилиты для генерации ISO-образа, общую папку RosBE и файлы Public SDK. Они пересекаются определениями с заголовками Windows SDK, в результате чего возникает огромное количество предупреждений о переопределении базовых типов или несовпадении аннотаций. Производится эта операция в настройках плагина PVS-Studio в разделе Don't Check Files.

Для вашего удобства: список исключённых папок из кодовой базы ReactOS в форме текста и относительных путей. Путь до папки share из RosBE потребуется указать в абсолютном формате.

dll\directx\wine\
dll\win32\advapi32\wine\
dll\win32\dbghelp\
drivers\filesystems\btrfs\
drivers\filesystems\udfs\
sdk\include\psdk\
sdk\lib\3rdparty\
sdk\tools\

Теперь самое время поискать защитные очки, ведь ReactOS известна своим умением "взорваться" от косого взгляда, над чем иногда шутят и сами разработчики :)

По случаю приближающейся 11-й годовщины с момента последней проверки ReactOS, представляю вашему вниманию 11 интересных находок в кодовой базе проекта (среди них — один сюрприз)! Будем постепенно разгоняться, начиная с классических копипаст и заканчивая по-настоящему "разрушительными сбоями".

Да, Windows делает так же, но точно ли так же?

V501 There are identical sub-expressions 'DCDest->dctype == DCTYPE_INFO' to the left and to the right of the '||' operator. bitblt.c 64

BOOL APIENTRY
NtGdiAlphaBlend(
  HDC hDCDest,
  LONG XOriginDest,
  LONG YOriginDest,
  LONG WidthDest,
  LONG HeightDest,
  HDC hDCSrc,
  LONG XOriginSrc,
  LONG YOriginSrc,
  LONG WidthSrc,
  LONG HeightSrc,
  BLENDFUNCTION BlendFunc,
  HANDLE hcmXform)
{
  PDC DCDest;
  PDC DCSrc;
  ....
  if (DCDest->dctype == DCTYPE_INFO || DCDest->dctype == DCTYPE_INFO)  // <=
  {
    GDIOBJ_vUnlockObject(&DCSrc->BaseObject);
    GDIOBJ_vUnlockObject(&DCDest->BaseObject);
    /* Yes, Windows really returns TRUE in this case */
    return TRUE;
  }
  ....
}

Поверим, что Windows действительно возвращает TRUE при совпадении типов исходного и конечного контекстов устройств GDI, но DCSrc почему-то в сравнении не участвует, из-за чего смешивание поверхностей не происходит! Об этом и сообщает диагностика: в условии стоят два одинаковых подвыражения, разделённые оператором ИЛИ.

Ваш сертификат безопасности, пожалуйста.

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. loaddlg.cpp 376

#define SECURITY_FLAG_SECURE                 0x00000001

static BOOL
CertGetSubjectAndIssuer(HINTERNET hFile,
                        CLocalPtr<char> &subjectInfo,
                        CLocalPtr<char> &issuerInfo)
{
  ....
  DWORD size, flags;

  size = sizeof(flags);
  if (!InternetQueryOptionA(hFile,
                            INTERNET_OPTION_SECURITY_FLAGS,
                            &flags,
                            &size))
  {
    return FALSE;
  }

  if (!flags & SECURITY_FLAG_SECURE)  // <=
  {
    return FALSE;
  }
  ....
}

Потерялись скобки: условие выполнится только в том случае, если переменная flags равна 0. Во всех остальных случаях оператор отрицания '!' вернёт значение false. Побитовая операция И в этом случае уже не играет никакой роли: условие всегда ложно и все соединения рассматриваются как защищённые.

Frown The Pointers

Продолжая тему проверки подлинности, под руку попал системный FTP-клиент. Авторизация на сервере и переключение прокси не оставили меня равнодушным к особо жестокому обращению с памятью в строках. Куда будет поставлен терминальный ноль? Где закончится строка? Чьи данные будут повреждены? Пришло время вызвать Access Violation!

V692 An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. ftp.c 1355

#define MAXHOSTNAMELEN 64
char* hostname;

void pswitch(int flag)
{
  ....
  static struct comvars {
    int connect;
    char name[MAXHOSTNAMELEN];
    ....
  } proxstruct, tmpstruct;
  struct comvars *ip, *op;
  ....
  if (flag) {
    if (proxy)
      return;
    ip = &tmpstruct;
    op = &proxstruct;
    proxy++;
  }
  ....
  if (hostname) {
    (void) strncpy(ip->name, hostname, sizeof(ip->name) - 1);
    ip->name[strlen(ip->name)] = '\0';
  } else
    ip->name[0] = 0;
  ....
}

Нас интересует следующая строка:

ip->name[strlen(ip->name)] = '\0';

В ней программист хочет записать терминальный ноль ('\0') в конец строки. Юмор ситуации в том, что для определения позиции для записи '\0' используется функция strlen. Функция определяет конец строки, осуществляя поиск ближайшего терминального нуля. Получается, чтобы записать конец строки, надо вначале найти конец строки :)

Терминальный ноль будет записан в то место, где он и так уже находится. Возникает опасная ситуация, так как это может произойти за пределами буфера. Формально это приводит к неопределённому поведению, а на практике может вызвать Access Violation.

В разбираемом случае ситуация не такая страшная. Обратите внимание, что объект tmpstruct является статическим. Это значит, что все его поля будут empty-инициализированы. В том числе и буфер name, который будет заполнен нулями.

В буфер копируется строка, с учётом необходимости добавить терминальный ноль. Поэтому, даже если строка-источник очень длинная и функция strncpy не запишет в конце '\0', он там уже будет.

Вернёмся к строке:

ip->name[strlen(ip->name)] = '\0';

Выхода за границу массива здесь нет, но сама операция бессмысленна. '\0' записывается туда, где он и так уже находится. Строчку можно смело удалить.

Надеюсь, у меня получилось объяснить эту интересную аномалию кода.

V1010 Unchecked tainted data is used in index: 'strlen(tmp)'. ftp.c 216

int login(const char *host)
{
  char tmp[80];
  ....
  while (user == NULL) {
    const char *myname = "none"; // This needs to become the username env
    
    if (myname)
      printf("Name (%s:%s): ", host, myname);
    else
      printf("Name (%s): ", host);
    (void) fflush(stdout);
    (void) fgets(tmp, sizeof(tmp) - 1, stdin);
    tmp[strlen(tmp) - 1] = '\0';                 // <=
    if (*tmp == '\0')
      user = myname;
    else
      user = tmp;
  }
  ....
}

Ещё более страшная вариация ошибки, найденной диагностикой V692. Про то, как можно испортить себе вечер функцией fgets, мы ранее рассказывали в статье "Стреляем в ногу, обрабатывая входные данные". Рекомендую ознакомиться, если интересно узнать, где кроется подвох.

Никто ничего не видел!

После увиденного в клиенте FTP срочно требовалось что-то для успокоения. Местная реализация модуля Windows Installer радует совершенно бесполезной операцией уточнения длины буфера lpValue (функция использует переменную pcchValue для обозначения длины запрошенного значения):

V519 The '* pcchValue' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1779, 1782. msi.c 1782

UINT WINAPI MsiGetPatchInfoExW(LPCWSTR szPatchCode, LPCWSTR szProductCode,
                               LPCWSTR szUserSid, MSIINSTALLCONTEXT dwContext,
                               LPCWSTR szProperty, LPWSTR lpValue,
                               DWORD *pcchValue)
{
  ....
  if ((*val && *pcchValue < len + 1) || !lpValue)
  {
    if (lpValue)
      r = ERROR_MORE_DATA;
    *pcchValue = len * sizeof(WCHAR);
  }
  *pcchValue = len;
  ....
}

V519 The 'Status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1254, 1255. cabinet.c 1255

ULONG
CabinetExtractFile(IN PCABINET_CONTEXT CabinetContext,
                   IN PCAB_SEARCH Search)
{
  ....
  if (Status != CS_SUCCESS)
  {
    DPRINT("Cannot uncompress block\n");
    if (Status == CS_NOMEMORY)
      Status = CAB_STATUS_NOMEMORY;
    Status = CAB_STATUS_INVALID_CAB;
    goto UnmapDestFile;
  }
  ....
}

Где установщик Windows Installer, там и CAB-архивы, так как в последние, как правило, упаковываются файлы для экономии места. Вероятно, автор хотел вернуть CAB_STATUS_NOMEMORY, но не обратил внимание, что статус не доходит до возврата функцией, поскольку сразу заменяется на "некорректный CAB-файл", что приведёт к неправильной обработке ошибки приложением.

Ваш минус ничего не значит

V605 Consider verifying the expression. An unsigned value is compared to the number -3. link.c 267

typedef enum {
  HLINKSETF_TARGET   = 0x00000001,
  HLINKSETF_LOCATION = 0x00000002
} HLINKSETF;

typedef unsigned long       DWORD;

static HRESULT WINAPI IHlink_fnSetStringReference(
  IHlink* iface,
  DWORD grfHLSETF,
  LPCWSTR pwzTarget,
  LPCWSTR pwzLocation)
{
  HlinkImpl  *This = impl_from_IHlink(iface);

  TRACE("(%p)->(%i %s %s)\n", This, grfHLSETF, debugstr_w(pwzTarget),
            debugstr_w(pwzLocation));

  if(grfHLSETF > (HLINKSETF_TARGET | HLINKSETF_LOCATION) &&
     grfHLSETF < -(HLINKSETF_TARGET | HLINKSETF_LOCATION))  // <=
       return grfHLSETF;
   ....
}

Теперь пора спуститься ещё глубже в пользовательской области. COM, ActiveX — не бросает ли вас в дрожь при упоминании этих сущностей? Здесь происходит что-то очень странное с обработкой гиперссылок при установке их целей. Вторая половина проверки вызывает неудобные вопросы.

Звук неуверенности

V560 A part of conditional expression is always true: adsi->pwfxSrc->wBitsPerSample == 16. imaadp32.c 794

V560 A part of conditional expression is always true: adsi->pwfxSrc->wBitsPerSample == 16. imaadp32.c 796

static LRESULT ADPCM_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
{
  ....
  if (adsi->pwfxSrc->nSamplesPerSec != adsi->pwfxDst->nSamplesPerSec ||
      adsi->pwfxSrc->nChannels != adsi->pwfxDst->nChannels ||
      adsi->pwfxSrc->wBitsPerSample != 16)         // <=
        goto theEnd;

  nspb = ((LPIMAADPCMWAVEFORMAT)adsi->pwfxDst)->wSamplesPerBlock;
  TRACE("spb=%u\n", nspb);

  /* we check that in a block, after the header, samples are present on
   * 4-sample packet pattern
   * we also check that the block alignment is bigger than
   * the expected size
   */
  if (((nspb - 1) & 3) != 0) goto theEnd;
  if ((((nspb - 1) / 2) + 4) * adsi->pwfxDst->nChannels
    < adsi->pwfxDst->nBlockAlign
  ) goto theEnd;

  /* adpcm coding... */
  if (adsi->pwfxSrc->wBitsPerSample == 16 // <=
   && adsi->pwfxSrc->nChannels == 2)
     aad->convert = cvtSS16imaK;
  if (adsi->pwfxSrc->wBitsPerSample == 16 // <=
   && adsi->pwfxSrc->nChannels == 1)
     aad->convert = cvtMM16imaK;
  ....
}

Звуковая подсистема ReactOS тоже больное место. Передискретизации (resampling) нет, громкость каналов не синхронизирована... Но представленный образец кода со звуковой подсистемой связан косвенно: это кодек ADPCM.

Суть ошибки: уже ранее была выполнена проверка разрядности сэмплов. Если разрядность сэмплов отличается от 16 бит, то происходит переход к метке theEnd. Последующие проверки разрядности всегда будут истинны.

Взаимоисключающие параграфы

V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 2389, 2392. devinst.c 2389

HDEVINFO WINAPI SetupDiGetClassDevsExW(
  CONST GUID *class,
  PCWSTR enumstr,
  HWND parent,
  DWORD flags,
  HDEVINFO deviceset,
  PCWSTR machine,
  PVOID reserved)
{
  if (flags & DIGCF_ALLCLASSES)
  {
    /* The caller wants all classes. Check if
     * the deviceset limits us to one class */
     ....
  }
  else if (class)
  {
    /* The caller wants one class. Check if it matches deviceset class */
    ....
  }
  else if (!IsEqualIID(&list->ClassGuid, &GUID_NULL))            // <=
  {
    /* No class specified. Try to use the one of the deviceset */
    if (IsEqualIID(&list->ClassGuid, &GUID_NULL))                // <=
      pClassGuid = &list->ClassGuid;
    else
    {
      SetLastError(ERROR_INVALID_PARAMETER);
      goto cleanup;
    }
  }
  else
  {
    SetLastError(ERROR_INVALID_PARAMETER);
    goto cleanup;
  }
  ....
}

Код в любом случае вернёт ошибку "Параметр задан неверно", так как GUID установленного устройства не был определён, и его свойства получить нельзя. Пока мы не отошли от работы с установленными устройствами, взглянем на ненавидимый разработчиками драйвер UniATA. Да, это сторонний компонент, его надо было из проверки исключить наряду с другими сторонними драйверами, но удержаться и пройти мимо логической путаницы и примера того, как не надо оформлять код... Нет, такое нельзя упускать.

В DMA позвонили и сбросили

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. id_ata.cpp 7820

ULONG
NTAPI
AtapiSendCommand(IN PVOID HwDeviceExtension, IN PSCSI_REQUEST_BLOCK Srb,
                 IN ULONG CmdAction)
{
  ....
  if((Srb->Cdb[0] == SCSIOP_REQUEST_SENSE)
  && !(deviceExtension->HwFlags & UNIATA_SATA)) {
    KdPrint2((
    PRINT_PREFIX "AtapiSendCommand: SCSIOP_REQUEST_SENSE -> no dma setup (2)\n"
    ));
    ....
    AtapiDmaReinit(deviceExtension, LunExt, AtaReq);
  } if(AtaReq->TransferLength) {    // <=
    if(!dma_reinited) {
      KdPrint2((PRINT_PREFIX "AtapiSendCommand: AtapiDmaReinit()\n"));
      AtapiDmaReinit(deviceExtension, LunExt, AtaReq);
    ....
    }
  } else {
    KdPrint2((PRINT_PREFIX "AtapiSendCommand: zero transfer\n"));
    ....
    if(!deviceExtension->opt_AtapiDmaZeroTransfer
    && !(deviceExtension->HwFlags & UNIATA_SATA)) {
      KdPrint2((PRINT_PREFIX "AtapiSendCommand: AtapiDmaReinit() to PIO\n"));
      AtapiDmaReinit(deviceExtension, LunExt, AtaReq);
    }
  }
  ....
}

Исходя из того, что в условиях проводятся разные проверки, здесь либо не хватает переноса строки, либо повторная инициализация DMA должна происходить в случае, если первая проверка вернула FALSE (предлагаемый диагностикой вариант), или если ситуация вынуждает перейти в PIO (программный ввод/вывод). Слишком подозрительно, слишком резкий "запах" от кода исходит. Был ли этот запах предвестником большой беды? Неопределённо.

Кастовать нельзя продвинуть

V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~(UCHAR) 0x0d)' is negative. id_ata.cpp 2705

BOOLEAN
NTAPI
AtapiResetController(IN PVOID HwDeviceExtension, IN ULONG PathId,
                     IN BOOLEAN CompleteType)
{
  ....
  case ATA_NVIDIA_ID: {
  ULONG offs;
  ULONG Channel = deviceExtension->Channel + j;
  ....
  if(ChipFlags & NVQ) {
    KdPrint2((PRINT_PREFIX "  NVQ, 32bits reg\n"));
    AtapiWritePortEx4(NULL,
      (ULONGIO_PTR)(&deviceExtension->BaseIoAddressSATA_0), offs + 4,
      AtapiReadPortEx4(NULL,
         (ULONGIO_PTR)(&deviceExtension->BaseIoAddressSATA_0), offs + 4)
       & ((~(ULONG)0x0000000d) << (!Channel * 16))
    );
  } else {
    AtapiWritePortEx1(NULL,
      (ULONGIO_PTR)(&deviceExtension->BaseIoAddressSATA_0), offs + 1,
      AtapiReadPortEx1(NULL,
         (ULONGIO_PTR)(&deviceExtension->BaseIoAddressSATA_0), offs + 1)
       & ((~(UCHAR)0x0d) << (!Channel * 4)) // <=
     );
  }
  ....
  }
}

До стандарта C++20 сдвиг отрицательного числа считался неопределённым поведением. Проблема в том, что код драйвера UniATA на деле является кодом на C, компилируемым как код C++, и явно без применения спецификаций C++20, поэтому это гарантированно неопределённое поведение. Можно поиграть в экстрасенса и попытаться угадать, что ожидал получить автор драйвера: намеренное воспроизведение неопределённого поведения или же каст инвертированного 0x0D до UCHAR, который выглядел бы как (UCHAR)~0x0D? Ничего не понятно, но очень интересно. Нужно найти материнскую плату с SATA-контроллером NVIDIA и посидеть с отладчиком...

То самое, одиннадцатое мгновение ReactOS

Тем не менее есть лучик света в тёмном царстве: в кодовой базе ReactOS присутствует один компонент, в котором PVS-Studio не выявил проблем. Он находится в зоне высокой ответственности — в загрузчике. Благодаря Джастину Миллеру (DarkFire01) в ReactOS появилась поддержка загрузки через UEFI, и именно этот компонент FreeLoader чист с точки зрения анализатора. Это определённо заслуживает награды: два года назад это было экспериментом, через год пошли попытки запуска на других устройствах, и сейчас это в основной кодовой базе. И мы это ставим 11-м мгновением ReactOS.

На подходе поддержка многопроцессорности, над которой бились куда дольше, чем над UEFI. Очень надеемся, что многопроцессорность будет так же изящно реализована, как и обновлённый загрузчик, потому что параллельная обработка данных на уровне ядра не прощает ошибок :)

Заключение

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

Надеюсь, что юбилейный ре-обзор ReactOS помог вам получить новые знания в области работы с низкоуровневым кодом или освежить память. Хотите попробовать проверить свой проект с помощью PVS-Studio? Процесс несложный, а полезный результат не заставит себя ждать! А если ваш проект с открытым исходным кодом, то у вас есть возможность получить лицензию для OSS-проектов.