Re-checking the ReactOS project - a large report

Here you can discuss ReactOS related topics.

Moderator: Moderator Team

Post Reply
Andrey_Karpov
Posts: 11
Joined: Fri Sep 16, 2011 6:28 pm

Re-checking the ReactOS project - a large report

Post by Andrey_Karpov »

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.

[ external image ]

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:
  1. 1320 first-level warnings;
  2. 814 second-level warnings;
  3. 2753 third-level 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

Code: Select all

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

Code: Select all

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:

Code: Select all

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.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.type != dst->channel.type.



And one more similar error:

Code: Select all

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:

Code: Select all

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:

Code: Select all

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.

Code: Select all

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.

Code: Select all

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:

Code: Select all

free(envptr >= _environ ? _environ[-1] : envptr);
The same semicolons can be found here:
  • V529 Odd semicolon ';' after 'for' operator. environ.c 119
  • V529 Odd semicolon ';' after 'for' operator. environ.c 171


Incorrect expression

Code: Select all

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.

Code: Select all

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:

Code: Select all

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

Code: Select all

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:
  • 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


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

Code: Select all

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:
  • 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
Passing a null pointer into a function

Code: Select all

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:

Code: Select all

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:

Code: Select all

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:

Code: Select all

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:

LIST.

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:

Code: Select all

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

Code: Select all

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:
  • 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
Conditions which are always true/false

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

Code: Select all

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

Code: Select all

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:
  • 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


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

Code: Select all

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:

Code: Select all

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.

Code: Select all

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

Code: Select all

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:
  • 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
Incorrect format specifier

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

Code: Select all

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.

Code: Select all

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:

Code: Select all

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:
  • 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
Operation priorities

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

Code: Select all

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:

Code: Select all

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

Code: Select all

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:

Code: Select all

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:

Code: Select all

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:

Code: Select all

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:

Code: Select all

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:

Code: Select all

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

Code: Select all

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


Other similar repeating checks can be found here:
  • 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
Miscellaneous

Now let's have a look at diverse bugs.

Incorrect calculation of the characters number

Code: Select all

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:
  • 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


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

Code: Select all

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

Code: Select all

#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

Code: Select all

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

Code: Select all

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.

Code: Select all

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:
  • V612 An unconditional 'break' within a loop. regexp.c 3633
  • V612 An unconditional 'break' within a loop. hlpfile.c 1131


Strange things

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

Code: Select all

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.
KnownSyntax
Posts: 16
Joined: Sat Dec 15, 2012 12:45 am
Location: Arizona, United States
Contact:

Re: Re-checking the ReactOS project - a large report

Post by KnownSyntax »

Wow this is pretty interesting, hopefully the developers will use this to clean up the code a bit since it could explain why ReactOS has bugs or acts the way it does with certain actions and programs.
FreshAquaria - Freshwater Discussion and Knowledgebase
vicmarcal
Test Team
Posts: 2733
Joined: Mon Jul 07, 2008 12:35 pm

Re: Re-checking the ReactOS project - a large report

Post by vicmarcal »

A bunch of these bugs are being fixed during these days thanks to the report ;)
A pity there isn't a link to the full report or does ReactOS performed so well that we have just these bugs? \0/

PD: A lot of the bugs are WINE or MESA/GALLIUM related. We can't fix them directly. But we can send them a patch or a report. Depends on them to be accepted or not.
Andrey_Karpov
Posts: 11
Joined: Fri Sep 16, 2011 6:28 pm

Re: Re-checking the ReactOS project - a large report

Post by Andrey_Karpov »

vicmarcal wrote:A bunch of these bugs are being fixed during these days thanks to the report ;)
A pity there isn't a link to the full report or does ReactOS performed so well that we have just these bugs? \0/
I can put out a complete list. But what's the point? Line numbers are rapidly shifting. Simply download the PVS-Studio and get a fresh list.
oswetto
Posts: 109
Joined: Mon Oct 26, 2009 10:43 pm

Re: Re-checking the ReactOS project - a large report

Post by oswetto »

Andrey_Karpov
Posts: 11
Joined: Fri Sep 16, 2011 6:28 pm

Re: Re-checking the ReactOS project - a large report

Post by Andrey_Karpov »

I followed your strange desire. Here is the report. But I do not understand how it can work. Download PVS-Studio and do not torment yourself.
PVS-Studio log
Plain text

The PVS-Studio tool allows you to:
1) Filter messages.
2) Turn off messages about optimization and 64-Bit
3) Mark the false alarms
4) And so on.
To work with the log without PVS-Studio is impossible.
Post Reply

Who is online

Users browsing this forum: Ahrefs [Bot], jesse628wallick and 34 guests