Проверка Wine: Год спустя

Чуть больше года назад для написания статьи о проверки проекта с помощью PVS-Studio был взят проект Wine. Статья написана, авторы были уведомлены и даже попросили полный отчёт проверки анализатором, на что получили положительный ответ. Недавно нам написал один из разработчиков проекта. В статье будет рассказано о нашем общении, проделанной работе команды разработчиков проекта Wine по улучшению кода и о том, что ещё предстоит сделать.

Введение

Wine (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. Недавно мы получили письмо от одного из разработчиков Wine — Michael Stefaniuc. В письме он поблагодарил команду PVS-Studio за использование статического анализатора и предоставление отчёта.

Также он привёл небольшую статистку по исправлению предупреждений анализатора за год. По этой ссылке можно найти около 180 коммитов, содержащих исправления исходного кода с пометкой «PVS-Studio».

На рисунке 1 представлена статистика исправления 20 самых полезных, с точки зрения авторов, типов предупреждений анализатора для проекта.

Рисунок 1 — The top 20 successful error codes for Wine

Michael пояснил, что совмещать текущий исходный код со старым отчётом анализатора уже затруднительно и попросил проверить проект ещё раз. Проект Wine активно развивается, статический анализатор PVS-Studio тоже активно развивается, поэтому я снова решил проверить этот проект. Результатом стала эта небольшая заметка, где я опишу 10 самых подозрительных участков кода. Естественно разработчики получили полный отчет и смогут изучить и прочие потенциально опасные места.

Top 10 предупреждений

Предупреждение 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;   .... }

Анализатор обнаружил операцию сложения с переменной, с которой выполняется двойное приведение типа. Скорее всего здесь забыли заключить первое приведение типа и операцию сложения в скобки. Выше по коду есть точно такой же фрагмент, только со скобками:

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

Предупреждение 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;   .... }

В коде имеется избыточное сравнение " lret == ERROR_SUCCESS". Видимо имеет место логическая ошибка. Условие истинно для всех значений переменной ‘lret’, неравных ‘ERROR_MORE_DATA’. Для наглядности можно посмотреть на таблицу истинности на рисунке 2.

Рисунок 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 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);   .... }

Анализатор обнаружил подозрительное место, в котором значение указателя пытаются распечатать с помощью спецификатора ‘%d. Написание этого фрагмента кода с большой вероятностью было выполнено методом copy-paste. Можно предположить, что сначала был написал первый вызов функции printf(), последний аргумент в которой правильно соответствует используемому спецификатору ‘%d’. Но потом эту строчку скопировали ещё два раза и в качестве последнего аргумента стали передавать указатель, а формат строки поменять забыли.

Предупреждение 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);      //<==   }   .... }

Анализатор обнаружил обращение к памяти за пределы массива ‘rgbuf’ к элементам с индексами 16 и 17. Сам массив содержит только 16 элементов. Возможно, условие «rqbuf[15]&0x8» редко является истинным и такую ошибку не заметили.

Предупреждение 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 )       {         ....       }     ....   }   .... }

В теле цикла обнаружено объявление переменной «filetime», совпадающей с переменной, используемой для контроля цикла. Это будет приводить к потере локальных изменений в «filename» при выходе из внутреннего цикла. Глядя на весь код функции можно предположить, что большой фрагмент кода был скопирован в тело цикла с незначительными изменениями. Это может и не нести серьёзной опасности, всё равно это является нехорошим стилем программирования.

Предупреждение 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++;   }   .... }

В коде найдена функция DSCF_AddRef(), возвращаемое значение которой не используется. Более того, эта функция не меняет какие-то состояния в программе. Это очень подозрительное место, которое необходимо проверить разработчикам.

Предупреждение 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);   }   .... }

Приоритет логических операций выше приоритета операции присваивания. Таким образом, в этом выражении первым вычисляется выражение «vsnprintf(….) < 0», следовательно в переменную ‘ret’ будет сохранено не количество записанных символов, а значение 0 или 1. Выражение «ret >= sz» будет всегда ложным, поэтому цикл выполнится только если в ‘ret’ запишется единица. Такой сценарий будет возможен, если функция vsnprintf() выполнится с ошибкой и вернёт отрицательное значение.

Предупреждение 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; }

В проекте Wine много мест, где тип HRESULT преобразуют в BOOL или просто работают с переменной это типа как с булевым значением. Опасность заключается в том, что тип HRESULT устроен достаточно сложно и должен сигнализировать о том, прошла ли операция успешно, какой результат был возвращён после выполнения операции, в случае ошибки — где произошла ошибка, обстоятельства этой ошибки и так далее.

К счастью, разработчики активно исправляют такие места и в баг-трекере можно найти много соответствующих коммитов.

Предупреждение 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 */   .... }

Анализатор обнаружил условие с одинаковыми блоками кода. Возможно, фрагмент кода просто скопировали и забыли изменить.

Предупреждение 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); .... }

Тестирование должно обеспечивать надёжность приложение, а если ошибки допущены в тестах, то беда. В этом фрагменте кода забыли проверить результат одной функции и сразу перешли к получению и проверке значения другой функции.

Заключение

В ответ на просьбу о повторной проверке проекта, мы отправили свежий отчёт анализатора PVS-Studio и временный ключ продукта для удобного просмотра отчёта средствами плагина для Visual Studio или утилиты Standalone. За год код проекта Wine стал значительно чище с точки зрения нашего анализатора, теперь разработчики могут ещё улучшить свой код.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing Wine: One Year Later.

Прочитали статью и есть вопрос?

Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.

ссылка на оригинал статьи http://habrahabr.ru/post/269265/