Checking Wine: A Year Later

A little over a year ago, to write an article about checking the project using PVS-Studio, the Wine project was taken. The article was written, the authors were notified and even asked for a full audit report by the analyzer, to which they received a positive response. Recently, one of the developers of the project wrote to us. The article will talk about our communication, the work done by the Wine project team to improve the code, and what remains to be done.

Introduction

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.

In August 2014, an article was published: Checking Wine with PVS-Studio and Clang Static Analyzer . Recently, we received a letter from one of the developers of Wine - Michael Stefaniuc. In a letter, he thanked the PVS-Studio team for using a static analyzer and providing a report.

He also provided some statistics on fixing analyzer warnings for the year. According to this linkyou can find about 180 commits containing source code fixes marked “PVS-Studio”.

Figure 1 shows the statistics of the correction of the 20 most useful types of analyzer warnings for the project, from the point of view of the authors.


Figure 1 - The top 20 successful error codes for Wine

Michael explained that it was already difficult to combine the current source code with the old analyzer report and asked to check the project again. The Wine project is actively developing, the PVS-Studio static analyzer is also actively developing, so I decided to check this project again. The result was this small note where I will describe the 10 most suspicious sections of code. Naturally, the developers received a full report and will be able to explore other potentially dangerous places.

Top 10 Warnings

V650 Warning

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

WINE_HIDP_PREPARSED_DATA* build_PreparseData(....)
{
  ....
  wine_report =
    (WINE_HID_REPORT*)((BYTE*)wine_report)+wine_report->dwSize;
  ....
}

The analyzer detected an addition operation with a variable with which double casts are performed. Most likely they forgot to enclose the first type conversion and the addition operation in brackets. Above in the code there is exactly the same fragment, only with brackets:

wine_report =
  (WINE_HID_REPORT*)(((BYTE*)wine_report)+wine_report->dwSize);

Warning V590

V590 Consider inspecting the 'lret == 0 || lret! = 234 'expression. The expression is excessive or contains a misprint. winemenubuilder.c 3430

static void cleanup_menus(void)
{
  ...
  while (1)
  {
    ....
    lret = RegEnumValueW(....);
    if (lret == ERROR_SUCCESS || lret != ERROR_MORE_DATA)
      break;
  ....
}

The code has a redundant comparison "lret == ERROR_SUCCESS". Apparently there is a logical error. The condition is true for all values ​​of the variable 'lret' equal to 'ERROR_MORE_DATA'. For clarity, you can look at the truth table in Figure 2.


Figure 2 - The truth table of the conditional expression

Two columns are highlighted in red, where the results of logical operations completely coincide.

Another such place:

• V590 Consider inspecting the 'last_error == 183 || last_error! = 3 'expression. The expression is excessive or contains a misprint. schedsvc.c 90

Warning V576

V576 Incorrect format. Consider checking the fourth actual argument of the 'printf' function. To print the value of pointer the '% p' ​​should be used. msvcirt.c 828

DEFINE_THISCALL_WRAPPER(streambuf_dbp, 4)
void __thiscall streambuf_dbp(streambuf *this)
{
  ....
  printf(" base()=%p, ebuf()=%p,  blen()=%d\n",
         this->base, this->ebuf, streambuf_blen(this));
  printf("pbase()=%p, pptr()=%p, epptr()=%d\n",
         this->pbase, this->pptr, this->epptr);
  printf("eback()=%p, gptr()=%p, egptr()=%d\n",
         this->eback, this->gptr, this->egptr);
  ....
}

The analyzer detected a suspicious place where they try to print the value of the pointer using the qualifier '% d. This code fragment was most likely written using the copy-paste method. It can be assumed that the first call to the printf () function was written first, the last argument in which correctly matches the used qualifier '% d'. But then this line was copied two more times and the pointer was passed as the last argument, but they forgot to change the format of the line.

Warning V557

V557 Array overrun is possible. The '16' index is pointing beyond array bound. winaspi32.c 232

/* SCSI Miscellaneous Stuff */
#define SENSE_LEN      14
typedef struct tagSRB32_ExecSCSICmd {
  ....
  BYTE        SenseArea[SENSE_LEN+2];
} SRB_ExecSCSICmd, *PSRB_ExecSCSICmd;
static void
ASPI_PrintSenseArea(SRB_ExecSCSICmd *prb)
{
  BYTE  *rqbuf = prb->SenseArea;
  ....
  if (rqbuf[15]&0x8) {
    TRACE("Pointer at %d, bit %d\n",
          rqbuf[16]*256+rqbuf[17],rqbuf[15]&0x7);      //<==
  }
  ....
}

The analyzer detected a memory access outside the 'rgbuf' array for elements with indices 16 and 17. The array itself contains only 16 elements. Perhaps the condition "rqbuf [15] & 0x8" is rarely true and such an error was not noticed.

Warning V711

V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. dplobby.c 765

static HRESULT WINAPI
IDirectPlayLobby3AImpl_EnumAddressTypes(....)
{
  ....
  FILETIME filetime;
  ....
  /* Traverse all the service providers we have available */
  for( dwIndex=0; RegEnumKeyExA( hkResult, dwIndex, subKeyName,
       &sizeOfSubKeyName,
       NULL, NULL, NULL, &filetime ) != ERROR_NO_MORE_ITEMS;
       ++dwIndex, sizeOfSubKeyName=50 )
  {
    ....
    FILETIME filetime;
    ....
    /* Traverse all the address type we have available */
      for( dwAtIndex=0; RegEnumKeyExA( hkServiceProviderAt,
           dwAtIndex, atSubKey, &sizeOfSubKeyName,
           NULL, NULL, NULL, &filetime ) != ERROR_NO_MORE_ITEMS;
        ++dwAtIndex, sizeOfSubKeyName=50 )
      {
        ....
      }
    ....
  }
  ....
}

The declaration of the variable “filetime” was found in the body of the loop, which coincides with the variable used to control the loop. This will result in the loss of local changes to the "filename" when exiting the inner loop. Looking at the entire function code, it can be assumed that a large piece of code was copied into the body of the loop with minor changes. This may not be a serious danger, anyway it is a bad programming style.

Warning V530

V530 The return value of function 'DSCF_AddRef' is required to be utilized. dsound_main.c 760

static ULONG WINAPI DSCF_AddRef(LPCLASSFACTORY iface)
{
    return 2;
}
HRESULT WINAPI DllGetClassObject(....)
{
  ....
  while (NULL != DSOUND_CF[i].rclsid) {
    if (IsEqualGUID(rclsid, DSOUND_CF[i].rclsid)) {
      DSCF_AddRef(&DSOUND_CF[i].IClassFactory_iface);  //<==
      *ppv = &DSOUND_CF[i];
      return S_OK;
    }
    i++;
  }
  ....
}

A function DSCF_AddRef () was found in the code, the return value of which is not used. Moreover, this function does not change any state in the program. This is a very suspicious place that developers need to check.

Warning V593

V593 Consider reviewing the expression of the 'A = B <C' kind. The expression is calculated as following: 'A = (B <C)'. user.c 3247

DWORD WINAPI FormatMessage16(....)
{
  ....
  int     ret;
  int     sz;
  LPSTR   b = HeapAlloc(..., sz = 100);
  argliststart=args+insertnr-1;
  /* CMF - This makes a BIG assumption about va_list */
  while ((ret = vsnprintf(....) < 0) || (ret >= sz)) {
      sz = (ret == -1 ? sz + 100 : ret + 1);
      b = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, b, sz);
  }
  ....
}

The priority of logical operations is higher than the priority of the assignment operation. Thus, in this expression, the expression “vsnprintf (....) <0” is calculated first, therefore not the number of written characters will be stored in the 'ret' variable, but the value 0 or 1. The expression “ret> = sz” will always be false , so the loop will only execute if one is written to 'ret'. Such a scenario will be possible if the vsnprintf () function executes with an error and returns a negative value.

Warning V716

V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. ordinal.c 5198

#define E_INVALIDARG _HRESULT_TYPEDEF_(0x80070057)
BOOL WINAPI SHPropertyBag_ReadLONG(....)
{
    VARIANT var;
    HRESULT hr;
    TRACE("%p %s %p\n", ppb,debugstr_w(pszPropName),pValue);
    if (!pszPropName || !ppb || !pValue)
        return E_INVALIDARG;
    V_VT(&var) = VT_I4;
    hr = IPropertyBag_Read(ppb, pszPropName, &var, NULL);
    if (SUCCEEDED(hr))
    {
        if (V_VT(&var) == VT_I4)
            *pValue = V_I4(&var);
        else
            hr = DISP_E_BADVARTYPE;
    }
    return hr;
}

In the Wine project, there are many places where the HRESULT type is converted to BOOL or just works with a variable of this type as with a Boolean value. The danger is that the HRESULT type is rather complicated and should signal whether the operation was successful, what result was returned after the operation, in case of an error - where the error occurred, the circumstances of this error, and so on.

Fortunately, developers are actively correcting such places and in the bug tracker you can find many relevant commits.

Warning V523

V523 The 'then' statement is equivalent to the 'else' statement. resource.c 661

WORD WINAPI GetDialog32Size16( LPCVOID dialog32 )
{
  ....
  p = (const DWORD *)p + 1; /* x */
  p = (const DWORD *)p + 1; /* y */
  p = (const DWORD *)p + 1; /* cx */
  p = (const DWORD *)p + 1; /* cy */
  if (dialogEx)
      p = (const DWORD *)p + 1; /* ID */
  else
      p = (const DWORD *)p + 1; /* ID */
  ....
}

The analyzer detected a condition with identical blocks of code. Perhaps the code fragment was simply copied and forgot to change.

Warning V519

V519 The 'res' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 5905, 5907. action.c 5907

static void test_publish_components(void)
{
  ....
  res = RegCreateKeyExA(....);
  res = RegSetValueExA(....);
  ok(res == ERROR_SUCCESS, "RegSetValueEx failed %d\n", res);
  RegCloseKey(key);
....
}

Testing should ensure the reliability of the application, and if mistakes are made in the tests, then the trouble. In this code fragment, they forgot to check the result of one function and immediately went on to receive and check the value of another function.

Conclusion

In response to the request for re-checking the project, we sent a fresh report from the PVS-Studio analyzer and a temporary product key for convenient viewing of the report using the plug-in for Visual Studio or the Standalone utility. Over the year, the Wine project code has become much cleaner from the point of view of our analyzer, now developers can still improve their code.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Analyzing Wine: One Year Later .