Sviatoslav Razmyslov

Oct 21 2015

Tags:

#Cpp

Analyzing Wine: One Year Later

Oct 21 2015

Author: Sviatoslav Razmyslov

Introduction
Top 10 warnings
Conclusion

One year ago, we picked Wine project to analyze with PVS-Studio and provided the analysis results in an article within the scope of our practice of analyzing open-source projects. So the article was written and the project's authors were informed about it. They even asked for a complete analysis log and we submitted it to them. Recently one of the project developers has contacted us again. In this article I will tell you about some points of our discussion, several improvements that Wine authors have done in their code and work that is yet to be done.

Introduction

Wine (Wine Is Not Emulator) is a set of programs allowing Linux, FreeBSD, and Solaris users to run Windows-applications without installing Microsoft Windows itself. Wine is an actively developing cross-platform, free and open-source software application distributed under the GNU Lesser General Public License.

In August 2014, we published an article "Checking Wine with PVS-Studio and Clang Static Analyzer". Recently we've got an email from one of the Wine developers Michael Stefaniuc, where he thanked PVS-Studio team for running the analyzer on their code and sharing the analysis report.

He also shared with us some statistics on the bug fixes, suggested by the analyzer. Here you can find 180 commits with source-code fixes labeled "PVS-Studio".

Figure 1 shows statistics on the fixes of 20 diagnostic warnings that the authors find to be most critical for their project.

Figure 1 - Top 20 successful error codes for Wine

Michael told us that trying to correlate the source code's current version with the old analysis report had become pretty hard, so he asked us to scan the project once again. Wine is actively developing; so is the PVS-Studio analyzer. So I decided to give it another run. Results of the new analysis are described in this small post, where I will tell you about 10 most suspicious code fragments in Wine. We have sent a complete log to the developers, so they can review all the rest potential issues.

Top 10 warnings

Warning V650

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 applied to a variable whose type is being cast twice. The error probably occurred because the programmer forgot to enclose the first type conversion and addition operation in parentheses. A bit earlier in the code, there is exactly the same fragment but with parentheses:

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 contains a redundant comparison "lret == ERROR_SUCCESS". It seems that we are dealing with a logical error here. The condition is true at any value of the 'lret' variable other than 'ERROR_MORE_DATA'. See the table in Figure 2.

Figure 2 - Truth table for a conditional expression

Marked red are the two columns where the results of logical operations totally coincide.

Another issue of this type:

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 code fragment where a pointer's value is attempted to be printed using the '%d specifier. This code is very likely to have been written by Copy-paste. Supposedly, the first call of the printf() function was written first and its last argument correctly matches the '%d' specifier. But then this line was copied twice more and the pointer was passed as the last argument. After all these actions, the programmer forgot to change the string formatting.

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 that the program tries to address items 16 and 17 of the 'rgbuf' array, which is beyond its bounds as it only contains 16 items. The "rqbuf[15]&0x8" condition is rarely true, that's why the error hasn't been 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 loop body contains declaration of the "filetime" variable, whose name coincides with that of the variable used to control the loop. This will lead to loss of local changes in "filename" when leaving the internal loop. The function's entire code suggests that a large code block was copied into the loop body with just slight edits. Although it may not be necessarily dangerous, it's still not a good 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++;
  }
  ....
}

The code contains DSCF_AddRef() function, whose return value is not used. Moreover, this function doesn't change any program states, which is very suspicious and should be checked by the developers.

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

Precedence of logical operations is higher than that of the assignment operation. Therefore, in this expression, the "vsnprintf(....) < 0" subexpression is evaluated first; consequently, what will be saved into the 'ret' variable is not the number of characters written, but value 0 or 1 instead. The "ret >= sz" subexpression will always be false, so the loop will execute only if 'ret' stores 1. And this 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;
}

There are plenty of spots in Wine project where the HRESULT type is cast to BOOL or a variable of this type is handled as a Boolean value. What makes it dangerous is that HRESULT is a fairly complex type designed to confirm successful operation execution and report the return result or an error's origin, its conditions, etc., should it occur.

Fortunately, the developers are actively fixing such fragments, and there are lots of corresponding commits to be found in the bug tracker.

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 branches. Seems that it's just a copy-pasted fragment that the programmer forgot to edit.

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 is meant to ensure application's reliability, so it's no good when tests themselves contain errors. In this code fragment, the programmer forgot to check the result of one function and went on to get and check the result of another.

Conclusion

In response to the project-rescan request, we've sent the Wine's authors a fresh report of the PVS-Studio analyzer and a temporary product registration key so that they can comfortably view it through the PVS-Studio plugin for Visual Studio or Standalone utility. Wine's code has become much cleaner since the last year, and now the authors will be able to improve it even more.

#Cpp