# Проверка Wine: Год спустя Чуть больше года назад для написания статьи о проверки проекта с помощью PVS\-Studio был взят проект Wine\. Статья написана, авторы были уведомлены и даже попросили полный отчёт проверки анализатором, на что получили положительный ответ\. Недавно нам написал один из разработчиков проекта\. В статье будет рассказано о нашем общении, проделанной работе команды разработчиков проекта Wine по улучшению кода и о том, что ещё предстоит сделать\. ![0352_Wine_ru/image1.png](https://import.viva64.com/docx/blog/0352_Wine_ru/image1.png) ## Введение [Wine](https://www.winehq.org/) \(Wine Is Not Emulator \-\- Wine \- не эмулятор\) \- это набор программ, позволяющий пользователям Linux, Mac, FreeBSD, и Solaris запускать Windows\-приложения без необходимости установки на компьютер самой Microsoft Windows\. Wine является активно развивающимся кросс\-платформенным свободным ПО, распространяемым под лицензией GNU Lesser General Public License\. В августе 2014 года была опубликована статья: [Проверяем Wine с помощью PVS\-Studio и Clang Static Analyzer](https://pvs-studio.ru/ru/blog/posts/cpp/0272/)\. Недавно мы получили письмо от одного из разработчиков Wine \- Michael Stefaniuc\. В письме он поблагодарил команду PVS\-Studio за использование статического анализатора и предоставление отчёта\. Также он привёл небольшую статистку по исправлению предупреждений анализатора за год\. По [этой ссылке](http://source.winehq.org/git/wine.git/search/HEAD?pg=0;s=PVS-Studio;st=commit) можно найти около 180 коммитов, содержащих исправления исходного кода с пометкой "PVS\-Studio"\. На рисунке 1 представлена статистика исправления 20 самых полезных, с точки зрения авторов, типов предупреждений анализатора для проекта\. ![0352_Wine_ru/image2.png](https://import.viva64.com/docx/blog/0352_Wine_ru/image2.png) Рисунок 1 \- The top 20 successful error codes for Wine Michael пояснил, что совмещать текущий исходный код со старым отчётом анализатора уже затруднительно и попросил проверить проект ещё раз\. Проект Wine активно развивается, статический анализатор PVS\-Studio тоже активно развивается, поэтому я снова решил проверить этот проект\. Результатом стала эта небольшая заметка, где я опишу 10 самых подозрительных участков кода\. Естественно разработчики получили полный отчет и смогут изучить и прочие потенциально опасные места\. ## Top 10 предупреждений ### Предупреждение V650 [V650](https://pvs-studio.ru/ru/docs/warnings/v650/) Type casting operation is utilized 2 times in succession\. Next, the '\+' operation is executed\. Probably meant: \(T1\)\(\(T2\)a \+ b\)\. descriptor\.c 967 ```cpp WINE_HIDP_PREPARSED_DATA* build_PreparseData(....) { .... wine_report = (WINE_HID_REPORT*)((BYTE*)wine_report)+wine_report->dwSize; .... } ``` Анализатор обнаружил операцию сложения с переменной, с которой выполняется двойное приведение типа\. Скорее всего здесь забыли заключить первое приведение типа и операцию сложения в скобки\. Выше по коду есть точно такой же фрагмент, только со скобками: ```cpp wine_report = (WINE_HID_REPORT*)(((BYTE*)wine_report)+wine_report->dwSize); ``` ### Предупреждение V590 [V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting the 'lret \=\= 0 \|\| lret \!\= 234' expression\. The expression is excessive or contains a misprint\. winemenubuilder\.c 3430 ```cpp static void cleanup_menus(void) { ... while (1) { .... lret = RegEnumValueW(....); if (lret == ERROR_SUCCESS || lret != ERROR_MORE_DATA) break; .... } ``` В коде имеется избыточное сравнение " lret \=\= ERROR\_SUCCESS"\. Видимо имеет место логическая ошибка\. Условие истинно для всех значений переменной 'lret', неравных 'ERROR\_MORE\_DATA'\. Для наглядности можно посмотреть на таблицу истинности на рисунке 2\. ![0352_Wine_ru/image3.png](https://import.viva64.com/docx/blog/0352_Wine_ru/image3.png) Рисунок 2 \- Таблица истинности условного выражения Красным выделены два столбца, где результаты логических операций полностью совпадают\. Ещё одно такое место: * V590 Consider inspecting the 'last\_error \=\= 183 \|\| last\_error \!\= 3' expression\. The expression is excessive or contains a misprint\. schedsvc\.c 90 ### Предупреждение V576 [V576](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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); .... } ``` Анализатор обнаружил подозрительное место, в котором значение указателя пытаются распечатать с помощью спецификатора '%d\. Написание этого фрагмента кода с большой вероятностью было выполнено методом copy\-paste\. Можно предположить, что сначала был написал первый вызов функции printf\(\), последний аргумент в которой правильно соответствует используемому спецификатору '%d'\. Но потом эту строчку скопировали ещё два раза и в качестве последнего аргумента стали передавать указатель, а формат строки поменять забыли\. ### Предупреждение V557 [V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The '16' index is pointing beyond array bound\. winaspi32\.c 232 ```cpp /* 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); // <= } .... } ``` Анализатор обнаружил обращение к памяти за пределы массива 'rgbuf' к элементам с индексами 16 и 17\. Сам массив содержит только 16 элементов\. Возможно, условие "rqbuf\[15\]&0x8" редко является истинным и такую ошибку не заметили\. ### Предупреждение V711 [V711](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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 ) { .... } .... } .... } ``` В теле цикла обнаружено объявление переменной "filetime", совпадающей с переменной, используемой для контроля цикла\. Это будет приводить к потере локальных изменений в "filename" при выходе из внутреннего цикла\. Глядя на весь код функции можно предположить, что большой фрагмент кода был скопирован в тело цикла с незначительными изменениями\. Это может и не нести серьёзной опасности, всё равно это является нехорошим стилем программирования\. ### Предупреждение V530 [V530](https://pvs-studio.ru/ru/docs/warnings/v530/) The return value of function 'DSCF\_AddRef' is required to be utilized\. dsound\_main\.c 760 ```cpp 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++; } .... } ``` В коде найдена функция DSCF\_AddRef\(\), возвращаемое значение которой не используется\. Более того, эта функция не меняет какие\-то состояния в программе\. Это очень подозрительное место, которое необходимо проверить разработчикам\. ### Предупреждение V593 [V593](https://pvs-studio.ru/ru/docs/warnings/v593/) Consider reviewing the expression of the 'A \= B < C' kind\. The expression is calculated as following: 'A \= \(B < C\)'\. user\.c 3247 ```cpp 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); } .... } ``` Приоритет логических операций выше приоритета операции присваивания\. Таким образом, в этом выражении первым вычисляется выражение "vsnprintf\(\.\.\.\.\) < 0", следовательно в переменную 'ret' будет сохранено не количество записанных символов, а значение 0 или 1\. Выражение "ret \>\= sz" будет всегда ложным, поэтому цикл выполнится только если в 'ret' запишется единица\. Такой сценарий будет возможен, если функция vsnprintf\(\) выполнится с ошибкой и вернёт отрицательное значение\. ### Предупреждение V716 [V716](https://pvs-studio.ru/ru/docs/warnings/v716/) Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL\. ordinal\.c 5198 ```cpp #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; } ``` В проекте Wine много мест, где тип HRESULT преобразуют в BOOL или просто работают с переменной это типа как с булевым значением\. Опасность заключается в том, что тип HRESULT устроен достаточно сложно и должен сигнализировать о том, прошла ли операция успешно, какой результат был возвращён после выполнения операции, в случае ошибки \- где произошла ошибка, обстоятельства этой ошибки и так далее\. К счастью, разработчики активно исправляют такие места и в баг\-трекере можно найти много соответствующих коммитов\. ### Предупреждение V523 [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. resource\.c 661 ```cpp 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 */ .... } ``` Анализатор обнаружил условие с одинаковыми блоками кода\. Возможно, фрагмент кода просто скопировали и забыли изменить\. ### Предупреждение V519 [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'res' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 5905, 5907\. action\.c 5907 ```cpp static void test_publish_components(void) { .... res = RegCreateKeyExA(....); res = RegSetValueExA(....); ok(res == ERROR_SUCCESS, "RegSetValueEx failed %d\n", res); RegCloseKey(key); .... } ``` Тестирование должно обеспечивать надёжность приложения, а если ошибки допущены в тестах, то беда\. В этом фрагменте кода забыли проверить результат одной функции и сразу перешли к получению и проверке значения другой функции\. ## Заключение В ответ на просьбу о повторной проверке проекта, мы отправили свежий отчёт анализатора [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) и временный ключ продукта для удобного просмотра отчёта средствами плагина для Visual Studio или утилиты Standalone\. За год код проекта Wine стал значительно чище с точки зрения нашего анализатора, теперь разработчики могут ещё улучшить свой код\.