Wine Check: One Year Later

Just over a year ago, the Wine project was taken to write an article on checking a project with the help of PVS-Studio. The article was written, the authors were notified and even asked for a full report of the check by the analyzer, and received a positive response. Recently, we wrote one of the developers of the project. The article will tell you 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 Linux, Mac, FreeBSD, and Solaris users to run Windows applications without having to install Microsoft Windows on the computer itself. Wine is an actively developing cross-platform free software distributed under the GNU Lesser General Public License.



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



He also brought a small statistic to correct the analyzer warnings for the year. You can find about 180 commits with this link , containing source code fixes marked “PVS-Studio”.

')

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





Figure 1 - The top 20 successful error codes for Wine



Michael explained that it is already difficult to combine the current source code with the old report of the analyzer 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 describe the 10 most suspicious sections of code. Naturally, developers have received a full report and will be able to explore other potentially dangerous places.

Top 10 Warnings

V650 warning

V650 Type casting operation is used 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 has detected an addition operation with a variable with which a double type conversion is performed. Most likely, they forgot to conclude the first type conversion and addition operation in brackets. Above the code there is exactly the same fragment, only with parentheses:

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

V590 warning

V590 Consider inspecting the 'lret == 0 || lret! = 234 'expression. The expression is 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 'lret' variable that are not 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 misprint. schedsvc.c 90

Warning v576

V576 Incorrect format. Consider checking the fourth argument of the printf function. To print the value of 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 the pointer value is attempted to be printed using the '% d' specifier. Writing this code fragment was most likely done by copy-paste method. It can be assumed that first the first call to the printf () function was written, the last argument in which correctly corresponds to the used specifier '% d'. But then this line was copied two more times and the pointer was passed as the last argument, and the format of the line was forgotten.

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 memory access outside the 'rgbuf' array to elements with indices 16 and 17. The array itself contains only 16 elements. Perhaps the “rqbuf [15] & 0x8” condition is rarely true and such an error was not noticed.

V711 Warning

It is a variable in 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 ) { .... } .... } .... } 

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

V530 Warning

V530 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 found the function DSCF_AddRef (), 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 A = B <C 'kind. The expression is calculated as the 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 an assignment operation. Thus, in this expression, the expression “vsnprintf (....) <0” is calculated first, therefore the variable 'ret' will not save the number of written characters, but the value 0 or 1. The expression “ret> = sz” will always be false therefore, the loop will be executed only if the unit is written in 'ret'. Such a scenario will be possible if the vsnprintf () function fails and returns a negative value.

V716 Warning

V716 Suspicious type conversion in return statement: returned HRESULT, but the 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 many places in the Wine project where the HRESULT type is converted to BOOL or simply work with a variable of this type as 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 was completed, in case of an error — where the error occurred, the circumstances of this error, and so on.



Fortunately, the developers are actively fixing such places and in the bug tracker you can find a lot of related 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 the same code blocks. Perhaps the code snippet was just copied and forgot to change.

Warning v519

V519 The 'res' variable is assigned 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 errors are made in the tests, then the trouble. In this code snippet, we forgot to check the result of one function and immediately went on to get and check the value of another function.

Conclusion

In response to a request to re-check the project, we sent a fresh report from the PVS-Studio analyzer and a temporary product key for easy viewing of the report using the plug-in tools for Visual Studio or the Standalone utility. Over the year, the code of the Wine project 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 .