SvyatoslavMC August 13, 2014 at 12:03

Checking Wine with PVS-Studio and Clang Static Analyzer

In the article I want to talk about the verification of the Wine project by such static C / C ++ code analyzers as PVS-Studio and Clang Static Analyzer.

Wine

Wine (Wine Is Not Emulator - Wine is not an emulator) is a set of programs that allows users of Linux, Mac, FreeBSD, and Solaris to run Windows applications without having to install Microsoft Windows on their computers. Wine is an actively developing cross-platform freeware distributed under the GNU Lesser General Public License.

The source code for the project can be obtained with the git clone git command: //source.winehq.org/git/wine.git

About analyzers

PVS-Studio is a static analyzer that detects errors in the source code of C / C ++ / C ++ 11 applications. To verify the project, the release version of PVS-Studio 5.18 was used.
Clang Static Analyzer is a static analyzer that finds errors in C, C ++ and Objective-C programs. To verify the project, we used the release version of Clang 3.4.2 for openSUSE 13.1.

Test Results in PVS-Studio

Negative shift

V610 Undefined behavior. Check the shift operator '<<. The left operand '(LONGLONG) - 1' is negative. propvar.c 127

...
if (*res >= ((LONGLONG)1 << (dest_bits-1)) ||
  *res < ((LONGLONG)-1 << (dest_bits-1)))
  return HRESULT_FROM_WIN32(ERROR_ARITHMETIC_OVERFLOW);
...

The LONGLONG type is declared as 'typedef signed __int64 LONGLONG;', i.e. is an iconic type. Offsets of negative numbers by the new standard lead to undefined or unspecified behavior. Why such a code can work and how to fix it better can be found in the article: Without knowing the ford, do not get into the water. Part three.

Typos

V501 There are identical sub-expressions '! LpScaleWindowExtEx-> xNum' to the left and to the right of the '||' operator. enhmetafile.c 1418

...
if (!lpScaleWindowExtEx->xNum || !lpScaleWindowExtEx->xDenom ||
    !lpScaleWindowExtEx->xNum || !lpScaleWindowExtEx->yDenom)
  break;
...

In the condition, lpScaleWindowExtEx-> xNum is checked twice, most likely in one place there should be lpScaleWindowExtEx-> yNum. In the declaration of the structure used, such a field is:

typedef struct {
    EMR  emr;
    LONG xNum;   //<==
    LONG xDenom;
    LONG yNum;   //<==
    LONG yDenom;
} EMRSCALEVIEWPORTEXTEX, *PEMRSCALEVIEWPORTEXTEX,
  EMRSCALEWINDOWEXTEX,   *PEMRSCALEWINDOWEXTEX;

V501 There are identical sub-expressions '! (Types [i + 1] & PathPointTypeBezier)' to the left and to the right of the '||' operator. graphics.c 1751

....
for(i = 1; i < count; i++){
  ....
  if((i + 2 >= count) ||
    !(types[i + 1] & PathPointTypeBezier) ||
    !(types[i + 1] & PathPointTypeBezier)){
    ....
  }
  i += 2;
}
....

This place was discovered by a similar diagnosis of V501 , but the reason for the same conditions is not so obvious here. Most likely, one condition should have types [i + 2], because above we checked the ability to access an array element with an index 2 more than 'i'.

V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. request.c 3354

if ((hr = SafeArrayAccessData( sa, (void **)&ptr )) != S_OK)
  return hr;
if ((hr = SafeArrayGetUBound( sa, 1, &size ) != S_OK)) //<==
{
    SafeArrayUnaccessData( sa );
    return hr;
}

Operator priority! = Is higher than assignment operator =. Moreover, by the condition above it is clearly seen that here it is also necessary to wrap the assignment in one more parenthesis, otherwise hr gets the value 0 or 1.

Another similar place:

V501 There are identical sub-expressions to the left and to the right of the '|' operator: VT_ARRAY | VT_ARRAY vartest.c 2161

Cascading conditional statements

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1754, 1765. msi.c 1754

if (!strcmpW( szProperty, INSTALLPROPERTY_LOCALPACKAGEW )) //<==
{
  ...
}
else if (!strcmpW( szProperty, INSTALLPROPERTY_INSTALLDATEW ))
{
  ...
}
else
if (!strcmpW( szProperty, INSTALLPROPERTY_LOCALPACKAGEW )) //<==
{
  ...
}
else if (!strcmpW( szProperty, INSTALLPROPERTY_UNINSTALLABLEW ) ||
         !strcmpW( szProperty, INSTALLPROPERTY_PATCHSTATEW ) ||
         !strcmpW( szProperty, INSTALLPROPERTY_DISPLAYNAMEW ) ||
         !strcmpW( szProperty, INSTALLPROPERTY_MOREINFOURLW ))
{
  ...
}
else
{
  ...
}

If identical conditions are checked in the cascade of conditional statements, then the latter do not receive control. Perhaps a typo here in the passed constant INSTALLPROPERTY_ *.

Equivalent branches of the if statement

V523 The 'then' statement is equivalent to the 'else' statement. filedlg.c 3302

if(pDIStruct->itemID == liInfos->uSelectedItem)
{
  ilItemImage = (HIMAGELIST) SHGetFileInfoW (
    (LPCWSTR) tmpFolder->pidlItem, 0, &sfi, sizeof (sfi),
    shgfi_flags );
}
else
{
  ilItemImage = (HIMAGELIST) SHGetFileInfoW (
    (LPCWSTR) tmpFolder->pidlItem, 0, &sfi, sizeof (sfi),
    shgfi_flags );
}

Such a code is either redundant or a typo takes place.

V523 The 'then' statement is equivalent to the 'else' statement. genres.c 1130

...
if(win32)
{
  put_word(res, 0);  /* Reserved */
  /* FIXME: The ResType in the NEWHEADER structure should
   * contain 14 according to the MS win32 doc. This is
   * not the case with the BRC compiler and I really doubt
   * the latter. Putting one here is compliant to win16 spec,
   * but who knows the true value?
   */
  put_word(res, 1);  /* ResType */
  put_word(res, icog->nicon);
  for(ico = icog->iconlist; ico; ico = ico->next)
  {
    ...
  }
}
else /* win16 */
{
  put_word(res, 0);  /* Reserved */
  put_word(res, 1);  /* ResType */
  put_word(res, icog->nicon);
  for(ico = icog->iconlist; ico; ico = ico->next)
  {
    ...
  }
}
...

Here one of the repeating branches is commented out. Perhaps the analyzer found a place that was not completed. Not a mistake, but decided to note.

Change string length

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. appdefaults.c 390

...
section[strlen(section)] = '\0'; /* remove last backslash  */
...

In fact, zero will be written to zero and nothing will change. For the strlen () function to work, the string must already end with a terminal zero. The comment hints to us that they probably wanted to cut off the slash at the end. Then, apparently, the code should be like this:

section[strlen(section) - 1] = '\0';

One counter for two cycles

V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 980, 1003. iphlpapi_main.c 1003

...
for (i = 0; i < num_v6addrs; i++)    //<==
{
    ...
    for (i = 0; i < 8 && !done; i++) //<==
    {
        ...
    }
    ...
    if (i < num_v6addrs - 1)
    {
        prefix->Next = (IP_ADAPTER_PREFIX *)ptr;
        prefix = prefix->Next;
    }
}
...

Suspicious place: the nested loop is organized using the variable i, which is also used in the outer loop.

Double cast

In the following two examples, two casts are applied to the * void pointer: first to char *, then to DWORD *, after which the offset is added. The expression lacks parentheses, or the code is redundant. Whether there is an error here depends on how much they wanted to increase the value of the pointer.

V650 Type casting operation is utilized 2 times in succession. Next, the '+' operation is executed. Probably meant: (T1) ((T2) a + b). typelib.c 9147

...
struct WMSFT_SegContents arraydesc_seg;
typedef struct tagWMSFT_SegContents {
    DWORD len;
    void *data;
} WMSFT_SegContents;
...
DWORD offs = file->arraydesc_seg.len;
DWORD *encoded;
encoded = (DWORD*)((char*)file->arraydesc_seg.data) + offs;//<==

Another similar situation:

V650 Type casting operation is utilized 2 times in succession. Next, the '+' operation is executed. Probably meant: (T1) ((T2) a + b). protocol.c 194

INT WINAPI
EnumProtocolsW(LPINT protocols, LPVOID buffer, LPDWORD buflen)
{
  ...
  unsigned int string_offset;
  ...
  pi[i].lpProtocol = (WCHAR*)(char*)buffer + string_offset;//<==
  ...
}

Unsigned number difference

V555 The expression 'This-> nStreams - nr> 0' will work as 'This-> nStreams! = Nr'. editstream.c 172

static HRESULT
AVIFILE_RemoveStream(IAVIEditStreamImpl* const This, DWORD nr)
{
  ...
  This->nStreams--;
  if (This->nStreams - nr > 0) { //<==
    memmove(This->pStreams + nr, This->pStreams + nr + 1,
            (This->nStreams - nr) * sizeof(EditStreamTable));
  }
  ...
}

The variable nr is of the base type DWORD. Subtracting it, the result of the difference will also have an unsigned type. If nr is greater than This-> nStreams, the condition will still be true.

Similar place:

V555 The expression 'This-> fInfo.dwStreams - nStream> 0' will work as 'This-> fInfo.dwStreams! = NStream'. avifile.c 469

First execution, then lunch

V595 The 'decl' pointer was utilized before it was verified against nullptr. Check lines: 1411, 1417. parser.y 1411

...
var_t *v = decl->var; //<==
expr_list_t *sizes = get_attrp(attrs, ATTR_SIZEIS);
expr_list_t *lengs = get_attrp(attrs, ATTR_LENGTHIS);
int sizeless;
expr_t *dim;
type_t **ptype;
array_dims_t *arr = decl ? decl->array : NULL;     //<==
type_t *func_type = decl ? decl->func_type : NULL; //<==
...

First, they took the value according to the pointer, then decided to check.

Similar places:

V595 The 'pcbData' pointer was utilized before it was verified against nullptr. Check lines: 1859, 1862. registry.c 1859
V595 The 'token_user' pointer was utilized before it was verified against nullptr. Check lines: 206, 213. lsa.c 206
V595 The 'psp' pointer was utilized before it was verified against nullptr. Check lines: 2680, 2689. propsheet.c 2680
V595 The 'lpFindInfo' pointer was utilized before it was verified against nullptr. Check lines: 6285, 6289. listview.c 6285
V595 The 'compiland' pointer was utilized before it was verified against nullptr. Check lines: 287, 294. symbol.c 287
V595 The 'graphics' pointer was utilized before it was verified against nullptr. Check lines: 2096, 2112. graphics.c 2096
V595 The 'current' pointer was utilized before it was verified against nullptr. Check lines: 240, 251. request.c 240

Printing the result of the same functions

V681 The language standard does not define an order in which the 'tlb_read_byte' functions will be called during evaluation of arguments. tlb.c 650

...
printf("\\%2.2x \\%2.2x\n", tlb_read_byte(), tlb_read_byte());
...

According to the C ++ language standard, the sequence of calculating the actual arguments of a function is not defined. Which function will be called first depends on the compiler, compilation options, and so on.

Dubious tests

In the directories of some modules there is a test directory with source files for tests. The macro 'ok' is used to display debugging information. Here are a few suspicious places:

V501 There are identical sub-expressions to the left and to the right of the '==' operator: ddsd3.lpSurface == ddsd3.lpSurface dsurface.c 272

...
ok(ddsd3.lpSurface == ddsd3.lpSurface,    //<==
  "lpSurface from GetSurfaceDesc(%p) differs\
    from the one returned by Lock(%p)\n",
  ddsd3.lpSurface, ddsd2.lpSurface);      //<==
...

Very similar to a typo. Most likely, the same variables that are printed should be compared here.

V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '==' operator. url.c 767

...
ok(size == no_callback ? 512 : 13, "size=%d\n", size);
...

The priority of the "==" operator is above '?:', So here the size variable is not compared with the values 512 and 13. The expression is always true, since it will be equal to 512 or 13 and therefore the check will not check anything.

Similar warnings:

V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '==' operator. string.c 1086
V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '==' operator. string.c 1111
V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '==' operator. reader.c 761
V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '==' operator. protocol.c 2928
V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '==' operator. dde.c 1594
V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '==' operator. reader.c 761

Test Results in Clang Static Analyzer

The search for potential errors by this analyzer is bypassing possible scripts for program execution. If a suspicious place is found, the analyzer will create a report for this file in HTML format (by default) or PLIST, in which one to several dozen steps leading to the suspicious code section will be commented.

Most of the messages received during the verification of the Wine project are of the same nature: a variable is declared without initialization; in the function to which the variable address is passed, initialization is skipped in some branches of the switch statement, or 'return' is executed before initialization. Similar situations are not processed further and the uninitialized variable continues to be used. The account of such places in the project goes to hundreds, therefore, this review will not be considered. Some of them, I think, can present real serious errors. Let's leave this to the conscience of the developers.

Uninitialized variable in condition

File: dlls / atl110 /../ atl / atl_ax.c

Location: line 1092, column 10

Description: Branch condition evaluates to a garbage value

HRESULT
WINAPI AtlAxCreateControlEx(LPCOLESTR lpszName, HWND hWnd,
  IStream *pStream, IUnknown **ppUnkContainer,
  IUnknown **ppUnkControl, REFIID iidSink, IUnknown *punkSink)
{
  ...
  IUnknown *pContainer;
  ...
  hRes = AtlAxAttachControl( pUnkControl, hWnd, &pContainer );
  if ( FAILED( hRes ) ) 
    WARN("cannot attach control to window\n");
  ...
  if ( pContainer ) //<==
  //Clang: Branch condition evaluates to a garbage value
        IUnknown_Release( pContainer );
  return S_OK;
}

The uninitialized variable pContainer is used in the condition after calling AtlAxAttachControl. A description of this feature is provided below.

HRESULT
WINAPI AtlAxAttachControl(IUnknown *control, HWND hWnd,
                          IUnknown **container)
{
  HRESULT hr;
  ...
  if (!control)
    return E_INVALIDARG;//<==
  hr = IOCS_Create( hWnd, control, container );
  return hWnd ? hr : S_FALSE;
}

This can return E_INVALIDARG before the container variable is initialized. As a result, the AtlAxCreateControlEx function will generate a warning and continue to work with the uninitialized variable.

Possible buffer overflow

File: tools / widl / typegen.c

Location: line 1158, column 28

Description: String copy function overflows destination buffer

static unsigned int write_new_procformatstring_type(...)
{
  char buffer[64];
  ...
  strcpy( buffer, "/* flags:" );
  if (flags & MustSize) strcat( buffer, " must size," );
  if (flags & MustFree) strcat( buffer, " must free," );
  if (flags & IsPipe) strcat( buffer, " pipe," );
  if (flags & IsIn) strcat( buffer, " in," );
  if (flags & IsOut) strcat( buffer, " out," );
  if (flags & IsReturn) strcat( buffer, " return," );
  if (flags & IsBasetype) strcat( buffer, " base type," );
  if (flags & IsByValue) strcat( buffer, " by value," );
  if (flags & IsSimpleRef) strcat( buffer, " simple ref," );
  ...
}

If you do not even meet all the conditions, there is a chance to get a line that is too long that does not fit in the buffer.

Potential memory leak

File: libs / wpp / ppl.yy.c

Location: line 4475, column 1

Description: Potential memory leak

static void macro_add_arg(int last)
{
  ..
  if(last || mep->args[mep->nargs-1][0])
  {
    yy_push_state(pp_macexp);
    push_buffer(NULL, NULL, NULL, last ? 2 : 1);
    ppy__scan_string(mep->args[mep->nargs-1]);
    //Clang: Calling 'ppy__scan_string'
    //Clang: Returned allocated memory
  }
    //Clang: Potential memory leak
}

The pyy__scan_string function has a return value that is not used. Calling this function one way or another leads to the return of the value by the malloc () function, after using which it is necessary to free memory.

Let's look at how calling the pyy__scan_string function results in a call to malloc.

YY_BUFFER_STATE ppy__scan_string (yyconst char * yystr )
{
  return ppy__scan_bytes(yystr,strlen(yystr) );
}
YY_BUFFER_STATE ppy__scan_bytes  (yyconst char * yybytes,
                                  yy_size_t  _yybytes_len )
{
  YY_BUFFER_STATE b;
  char *buf;
  ...
  buf = (char *) ppy_alloc(n  );
  ...
  b = ppy__scan_buffer(buf,n );
  ...
  return b;
}
YY_BUFFER_STATE ppy__scan_buffer  (char * base, yy_size_t size )
{
  YY_BUFFER_STATE b;
    ...
  b=(YY_BUFFER_STATE) ppy_alloc(sizeof(struct yy_buffer_state));
  ...
  return b;
}
void *ppy_alloc (yy_size_t  size )
{
  return (void *) malloc( size );
}

Division by zero

File: dlls / winex11.drv / palette.c

Location: line 601, column 43

Description: Division by zero

#define NB_RESERVED_COLORS 20
...
static void X11DRV_PALETTE_FillDefaultColors(....)
{
  ...
  int i = 0, idx = 0;
  int red, no_r, inc_r;
  ...
  if (palette_size <= NB_RESERVED_COLORS)
    return;
  while (i*i*i < (palette_size - NB_RESERVED_COLORS)) i++;
  no_r = no_g = no_b = --i;
  ...
  inc_r = (255 - NB_COLORCUBE_START_INDEX)/no_r;
  //Clang: Division by zero
  ...
}

The code will continue to run if the value of the palette_size variable is 21 or more. With a value of 21, the variable 'i' at the beginning will be increased by one, and then reduced by one. As a result, 'i' will remain equal to zero, which will lead to division by zero.

Uninitialized array element

File: dlls / avifil32 / api.c

Location: line 1753, column 10

Description: Assigned value is garbage or undefined

#define MAX_AVISTREAMS 8
...
HRESULT WINAPI AVISaveVW(....int nStreams ....)
{
  ...
  //Объявление массива из 8 элементов, [0..7]
  PAVISTREAM     pInStreams[MAX_AVISTREAMS];
  ...
  if (nStreams >= MAX_AVISTREAMS) {
    WARN(...);
    return AVIERR_INTERNAL;
  }
  ...
  //Инициализация первых семи элементов, [0..6].
  for (curStream = 0; curStream < nStreams; curStream++) {
    pInStreams[curStream]  = NULL;
    pOutStreams[curStream] = NULL;
  }
  ...
  for (curStream = 0; curStream < nStreams; curStream++) {
  ...
  if (curStream + 1 >= nStreams) {
    /* move the others one up */
    PAVISTREAM *ppas = &pInStreams[curStream];
    int            n = nStreams - (curStream + 1);
    do {
      *ppas = pInStreams[curStream + 1];
      //Clang: Assigned value is garbage or undefined
    } while (--n);
  }
  ...
  }
...
}

An array of 8 elements is declared here. The code will continue to execute if the value of the nStreams variable is less than 8, i.e. maximum 7. All cycles in this function, with a conditional expression (curStream <nStreams), do not count the last element, both during initialization and during use. In the place where the Clang message was issued, the eighth element with index 7 is taken, because the expression (curStream + 1> = nStreams) will be true with the values curStream == 6 and nStreams == 7. Addressing the pInStreams [curStream + 1 array ] will give the last element uninitialized earlier.

Zero way

File: dlls / crypt32 / rootstore.c
Location: line 413, column 10
Description: Null pointer passed as an argument to a 'nonnull' parameter

static BOOL import_certs_from_path(LPCSTR path,
  HCERTSTORE store, BOOL allow_dir)
{
  ...
  fd = open(path, O_RDONLY);
  //Clang: Null pointer passed as
  //an argument to a 'nonnull' parameter
  ...
}

To understand why Clang believes that NULL can come here, consider the place where this function is called:

static BOOL import_certs_from_dir(LPCSTR path, HCERTSTORE store)
{
  ...
  char *filebuf = NULL;
  //Clang: 'filebuf' initialized to a null pointer value
  struct dirent *entry;
  while ((entry = readdir(dir)))
  {
    ...
    size_t name_len = strlen(entry->d_name);
    //Вызов функции для изменения filebuf
    if (!check_buffer_resize(&filebuf, &bufsize,
                              path_len + 1 + name_len + 1))
    {
      ERR(...);
      break;
    }
    snprintf(filebuf, bufsize, "%s/%s", path, entry->d_name);
    if (import_certs_from_path(filebuf, store, FALSE) && !ret)
      //Clang: Passing null pointer value via 1st parameter 'path'
      //Clang: Calling 'import_certs_from_path'
      ret = TRUE;
    ...
  }
}

Here the function check_buffer_resize is called, in which the value of the filebuf variable should be changed or FALSE returned, but the function may not change filebuf and return TRUE, consider the function code below:

static BOOL check_buffer_resize(char **ptr_buf,
  size_t *buf_size, size_t check_size)
{
  if (check_size > *buf_size)
  {
    ...
    *ptr_buf = CryptMemAlloc(*buf_size);
    ...
  }
  return TRUE;
}

The function contains only one condition in which the ptr_buf variable is changed, and if the condition is not met, the true return result allows you to use this variable in the future.

A similar situation with the memcpy () function:

File: server / directory.c

Location: line 548, column 21

Description: Null pointer passed as an argument to a 'nonnull' parameter

Invalid Check

File: dlls / advapi32 / registry.c

Location: line 1209, column 13

Description: Array access (from variable 'str') results in a null pointer dereference

LSTATUS WINAPI RegSetValueExW(...., const BYTE *data, .... )
{
  ...
  if (data && ((ULONG_PTR)data >> 16) == 0)
    //Assuming pointer value is null
    return ERROR_NOACCESS;
  if (count && is_string(type))
  {
    LPCWSTR str = (LPCWSTR)data;
    //Clang: 'str' initialized to a null pointer value
    if (str[count / sizeof(WCHAR) - 1] &&
        !str[count / sizeof(WCHAR)])
    //Clang: Array access (from variable 'str') results in
    //a null pointer dereference
        count += sizeof(WCHAR);
  }
  ...
}

If the null pointer data comes, the program will continue to run until the str variable is accessed.

Similar place:

File: dlls / comctl32 / comctl32undoc.c

Location: line 964, column 12

Description: Array access (from variable 'lpDest') results in a null pointer dereference

Conclusion

Compared analyzers PVS-Studio and Clang Static Analyzer use different code analysis methodology, therefore, different but good results were obtained for both analyzers.

It is worth noting that Clang Static Analyzer was distinguished by the lack of diversity in diagnostics. In fact, he warns everywhere that the variable has an incorrect value (null pointer, zero, uninitialized variable, and so on). Depending on the value of the variable and how it is used, a diagnostic message is generated. PVS-Studio is more diverse in types of diagnostics and is well able to detect various typos.

Of course, I could have missed something in the reports. Therefore, viewing the reports of any analyzers by the authors of the source code will give the best result.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Checking Wine with PVS-Studio and Clang Static Analyzer .

Have you read the article and have a question?

Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2014 . Please see the list.

Tags: