Перевірка 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 попереджень
Попередження V650V650 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);

Попередження V590V590 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
Попередження V576V576 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'. Але потім цю сходинку скопіювали ще два рази і в якості останнього аргументу стали передавати покажчик, а формат рядки поміняти забули.

Попередження V557V557 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» рідко є істинним і таку помилку не помітили.

Попередження V711V711 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» при виході з внутрішнього циклу. Дивлячись на весь код функції можна припустити, що великий фрагмент коду був скопійований в тіло циклу з незначними змінами. Це може і не нести серйозної небезпеки, все одно це є поганим стилем програмування.

Попередження V530V530 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(), що повертає значення якого не використовується. Більше того, ця функція не змінює якісь стану в програмі. Це дуже підозріле місце, яке необхідно перевірити розробникам.

Попередження V593V593 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() виконається з помилкою і поверне від'ємне значення.

Попередження V716V716 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 влаштований досить складно і повинен сигналізувати про те, чи пройшла операція успішно, який результат був повернутий після виконання операції, у разі помилки — де сталася помилка, обставини цієї помилки і так далі.

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

Попередження V523V523 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 */
....
}

Аналізатор виявив умова з однаковими блоками коду. Можливо, фрагмент коду просто скопіювали і забули змінити.

Попередження V519V519 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. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.