Re-checking the ReactOS project - a large report

The ReactOS project is rapidly developing. One of the developers participating in this project suggested that we re-analyzed the source code, as the code base is growing fast. We were glad to do that. We like this project, and we'll be happy if this article helps the developers to eliminate some bugs. Analysis was performed with the PVS-Studio 5.02 code analyzer.

Let me remind you what ReactOS is. This is a free and open-source operating system based on the Windows NT architecture principles. The system was developed from scratch and therefore is not based on Linux and doesn't have anything in common with the UNIX architecture. The main aim of the ReactOS project is to create a Windows binary-compatible operating system that would allow users to execute Windows-compatible applications and drivers as if they were executed in Windows itself.

We analyzed this project once some time ago. The results of that check were described in the post "PVS-Studio: analyzing ReactOS's code". After re-checking the project, we have found a lot of new bugs and suspicious code fragments. This fact proves very well that static code analysis should be performed regularly, not occasionally! Doing it that way will help you to significantly reduce the number of errors at the stage of coding already, which means that errors detected will take much less time to eliminate.

Note that the article describes far not all the fragments worth considering. ReactOS has become a big boy now: the solution includes 803 projects. For those, the PVS-Studio analyzer has generated a good deal of general warnings:

It was natural that I didn't find enough courage just to sit down and study all these warnings in detail. So, I'll only point at the most suspicious fragments that caught my glance. There certainly must be other warnings that should be examined as attentively; and there are also diagnostics related to 64-bit errors and micro-optimizations which I didn't examine at all.

The PVS-Studio demo version will be insufficient to examine all the 4887 warnings. However, we are friendly to open-source projects: if the ReactOS developers ask us, we'll give them our tool for free for a while.

Misprints

PVS-Studio is good at detecting various misprints. We may call it its "hobbyhorse". This is a very useful function, as misprints inevitably exist in any project. Let's see what ReactOS has to show us in this field.

Variable overwrite

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

The assignment operation overwrites the previous value of the XAFlags term. The following text should be most likely written instead: cddata->XAFlags |= XA_USE_READ_CD | XA_USE_10_BYTE;. But of course I can't be quite sure, as I don't know the logic of this code.

Repetition in condition

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

The check "src_tex->target != PIPE_TEXTURE_2D" is executed twice. It's another constant that the 'target' term must be compared to for the second time. Otherwise this comparison is unnecessary.

Here's another error of this kind:

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

The correct check seems to be as follows: src->channel[i].type != dst->channel[i].type.

And one more similar error:

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

One more:

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

By the way, this particular bug remains unfixed since the previous check. I didn't describe it in the previous article, although it is included into my error samples base. Don't remember why I didn't mention it - perhaps I was concerned about not making the article too large. The developers must have never run PVS-Studio on their project, and the bug has successfully survived inside the code for at least a couple of years.

One more:

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

The error is quite obvious, I believe. Don't know how to fix it.

Be patient - I have some other twin bugs to show you. And I can't help it... You see, these are very typical software bugs.

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

One of the "ir->from == 0" comparisons must be replaced with "ir->to == NULL".

The same error, caused through the copy-paste technology, can be found here: 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

Unnecessary semicolon

We have finally got to another class of misprints - the unnecessary semicolon ';' that spoils everything.

int BlockEnvToEnvironA(void)
{
  ....
  for (envptr--; envptr >= _environ; envptr--);
    free(*envptr);
  ....
}

V529 Odd semicolon ';' after 'for' operator. environ.c 67

Note the ';' character after the 'for' operator. It results in the free() function being called only once, which leads to memory leaks. It also causes releasing a memory area that wasn't intended to be released. This is how the incorrect code works in its present state:

free(envptr >= _environ ? _environ[-1] : envptr);

The same semicolons can be found here:

Incorrect expression

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

One of the operands seems to have an incorrectly defined name in the expression.

Here's a misprint that causes the size of a rectangle to be calculated incorrectly.

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

I'm almost sure that the following code must be written here: "rect->Y = gdip_round(rectf.Y);". If it's not so, there should be some comment on this.

The following is a code fragment where a variable is assigned to itself:

DWORD WINAPI
DdGetDriverInfo(LPDDHAL_GETDRIVERINFODATA pData)
{
  ....
  pUserColorControl->dwFlags = pUserColorControl->dwFlags;
  ....
}

V570 The 'pUserColorControl->dwFlags' variable is assigned to itself. gdientry.c 1029

The assignment is meaningless. The expression must be incomplete, or something is messed up. The same error here:

V570 The 'Irp->IoStatus.Information' variable is assigned to itself. hidclass.c 461

Let's talk about null pointers

If you have a C/C++ application, you have troubles with pointers. This is the price we have to pay for the language's efficiency. However, C++ and especially C++11 offer a number of ways to avoid handling wild pointers. But that is a subject to be discussed individually.

Let's see what can be found in ReactOS regarding this kind of bugs.

Null pointer dereferencing

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

If the "case ACPI_NOTIFY_EJECT_REQUEST:" branch is chosen in the 'switch' operator, the 'device' pointer will still be equal to zero at the moment. Dereferencing it in the "device->pnp.bus_id" expression will have unpleasant consequences.

In the same bad way the 'device' variable is used in some other fragments:

Here's another code fragment where a variable remains equal to zero by the moment it must be used:

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

At the moment of calling the value() function, the 'tag' variable will still equal zero. That's no good. There are some other similar null pointer dereferencing bugs found in ReactOS:

Passing a null pointer into a function

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

The wcscpy() function is called only if the 'lpMsgBuf' variable equals zero. This variable is passed as an argument into the 'wcscpy' function. It's hooliganism to pass zero into the 'wcscpy' function.

Here, another hooligan is torturing a cat the strstr() function:

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

The _wcsicmp() function has suffered for company as well:

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

By the time the _wcsicmp() function must be called, the pointer tmpPrefix is still a null pointer.

Dereferencing a possible null pointer

There are very many code fragments where the pointer is first dereferenced and only then is checked for being a null pointer. It's not always an error. Perhaps the pointer simply cannot be a null pointer, and the check is just unnecessary. But such code usually appears due to inattention and is incorrect. It works only until the unhappy pointer suddenly becomes a null pointer through a coincidence.

I will cite only one simple example here:

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

Look, the 'PolicyAccountDomainInfo' pointer is dereferenced first. And then it is suddenly checked for being a null pointer. Such a code is usually created as a result of swift refactoring. Variables are starting to be used when there are not checked yet.

The reason why I'm describing only one error of this kind is that they all look much alike. And also because they are AWFULLY NUMEROUS. I'm not interested in examining and describing each individual case. Moreover, it's impossible to include them all into the article - it would then be a reference book instead. That's why I'll just cite the diagnostic messages for you:

Macros

Macros are bad - of that I'm still dead sure. You should use regular functions wherever possible.

Someone felt too lazy to make a full-fledged function stat64_to_stat() in ReactOS and contented himself/herself with creating a shit-macro. This is what it looks like:

#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; \

Let's see how this macro is used in the _tstat function:

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

Do you think the 'stat64_to_stat' macro is executed if the 'ret' variable equals zero? It is absolutely not. The macro is expanded into a set of separate lines. That's why only the "buf->st_dev = (buf64)->st_dev;" line refers to the 'if' operator, while all the other lines will be executed all the time!

There are other fragments that employ this incorrect macro:

Conditions which are always true/false

Here's an issue when an always true condition might cause an infinite loop.

#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

The loop is meant to search through the DISKREADBUFFER array for a byte whose value doesn't equal '0xCD'. If such a byte doesn't exist, the 'Changed' variable always has the FALSE value. In this case, the "i < DISKREADBUFFER_SIZE" expression is the truncation condition. As this expression is always true, the program will start iterating an infinite loop.

The error is this: the 'i' variable has the 'unsigned short' type. It can take values within the range from 0 to 65535. These values are always below '0x10000'.

A typical error I often see in many projects is the assumption that SOCKET is a signed variable. It's not so. To be more exact, it depends on the library implementation.

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

The 'udpsocket' variable is unsigned, which means that the 'ads->udpsocket < 0' condition is always false. To figure out where the error has occurred we need to use the SOCKET_ERROR constant.

Similar socket handling errors can be found here:

Incorrect checks may lead to buffer overflows and, consequently, to undefined behavior. Here's a sample where the exception handler fails.

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

The 'LeftOfBuffer' variable should most likely be a signed one.

It often happens that unsigned variables cause function return values to be checked incorrectly. Here's such a code:

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

The NtUserMenuItemFromPoint() function can return the negative value (-1). The error occurs because of the 'Id' variable being unsigned. That results in the '0 <= Id' check being meaningless.

A function parameter is checked incorrectly in the following code fragment.

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

It's not interesting to discuss V547 warnings any further, so let me just cite the remaining fragments I've noticed:

Undefined behavior and Unspecified behavior.

You must not shift negative numbers - even if the code that has these shifts seems to work successfully for a long time. It is incorrect. It leads to undefined or unspecified behavior. The issue may reveal itself when you start using another platform or another compiler or change optimization switches. I discussed negative number shifts in detail in the article "Wade not in unknown waters. Part three".

This is an incorrect code sample:

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

No one knows what the (-16 << shift) expression is equal to. Other similar fragile code samples can be found in the following fragments:

Incorrect format specifier

Let's have a look at several samples demonstrating incorrect ways of using variadic functions to print variable values.

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

You should write "%llu" instead of "%ull" to print a 64-bit variable.

Using "%u" is one more incorrect way to print the pointer value. There exists the "%p" specifier for this purpose. However, the programmer must have made a misprint in the code below, and it is "%s" that should have been written there.

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

The errors when Unicode and non-Unicode strings are used together are very frequent. For example, if you need to print a UNICODE character in the fprintf() function, you should use '%C', not '%c'. Here's an incorrect code sample with that error:

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

The same bugs can be found in the following fragments:

Operation priorities

I've found several errors related to operation priorities confusion.

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

According to operation priorities in C/C++, the "SafeArrayGetUBound(sa, 1, &size) != S_OK" comparison is executed in the first place, while it is only then that assignment is performed. However, the condition will work well. The incorrect thing is that the 'hr' variable will store 0 or 1 instead of the status. The function will therefore return an incorrect status.

Here is another very similar error:

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

The code is difficult to read. But if you look close, you'll notice that the UnDecorateSymbolName() function's return result is compared to zero first, then the comparison result is put into the 'sym_info->NameLen' variable.

Array index out of bounds

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

The programmer intended 'lastPtr' to point at a memory cell after that last character in the string. That won't happen though. The string consists of WCHAR characters. It means that it's the buffer size that is added, not the number of characters. And that value is twice larger than necessary. When writing the null character, the array index out of bounds error with all its implications will occur.

This is what the fixed code looks like:

FF_T_WCHAR *lastPtr = pDirent->FileName +
  sizeof(pDirent->FileName) / sizeof(pDirent->FileName[0]);

The strncat() function is pretty dangerous regarding this class of bugs. The reason is that it's not the total buffer size that the last argument should specify, but how many more characters can be put into it. Because of misunderstanding this thing, programmers write unsafe code:

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

It cannot be guaranteed that no buffer overflow occurs. To learn more about this class of errors, see the documentation (V645 diagnostic).

A similar trouble can be found here:

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

Repetitions

Repetitions are related to conditions and can be of two types.

Type one. The same operations are executed regardless of the condition. For example:

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

Another example:

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

One more repetition of a large code fragment can be found here:

V523 The 'then' statement is equivalent to the 'else' statement. tab.c 1043

Type two. A condition is repeated. It appears that the second condition will never hold. For example:

#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

I suppose that the second check should have been written in the following way:

if (time_data[i]==LOCALE_SLONGDATE && !lcid[LC_TIME])

Other similar repeating checks can be found here:

Miscellaneous

Now let's have a look at diverse bugs.

Incorrect calculation of the characters number

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

It seems that the "sizeof(DosDevices.Buffer) / sizeof(WCHAR)" expression was intended to calculate the number of characters in a string. But 'DosDevices.Buffer' is just a pointer. As a result, the pointer size is divided by 'sizeof(WCHAR)'. Other similar errors can be found here:

Here's another case of incorrect calculation of the number of characters in strings. In the following sample it's multiplication instead of division:

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

It results in the ListView_GetItemText() function assuming that the buffer size is larger than it actually is. It may potentially cause a buffer overflow.

Function return result not used

#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

Uninitialized variable

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

While calling the RegCreateKeyExW() function, the 'hKey' variable is not initialized yet.

High-order bits that may be significant get truncated

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

This type of errors is very much unobvious. I suggest that you read the description of the V642 diagnostic to understand the point. To put it briefly, the trouble is that the memcmp() function doesn't necessarily return only values -1, 0, and 1. It may well return, for instance, number 0x100000. When casting this number to the "unsigned short" type, it will turn into 0.

One-time loops

I've encountered several very strange loops. They don't have the 'continue' operator yet have the unconditional operator 'break'. It means that the loop bodies are executed only once. Here's an example of that kind.

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

Other similar strange loops:

Strange things

There are code fragments which are probably not bugs. They are simply very strange. For example:

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

The 'MinLength' variable is assigned different values twice in a row. Perhaps it somehow helps in debugging - I don't know. I would consider this an error, but there are many fragments of that kind throughout the code. I won't mention them, as the post is already huge enough.

Conclusion

I fail to make any wise conclusions. ReactOS is a rapidly growing and developing project. Hence it contains quite a lot of errors. As you can see from this article, static analysis can catch a good deal of them in a project like that. If one used it regularly, the benefit would be just invaluable.

Follow us on Twitter to keep track of PVS-Studio's new interesting feats in its struggle against bugs. There we also post links to interesting articles on C/C++ programming and related subjects.