In this article, I'm going to tell you about the check of the Wine project done by the static analyzers for C/C++ code PVS-Studio and Clang Static Analyzer.
Wine (originally an acronym for "Wine Is Not an Emulator") enables Linux, Mac, FreeBSD, and Solaris users to run Windows applications without a copy of Microsoft Windows. Wine is free software under constant development. Other platforms may benefit as well.
You can get the project's source code through the git clone command at git://source.winehq.org/git/wine.git
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. it is a signed type. According to the new standard, shifting negative numbers leads to undefined or unspecified behavior. To find out why such code still may work and how to fix it best, see the article Wade not in unknown waters - part three.
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;
...The lpScaleWindowExtEx->xNum condition is checked twice. Instead of one of the checks, the statement lpScaleWindowExtEx->yNum was most likely meant to be used. The corresponding field is found in the structure declaration:
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 fragment was discovered by the same diagnostic V501, but this time it doesn't show the reason for identical conditions that clearly. Most likely, one of them should contain types[i + 2] because the programmer has checked before if the array item with the index larger than 'i' by 2 can be addressed.
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;
}The precedence of the != operator is higher than that of the assignment operator =. And you can see clearly well from the condition above that the assignment should also be enclosed in another pair of parentheses as hr would otherwise get value 0 or 1.
Another similar issue:
V501 There are identical sub-expressions to the left and to the right of the '|' operator: VT_ARRAY | VT_ARRAY vartest.c 2161
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 a cascade of conditional operators, those conditions never get control. Perhaps there is a typo in the constant INSTALLPROPERTY_* passed to this fragment.
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 );
}This code is either excessive or contains a typo.
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)
{
...
}
}
...One of the repeating branches is commented. Perhaps this is an incomplete fragment, not an error, but I decided to point it out anyway.
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 this code, NULL character will actually be written into NULL character and nothing will change. For the strlen() function to work properly, the string must be already null-terminated. The comment suggests that the programmer probably wanted to cut off the last backslash. Then the code should look like this:
section[strlen(section) - 1] = '\0';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;
}
}
...This fragment is suspicious: a nested loop is organized through the i variable which is also used in the external loop.
In the next two examples, the *void pointer is cast to other types twice: first to char*, then to DWORD*, after which an offset is added. Either the expression lacks parentheses or the code is excessive. Whether or not there is an error here depends on how much the programmer wanted to increment 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 issue:
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;// <=
...
}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 nr variable has the unsigned type DWORD. Subtracting it will result in an unsigned value, too. If nr is larger than This->nStreams, then the condition will still be true.
A similar issue:
V555 The expression 'This->fInfo.dwStreams - nStream > 0' will work as 'This->fInfo.dwStreams != nStream'. avifile.c 469
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 the value by pointer is taken, then it is checked.
Other similar fragments:
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 computing a function's actual arguments is not defined. Which function will be called first depends on the compiler, compilation parameters, etc.
Some of the modules' folders contain the test folder with source files for tests. Debug information is printed through the 'ok' macro. Here are a few suspicious fragments:
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); // <=
...It very much looks like a typo. I suspect this code should compare the same variables that are printed.
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 precedence of the "==" operator is higher than that of '?:', so the size variable is not compared to the values 512 and 13. The expression is always true as it evaluates either to 512 or 13, which means this check doesn't check anything.
Other similar fragments:
Clang Static Analyzer implements the mechanism of searching for potential errors through parsing possible application execution branches. When a suspicious fragment is detected, the analyzer creates a report for this file in the HTML format (by default) or the PLIST format, with comments for a number of steps (from one to several dozens) which lead to that suspicious code fragment.
Most of the messages I got while analyzing the Wine project were of the same kind: a variable is not initialized when declared; a function receiving a variable address does not initialize it in certain branches of the switch operator, or 'return' is executed before initialization. Such issues are not handled anywhere further in the code, and the program keeps using the uninitialized variable. There are hundreds of such issues in the project, so I won't discuss them in this article. Some of them, as I guess, may be real, critical bugs. But let's leave them lay on the authors' conscience.
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 the call of AtlAxAttachControl. This function's description is given 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;
}In this code, the E_INVALIDARG value may be returned before initializing the container variable. It will result in the AtlAxCreateControlEx function generating the warning and going on to work with the uninitialized variable.
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," );
...
}Even if not all the conditions are true, you still risk getting a too lengthy string that won't suit the buffer.
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 non-used return value. Calling this function will anyway make the malloc() function return the value, and after it is called, memory must be freed.
Let's see how the call of the pyy__scan_string function leads to calling 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 );
}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 executing if the palette_size variable is larger than or equal to 21. With the value 21, the 'i' variable will be first incremented by one and then decremented by one. As a result, the 'i' variable will remain equal to zero, which will cause the division-by-zero error.
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 ....)
{
...
//Declaring 8-item array, [0..7]
PAVISTREAM pInStreams[MAX_AVISTREAMS];
...
if (nStreams >= MAX_AVISTREAMS) {
WARN(...);
return AVIERR_INTERNAL;
}
...
//Initializing first 7 items, [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);
}
...
}
...
}In this code, an array of 8 items is declared. The code will continue executing as long as the nStreams variable is less than 8, i.e. 7 at most. All the loops in this function with the conditional statement (curStream < nStreams) fail to iterate through the last item, both before its initialization and when using it. The line Clang displayed the message on is just that very line where the eighth item with the index 7 is taken, as the (curStream + 1 >= nStreams) condition will be true at curStream==6 and nStreams==7. Addressing the pInStreams[curStream + 1] array will give us the last, previously uninitialized item.
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 suspects that NULL may get here, let's examine the fragment 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);
//Calling function to change 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;
...
}
}In this code, the check_buffer_resize function is called where either the value of the filebuf variable must change or FALSE must be returned; but the function may fail to change filebuf and return TRUE instead. Take a look at the function's 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 where the ptr_buf variable is changed, and if this condition is false, the true return result will allow you to use this variable further on.
A similar issue is with the memcpy() function:
File: server/directory.c
Location: line 548, column 21
Description: Null pointer passed as an argument to a 'nonnull' parameter
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 gets here, the program will go on executing until addressing the str variable.
Another similar issue:
File: dlls/comctl32/comctl32undoc.c
Location: line 964, column 12
Description: Array access (from variable 'lpDest') results in a null pointer dereference
The PVS-Studio analyzer and Clang Static Analyzer compared in this article use different code analysis methodologies, so it is natural that we've got different yet useful results by both tools.
I should note that Clang Static Analyzer's diagnostics are quite alike. In fact, it actually warns you about a variable having an incorrect value (a null pointer, a zero, uninitialized variable, etc.). Depending on the variable value and how this variable is used, the corresponding diagnostic message is formed. PVS-Studio offers a wider variety of diagnostic types and is good at catching various typos.
Of course, I may have missed something when looking through the reports. That's why it would be much better if the project authors studied the reports by any of the analyzers themselves.