Проверяем Wine с помощью PVS-Studio и Clang Static Analyzer
В статье хочу рассказать о проверке проекта Wine такими статическими анализаторами C/C++ кода, как PVS-Studio и Clang Static Analyzer.WineWine (Wine Is Not Emulator — Wine — не эмулятор) — это набор программ, позволяющий пользователям Linux, Mac, FreeBSD, и Solaris запускать Windows-приложения без необходимости установки на компьютер самой Microsoft Windows. Wine является активно развивающимся кросс-платформенным свободным ПО, распространяемым под лицензией GNU Lesser General Public License.Исходный код проекта можно получить командой git clone git://source.winehq.org/git/wine.git
Об анализаторах PVS-Studio — статический анализатор, выявляющий ошибки в исходном коде приложений на языке C/C++/C++11. Для проверки проекта использовалась релиз-версия PVS-Studio 5.18. Clang Static Analyzer — статический анализатор, который находит ошибки в C, C++ и Objective-C программах. Для проверки проекта использовалась релиз-версия Clang 3.4.2 для openSUSE 13.1. Результаты проверки в PVS-Studio Сдвиг отрицательного числа V610 Undefined behavior. Check the shift operator ' … if (*res >= ((LONGLONG)1 << (dest_bits-1)) || *res < ((LONGLONG)-1 << (dest_bits-1))) return HRESULT_FROM_WIN32(ERROR_ARITHMETIC_OVERFLOW); ... Тип LONGLONG объявлен как 'typedef signed __int64 LONGLONG;', т.е. является знаковым типом. Сдвиги отрицательных чисел по новому стандарту приводят к неопределённому или неуточненному поведению. Почему такой код может работать и как его лучше исправить, можно прочитать в статье: Не зная брода, не лезь в воду. Часть третья.Опечатки V501 There are identical sub-expressions '!lpScaleWindowExtEx->xNum' to the left and to the right of the '||' operator. enhmetafile.c 1418 … if (! lpScaleWindowExtEx→xNum || ! lpScaleWindowExtEx→xDenom || ! lpScaleWindowExtEx→xNum || ! lpScaleWindowExtEx→yDenom) break; … В условии два раза проверяется lpScaleWindowExtEx→xNum, скорее всего в одном месте должно быть lpScaleWindowExtEx→yNum. В объявлении используемой структуры такое поле есть: typedef struct { EMR emr; LONG xNum; //<== LONG xDenom; LONG yNum; //<== LONG yDenom; } EMRSCALEVIEWPORTEXTEX, *PEMRSCALEVIEWPORTEXTEX, EMRSCALEWINDOWEXTEX, *PEMRSCALEWINDOWEXTEX; V501 There are identical sub-expressions '!(types[i + 1] & PathPointTypeBezier)' to the left and to the right of the '||' operator. graphics.c 1751 .... for(i = 1; i < count; i++){ .... if((i + 2 >= count) || !(types[i + 1] & PathPointTypeBezier) || !(types[i + 1] & PathPointTypeBezier)){ … } i += 2; } … Это место обнаружено аналогичной диагностикой V501, но причина одинаковых условий тут не так очевидна. Скорее всего, в одном условии должно быть types[i + 2], потому что выше проверили возможность обратиться к элементу массива с индексом на 2 больше, чем 'i'.V593 Consider reviewing the expression of the 'A = B!= C' kind. The expression is calculated as following: 'A = (B!= C)'. request.c 3354
if ((hr = SafeArrayAccessData (sa, (void **)&ptr)) != S_OK) return hr; if ((hr = SafeArrayGetUBound (sa, 1, &size) != S_OK)) //<== { SafeArrayUnaccessData( sa ); return hr; } Приоритет оператора != выше, чем оператора присваивания =. Причём по условию выше хорошо видно, что здесь тоже необходимо обернуть присваивание ещё в одни скобки, иначе hr получает значение 0 или 1.Ещё похожее место:
V501 There are identical sub-expressions to the left and to the right of the '|' operator: VT_ARRAY | VT_ARRAY vartest.c 2161
Каскадирование условных операторов V517 The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 1754, 1765. msi.c 1754 if (! strcmpW (szProperty, INSTALLPROPERTY_LOCALPACKAGEW)) //<== { ... } else if (!strcmpW( szProperty, INSTALLPROPERTY_INSTALLDATEW )) { ... } else if (!strcmpW( szProperty, INSTALLPROPERTY_LOCALPACKAGEW )) //<== { ... } else if (!strcmpW( szProperty, INSTALLPROPERTY_UNINSTALLABLEW ) || !strcmpW( szProperty, INSTALLPROPERTY_PATCHSTATEW ) || !strcmpW( szProperty, INSTALLPROPERTY_DISPLAYNAMEW ) || !strcmpW( szProperty, INSTALLPROPERTY_MOREINFOURLW )) { ... } else { ... } Если в каскаде условных операторов проверяются одинаковые условия, то последние не получают управление. Возможно здесь опечатка в переданной константе INSTALLPROPERTY_*.Эквивалентные ветви оператора if V523 The 'then' statement is equivalent to the 'else' statement. filedlg.c 3302 if(pDIStruct->itemID == liInfos→uSelectedItem) { ilItemImage = (HIMAGELIST) SHGetFileInfoW ( (LPCWSTR) tmpFolder→pidlItem, 0, &sfi, sizeof (sfi), shgfi_flags); } else { ilItemImage = (HIMAGELIST) SHGetFileInfoW ( (LPCWSTR) tmpFolder→pidlItem, 0, &sfi, sizeof (sfi), shgfi_flags); } Подобный код либо избыточен, либо имеет место опечатка.V523 The 'then' statement is equivalent to the 'else' statement. genres.c 1130
… if (win32) { put_word (res, 0); /* Reserved */ /* FIXME: The ResType in the NEWHEADER structure should * contain 14 according to the MS win32 doc. This is * not the case with the BRC compiler and I really doubt * the latter. Putting one here is compliant to win16 spec, * but who knows the true value? */ put_word (res, 1); /* ResType */ put_word (res, icog→nicon); for (ico = icog→iconlist; ico; ico = ico→next) { … } } else /* win16 */ { put_word (res, 0); /* Reserved */ put_word (res, 1); /* ResType */ put_word (res, icog→nicon); for (ico = icog→iconlist; ico; ico = ico→next) { … } } … Здесь одна из повторяющихся ветвей прокомментирована. Возможно анализатор нашёл место, которое недоделали. Не ошибка, но решил отметить.Изменение длины строки V692 An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. appdefaults.c 390 … section[strlen (section)] = '\0'; /* remove last backslash */ … На самом деле ноль запишется в ноль и ничего не изменится. Для работы функции strlen () строка уже должна заканчиваться терминальным нулём. Комментарий намекает нам, что, наверное, хотели отрезать косую черту в конце. Тогда, видимо код должен быть такой: section[strlen (section) — 1] = '\0'; Один счётчик на два цикла V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 980, 1003. iphlpapi_main.c 1003 … for (i = 0; i < num_v6addrs; i++) //<== { ... for (i = 0; i < 8 && !done; i++) //<== { ... } ... if (i < num_v6addrs - 1) { prefix->Next = (IP_ADAPTER_PREFIX *)ptr; prefix = prefix→Next; } } … Подозрительное место: вложенный цикл организован с использованием переменной i, которая также используется и во внешнем цикле.Двойное приведение типов В следующих двух примерах к указателю *void применяется два приведения к типу: сначала к char*, потом к DWORD*, после чего прибавляется смещение. В выражении не хватает скобок, либо код избыточен. Есть ли тут ошибка, зависит от того, на сколько хотели увеличить значение указателя.V650 Type casting operation is utilized 2 times in succession. Next, the '+' operation is executed. Probably meant: (T1)((T2)a + b). typelib.c 9147
… struct WMSFT_SegContents arraydesc_seg; typedef struct tagWMSFT_SegContents { DWORD len; void *data; } WMSFT_SegContents; … DWORD offs = file→arraydesc_seg.len; DWORD *encoded; encoded = (DWORD*)((char*)file→arraydesc_seg.data) + offs;//<== Ещё одна схожая ситуация:V650 Type casting operation is utilized 2 times in succession. Next, the '+' operation is executed. Probably meant: (T1)((T2)a + b). protocol.c 194
INT WINAPI EnumProtocolsW (LPINT protocols, LPVOID buffer, LPDWORD buflen) { … unsigned int string_offset; … pi[i].lpProtocol = (WCHAR*)(char*)buffer + string_offset;//<== ... } Разность беззнаковых чисел V555 The expression 'This->nStreams — nr > 0' will work as 'This→nStreams!= nr'. editstream.c 172 static HRESULT AVIFILE_RemoveStream (IAVIEditStreamImpl* const This, DWORD nr) { … This→nStreams--; if (This→nStreams — nr > 0) { //<== memmove(This->pStreams + nr, This→pStreams + nr + 1, (This→nStreams — nr) * sizeof (EditStreamTable)); } … } Переменная nr имеет баззнаковый тип DWORD. Вычитая её, результат разности также будет иметь беззнаковый тип. Если nr будет больше This→nStreams — условие всё равно будет истинным.Аналогичное место:
V555 The expression 'This→fInfo.dwStreams — nStream > 0' will work as 'This→fInfo.dwStreams!= nStream'. avifile.c 469
Сначала казнь, потом обед V595 The 'decl' pointer was utilized before it was verified against nullptr. Check lines: 1411, 1417. parser.y 1411 … var_t *v = decl→var; //<== expr_list_t *sizes = get_attrp(attrs, ATTR_SIZEIS); expr_list_t *lengs = get_attrp(attrs, ATTR_LENGTHIS); int sizeless; expr_t *dim; type_t **ptype; array_dims_t *arr = decl ? decl->array: NULL; //<== type_t *func_type = decl ? decl->func_type: NULL; //<== ... Сначала брали значение по указателю, потом решили проверять.Аналогичные места:
V595 The 'pcbData' pointer was utilized before it was verified against nullptr. Check lines: 1859, 1862. registry.c 1859 V595 The 'token_user' pointer was utilized before it was verified against nullptr. Check lines: 206, 213. lsa.c 206 V595 The 'psp' pointer was utilized before it was verified against nullptr. Check lines: 2680, 2689. propsheet.c 2680 V595 The 'lpFindInfo' pointer was utilized before it was verified against nullptr. Check lines: 6285, 6289. listview.c 6285 V595 The 'compiland' pointer was utilized before it was verified against nullptr. Check lines: 287, 294. symbol.c 287 V595 The 'graphics' pointer was utilized before it was verified against nullptr. Check lines: 2096, 2112. graphics.c 2096 V595 The 'current' pointer was utilized before it was verified against nullptr. Check lines: 240, 251. request.c 240 Печать результата одинаковых функций V681 The language standard does not define an order in which the 'tlb_read_byte' functions will be called during evaluation of arguments. tlb.c 650 … printf (»\\%2.2x \\%2.2x\n», tlb_read_byte (), tlb_read_byte ()); … Согласно стандарту языка Си++, не определена последовательность вычисления фактических аргументов функции. Какая функция будет вызвана первой, зависит от компилятора, параметров компиляции и так далее.Сомнительные тесты В директориях некоторых модулей имеется каталог test с исходными файлами для тестов. Для вывода отладочной информации используется макрос 'ok'. Вот несколько подозрительных мест: V501 There are identical sub-expressions to the left and to the right of the '==' operator: ddsd3.lpSurface == ddsd3.lpSurface dsurface.c 272
… ok (ddsd3.lpSurface == ddsd3.lpSurface, //<== "lpSurface from GetSurfaceDesc(%p) differs\ from the one returned by Lock(%p)\n", ddsd3.lpSurface, ddsd2.lpSurface); //<== ... Очень похоже на опечатку. Скорее всего здесь должны сравниваться те же переменные, которые выводятся на печать.V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. url.c 767
… ok (size == no_callback? 512: 13, «size=%d\n», size); … Приоритет оператора »==» выше '?:', поэтому здесь переменная size не сравнивается со значениями 512 и 13. Выражение всегда истинно, так как будет равно 512 или 13 и значит проверка ничего не проверят.Аналогичные предупреждения:
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. string.c 1086 V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. string.c 1111 V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. reader.c 761 V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. protocol.c 2928 V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. dde.c 1594 V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. reader.c 761 Результаты проверки в Clang Static Analyzer Поиск потенциальных ошибок этим анализатором осуществляется путём обхода возможных сценариев выполнения программы. Если подозрительное место найдено, то анализатор создаст отчёт для этого файла в формате HTML (по умолчанию) или PLIST, в котором будут прокомментированы от одного до нескольких десятков шагов, приводящих к подозрительному участку кода.Большинство сообщений, полученных в ходе проверки проекта Wine, имеют один и тот же характер: объявляется переменная без инициализации; в функции, в которую передаётся адрес переменной, пропущена инициализация в некоторых ветвях оператора switch, либо выполнен 'return' до инициализации. Подобные ситуации в дальнейшем не обрабатываются и неинициализированная переменная продолжает использоваться. Счёт таких мест в проекте идёт на сотни, поэтому в данном обзоре рассмотрены не будут. Что-то из них, думаю, может представлять настоящие серьезные ошибки. Оставим это на совести разработчиков.
Неинициализированная переменная в условии File: dlls/atl110/…/atl/atl_ax.cLocation: line 1092, column 10
Description: Branch condition evaluates to a garbage value
HRESULT WINAPI AtlAxCreateControlEx (LPCOLESTR lpszName, HWND hWnd, IStream *pStream, IUnknown **ppUnkContainer, IUnknown **ppUnkControl, REFIID iidSink, IUnknown *punkSink) { … IUnknown *pContainer; … hRes = AtlAxAttachControl (pUnkControl, hWnd, &pContainer); if (FAILED (hRes)) WARN («cannot attach control to window\n»); … if (pContainer) //<== //Clang: Branch condition evaluates to a garbage value IUnknown_Release( pContainer ); return S_OK; } Неинициализированная переменная pContainer используется в условии после вызова AtlAxAttachControl. Описание этой функции представлено ниже. HRESULT WINAPI AtlAxAttachControl(IUnknown *control, HWND hWnd, IUnknown **container) { HRESULT hr; ... if (!control) return E_INVALIDARG;//<== hr = IOCS_Create( hWnd, control, container ); return hWnd ? hr : S_FALSE; } Здесь может быть выполнен возврат значения E_INVALIDARG до инициализации переменной container. В результате, функция AtlAxCreateControlEx выдаст предупреждение и продолжит работать с неинициализированной переменной.Возможное переполнение буфера File: tools/widl/typegen.cLocation: line 1158, column 28
Description: String copy function overflows destination buffer
static unsigned int write_new_procformatstring_type (…) { char buffer[64]; … strcpy (buffer,»/* flags:»); if (flags & MustSize) strcat (buffer,» must size,»); if (flags & MustFree) strcat (buffer,» must free,»); if (flags & IsPipe) strcat (buffer,» pipe,»); if (flags & IsIn) strcat (buffer,» in,»); if (flags & IsOut) strcat (buffer,» out,»); if (flags & IsReturn) strcat (buffer,» return,»); if (flags & IsBasetype) strcat (buffer,» base type,»); if (flags & IsByValue) strcat (buffer,» by value,»); if (flags & IsSimpleRef) strcat (buffer,» simple ref,»); … } При выполнении даже не всех условий, есть вероятность получить слишком длинную строку, которая не поместится в буфер.Потенциальная утечка памяти File: libs/wpp/ppl.yy.cLocation: line 4475, column 1
Description: Potential memory leak
static void macro_add_arg (int last) { … if (last || mep→args[mep→nargs-1][0]) { yy_push_state (pp_macexp); push_buffer (NULL, NULL, NULL, last? 2: 1); ppy__scan_string (mep→args[mep→nargs-1]); //Clang: Calling 'ppy__scan_string' //Clang: Returned allocated memory } //Clang: Potential memory leak } Функция pyy__scan_string имеет возвращаемое значение, которое не используется. Вызов этой функции так или иначе приводит к возврату значения функцией malloc (), после использования которой необходимо освобождать память.Давайте рассмотрим, как вызов функции pyy__scan_string приводит к вызову malloc.
YY_BUFFER_STATE ppy__scan_string (yyconst char * yystr) { return ppy__scan_bytes (yystr, strlen (yystr)); }
YY_BUFFER_STATE ppy__scan_bytes (yyconst char * yybytes, yy_size_t _yybytes_len) { YY_BUFFER_STATE b; char *buf; … buf = (char *) ppy_alloc (n); … b = ppy__scan_buffer (buf, n); … return b; }
YY_BUFFER_STATE ppy__scan_buffer (char * base, yy_size_t size) { YY_BUFFER_STATE b; … b=(YY_BUFFER_STATE) ppy_alloc (sizeof (struct yy_buffer_state)); … return b; }
void *ppy_alloc (yy_size_t size) { return (void *) malloc (size); } Деление на ноль File: dlls/winex11.drv/palette.cLocation: line 601, column 43
Description: Division by zero
#define NB_RESERVED_COLORS 20 … static void X11DRV_PALETTE_FillDefaultColors (…) { … int i = 0, idx = 0; int red, no_r, inc_r; … if (palette_size <= NB_RESERVED_COLORS) return; while (i*i*i < (palette_size - NB_RESERVED_COLORS)) i++; no_r = no_g = no_b = --i; ... inc_r = (255 - NB_COLORCUBE_START_INDEX)/no_r; //Clang: Division by zero ... } Код продолжит выполняться если значение переменной palette_size равно 21 и более. При значении 21, переменная 'i' в начале будет увеличена на единицу, а затем уменьшена на единицу. В результате 'i' останется равна нулю, что и приведёт к делению на ноль.Неинициализированный элемент массива File: dlls/avifil32/api.cLocation: line 1753, column 10
Description: Assigned value is garbage or undefined
#define MAX_AVISTREAMS 8 … HRESULT WINAPI AVISaveVW (…int nStreams …) { … //Объявление массива из 8 элементов, [0…7] PAVISTREAM pInStreams[MAX_AVISTREAMS]; … if (nStreams >= MAX_AVISTREAMS) { WARN (…); return AVIERR_INTERNAL; } … //Инициализация первых семи элементов, [0…6]. for (curStream = 0; curStream < nStreams; curStream++) { pInStreams[curStream] = NULL; pOutStreams[curStream] = NULL; } ... for (curStream = 0; curStream < nStreams; curStream++) { ... if (curStream + 1 >= nStreams) { /* move the others one up */ PAVISTREAM *ppas = &pInStreams[curStream]; int n = nStreams — (curStream + 1);
do { *ppas = pInStreams[curStream + 1]; //Clang: Assigned value is garbage or undefined } while (--n); } … } … } Здесь объявляется массив из 8 элементов. Код продолжит выполняться если значение переменной nStreams меньше 8, т.е. максимум 7. Все циклы в этой функции, с условным выражением (curStream = nStreams) будет истинно при значениях curStream==6 и nStreams==7.Обращение к массиву pInStreams[curStream + 1] даст последний неинициализированный ранее элемент.Нулевой путь File: dlls/crypt32/rootstore.cLocation: line 413, column 10Description: Null pointer passed as an argument to a 'nonnull' parameter static BOOL import_certs_from_path (LPCSTR path, HCERTSTORE store, BOOL allow_dir) { … fd = open (path, O_RDONLY); //Clang: Null pointer passed as //an argument to a 'nonnull' parameter … } Чтобы понять, почему Clang считает, что сюда может прийти NULL, рассмотрим место, где вызывается эта функция: static BOOL import_certs_from_dir (LPCSTR path, HCERTSTORE store) { … char *filebuf = NULL; //Clang: 'filebuf' initialized to a null pointer value struct dirent *entry; while ((entry = readdir (dir))) { … size_t name_len = strlen (entry→d_name);
//Вызов функции для изменения filebuf
if (! check_buffer_resize (&filebuf, &bufsize, path_len + 1 + name_len + 1)) { ERR (…); break; } snprintf (filebuf, bufsize,»%s/%s», path, entry→d_name); if (import_certs_from_path (filebuf, store, FALSE) && ! ret) //Clang: Passing null pointer value via 1st parameter 'path' //Clang: Calling 'import_certs_from_path' ret = TRUE; … } } Здесь зовётся функция check_buffer_resize, в которой должно изменяться значение переменной filebuf или возвращаться FALSE, но функция может не изменить filebuf и вернуть TRUE, рассмотрим код функции ниже: static BOOL check_buffer_resize (char **ptr_buf, size_t *buf_size, size_t check_size) { if (check_size > *buf_size) { … *ptr_buf = CryptMemAlloc (*buf_size); … } return TRUE; } Функция содержит только одно условие, в котором изменяется переменная ptr_buf и в случае невыполнения условия, истинный результат возврата позволяет в дальнейшем использовать эту переменную.Похожая ситуация с функцией memcpy ():
File: server/directory.c
Location: line 548, column 21
Description: Null pointer passed as an argument to a 'nonnull' parameter
Ненадёжная проверка File: dlls/advapi32/registry.cLocation: line 1209, column 13
Description: Array access (from variable 'str') results in a null pointer dereference
LSTATUS WINAPI RegSetValueExW (…, const BYTE *data, …) { … if (data && ((ULONG_PTR)data >> 16) == 0) //Assuming pointer value is null return ERROR_NOACCESS;
if (count && is_string (type)) { LPCWSTR str = (LPCWSTR)data; //Clang: 'str' initialized to a null pointer value if (str[count / sizeof (WCHAR) — 1] && ! str[count / sizeof (WCHAR)]) //Clang: Array access (from variable 'str') results in //a null pointer dereference count += sizeof (WCHAR); } … } Если придёт нулевой указатель data, то программа продолжит выполняться до обращения к переменной str.Похожее место:
File: dlls/comctl32/comctl32undoc.c
Location: line 964, column 12
Description: Array access (from variable 'lpDest') results in a null pointer dereference
Заключение Сравниваемые анализаторы PVS-Studio и Clang Static Analyzer применяют разную методологию анализа кода, следовательно, были получены различные, но неплохие результаты для обоих анализаторов.Стоит отметить, что Clang Static Analyzer отличился отсутствием разнообразия в диагностиках. По сути он предупреждает везде о том, что переменная имеет некорректное значение (нулевой указатель, ноль, неинициализированная переменная и так далее). В зависимости от значения переменной и как она используется, и формируется диагностическое сообщение. PVS-Studio более разнообразен в типах диагностик и хорошо умеет выявлять различные опечатки.
Я конечно же мог что-то пропустить в отчетах. Поэтому просмотр отчётов любых анализаторов самими авторами исходного кода даст лучший результат.
Эта статья на английском Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking Wine with PVS-Studio and Clang Static Analyzer.
