Eleven moments of ReactOS: user mode gets better?

ReactOS is a project where the victory over regression and the delivery of a new feature (or its working prototype) are loudly celebrated. Even the FOSS community has to take a break from rewriting everything in Rust and pause polemics about systemd. We've already checked ReactOS in 2013, almost eleven years ago. The analysis was inaccurate because we didn't fully understand the project folder structure, and it played a cruel joke: PVS-Studio analyzed the Wine fragments too. It's time to refresh our memory and check this project again, but now we'll consider the experience of the previous flaws.

Introduction

Let us remind you that ReactOS doesn't use the GNU/Linux kernel "under the hood". It has the self-written kernel that behaves like the 32-bit Windows Server 2003 R2 and the 64-bit Windows Vista. Developers used the Wine components as a compatibility layer capable of running Windows applications on several POSIX-compliant operating systems to implement the Windows user environment and run Windows applications, but it does not mean that the OS will use the GNU/Linux kernel. When you install WineVDM on Windows 11, the kernel of your OS won't be replaced by the kernel of, let's say, Debian 12, will it? :)

It's also important to note that the behavior of the target Windows kernel is reproduced only via observing functions and disassembling. For the record, the use of the leaked Windows resources is forbidden.

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.

"Black-box" reverse engineering is an arduous process for such a project, and it's impossible to do it without mistakes. The comment section experts of the various FOSS communities and news aggregators are annoyed that the ReactOS developers consciously refuse to "taste the forbidden fruit". The commentators allude to the fact that the OS has been in alpha for 26 years, winking and nodding that "maybe it's time to do something". Let's save this for other discussion and get started on setting up PVS-Studio to check the code.

PVS-Studio installation

You can download PVS-Studio here. To analyze the project, you will need a license. Here you can get a trial version. The PVS-Studio has a user-friendly installation interface: each step is explained, and if you have any difficulties, the guide for the quick start on Windows is always at your service. The project to analyze is not typical, so we'll use the "C and C++ Compiler Monitoring" tool and integration with any IDE. We'll use the extension for Visual Studio 2022. The current version of the analyzer at the moment of writing the article is 7.30.

Build configuration and running its analysis

Like Unreal Engine, React OS requires a non-standard approach to analysis. The ReactOS Build Environment (RosBE) is a complete toolkit for creating an installation or live media of an OS. It includes special versions of the CMake, Bison, Flex, and Ninja *utilities. We'll analyze the standard debug build via *GCC. The code matches the 00c4b3d commit in the master branch. Let's configure the workspace.

We will analyze the project using CLMonitor. *What is it? This is the PVS-Studio console utility that allows us to analyze projects regardless of their build system. The main requirement is that the compiler should be supported by the analyzer. As *GCC is used for compilation, each compiler call will be detected and logged.

Run RosBE, navigate to the source folder from the pulled repository, and run the configure.cmd script. Once the configuration is complete, run CLMonitor (the path is set by default) in the monitoring mode:

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

Run a build, for example, bootcd (only the installation media):

cd output-MinGW-i386
ninja bootcd

Once the build is completed, close CLMonitor with saving the result:

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

Now we can start checking the project. Run Visual Studio, select the Continue without code option in the welcome window. Next, open the log file saved by CLMonitor. We saved it to the same folder where the RosBE output files are located.

To open the saved log file, select Extensions > PVS-Studio > Open/Save > Open Analysis Report.

Analysis of detected errors

"First things first" is the welcome message when we start Microsoft Office 2016 for the first time, prompting us to perform the initial setup. The first start of PVS-Studio is equally important in any project where it has never been used before. We need to remove third-party components from the list of checked files as much as possible. These are auxiliary utilities for creating ISO images, the RosBE public folder, and the Public SDK files. Such preparation is essential because these components overlap with the Windows SDK headers, which leads to numerous warnings about overriding basic types or annotation mismatches. We can do it in the PVS-Studio plugin settings in the Don't Check Files section.

For your convenience, there's a list of the excluded folders from the ReactOS codebase in the form of text and relative paths. Note that the path to the share folder from RosBE should be specified in absolute format.

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

Now it's time to look for safety glasses, because ReactOS is known for its ability to "explode" from any oblique glance. Sometimes, even its developers joke about it :)

Our version of Alexander Rechitskii's (Jedi-to-be) meme

As the eleventh anniversary of the last ReactOS check is coming up, I bring to your attention eleven curious moments of the project codebase (and one special surprise among them, don't miss it)! We'll gradually move from classic copy-pastes to truly "catastrophic failures".

Windows does the same thing! But is it exactly the same?

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;
  }
  ....
}

Let's believe that Windows really returns TRUE when the types of the source and destination GDI device contexts match. However, for some reason, DCSrc isn't compared, and that's why the surface blending doesn't occur! The diagnostic rule reports that the condition contains two identical sub-expressions separated by the OR operator.

Security certificate please!

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;
  }
  ....
}

The parentheses are missing; the condition will be executed only if the flags variable is equal to 0. In other cases, the negation operator '!' will return false. The bitwise AND operator plays no role here: the condition is always false, and all connections are deemed as safe.

Frown The Pointers

Let's continue the topic of authentication and take a look at the system FTP client. Server authorization and proxy switch don't leave me cold to the particularly cruel abuse of memory in the strings. Where will the terminal null be placed? Where does the string end? What data will be corrupted? It's time to "vote off" the access violation error!

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;
  ....
}

We're interested in the following line:

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

A developer wants to write the terminal null ('\0') at the end of the string. The funny thing is that the strlen function is used to find where to write '\0'. The function finds the end of the string by searching for the nearest terminal null. It turns out that to mark the end of the string, we should first find the end of the string :)

The terminal null will be written to the same place where it already is. A dangerous issue arises because this can happen out of the buffer. Technically, this causes undefined behavior, but in real cases, it can lead to an Access Violation error.

Here, the issue isn't so serious. Note that the tmpstruct object is static. This means that all its fields will be empty-initialized, including the name buffer, which will be filled with zeroes.

A string is copied into the buffer with the reserved terminal null. So, even if the source string is very long and the strncpy function doesn't write '\0' at the end, it still will be there.

Let's go back to the string:

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

There is no array overrun here, but the operation is meaningless. The '\0' character is written where it already is. We can freely delete the line.

I hope I've managed to shed some light on this curious code anomaly.

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;
  }
  ....
}

Here's a more serious case of the error detected by the V692 diagnostic rule. We've already talked about how we can screw our evening up with the fgets function in the article "Shoot yourself in the foot when handling input data". I recommend reading it if you're interested in where the catch is.

No one saw anything!

After what I had seen in the FTP client, I felt an imminent need for something to calm me down. The way the Windows Installer module was implemented here astonishes me with the useless correction of the lpValue buffer length. Here, the function uses the pcchValue variable to specify the length of the requested value:

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;
  }
  ....
}

If we see the Windows Installer, there we'll see the CAB archives because files are packed into these archives to save space. A dev may have wanted to return CAB_STATUS_NOMEMORY, but they overlooked that it doesn't reach the return function because the status is immediately replaced with "the invalid CAB file". As a result, an application will handle the error incorrectly.

Your minus is nothing

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;
   ....
}

Now it's time to dive deeper into the user environment. COM and ActiveX. You shudder at hearing about these entities, don't you? There is something very strange going on here with handling the hyperlinks when we set their targets. The second half of the check makes me ask a developer some embarrassing questions.

(Unsure sounds)

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;
  ....
}

The ReactOS sound subsystem is also the Achilles' heel. There is no resampling, no channels volume synchronization. However, the code fragment is connected with the sound subsystem indirectly: it's the ADPCM codec.

The essence of the error is that the sample bitness has already been checked earlier. If the sample bitness differs from 16 bits, the control is transferred to the theEnd label. The following bitness checks will always be TRUE.

Mutually exclusive paragraphs

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;
  }
  ....
}

The code will return the "The parameter is incorrect" error in any case because the GUID of the installed device hasn't been defined, and its properties can't be obtained. Before we move away from working with installed devices, let's take a look at the developer-hated UniATA driver. Yes, it's a third-party component that we shouldn't check. However, it's very hard to resist and pass by the logical trap and an example of how not to design the code... I can't just let it go.

We called DMA and hang up

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);
    }
  }
  ....
}

If we see that checks are different in the conditions, we can suggest that something is wrong here. Either the line break is missed, or DMA should be reinitialized if the first check returned FALSE (the option suggested by the diagnostic rule), or if the case requires switching to PIO (program I/O). The code "smells" a little suspicious, don't you think? Is this smell a harbinger of great misfortune or not? It's undefined.

Cast you cannot shift (add missing comma)

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)) // <=
     );
  }
  ....
  }
}

Before the C++20 standard, the shift of negative number was undefined behavior. The issue is that the UniATA driver code is written in C but compiles as C++ code, and developers haven't explicitly specified the C++20 standard. As a result, it leads to undefined behavior. We may read the coffee grounds and try to fortune-tell what the driver author expected to get: an intentional undefined behavior or a cast of the inverted 0x0D to a UCHAR that would look like (UCHAR)~0x0D? Nothing is clear, but it's very interesting. Maybe I should find a motherboard with an NVIDIA SATA controller and spend some time with a debugger...

That eleventh moment of ReactOS

Nevertheless, there is a bright spark in the dark: ReactOS has a component where PVS-Studio hasn't detected any errors. It's a very important place – the bootloader. Thanks to Justin Miller aka DarkFire01, ReactOS now supports booting via UEFI, and this FreeLoader component is clean according the analyzer. It definitely merits an award: two years ago it was an experiment, a year later devs tried to run it on other devices, and now it's in the main codebase. And we're placing it at the Eleventh moment of ReactOS.

Soon, developers will introduce multiprocessor support that has been in work much longer than UEFI. We really hope that multiprocessing will be as gracefully implemented as the updated bootloader, because parallel data processing at the kernel level doesn't forgive mistakes :)

Conclusion

Despite the non-standard build system, PVS-Studio checked the entire OS codebase and successfully completed the static analyzer mission. ReactOS made progress in enhancing code quality, but issues still exist. I'm glad that maintainers cope with the challenges and break the mold of the "experts" and their endless hate waves. But it's frustrating that developers don't explain why they may not implement some missing features.

I hope the ReactOS anniversary recheck helped you gain new insights into working with low-level code or just refreshed your memory. Traditionally, I can't help but suggest you try to check your project with PVS-Studio. The process isn't difficult, and the useful result won't make you wait! If your project is open-source, you can get a license for the OSS projects.

Comments

[a.infosecflavour]

Very nice documented!