В статье хочу рассказать о проверке проекта Wine такими статическими анализаторами C/C++ кода, как PVS-Studio и Clang Static Analyzer.
Wine
Wine (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 '<<. The left operand '(LONGLONG) — 1' is negative. propvar.c 127
...
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)] = ''; /* remove last backslash */
...На самом деле ноль запишется в ноль и ничего не изменится. Для работы функции strlen() строка уже должна заканчиваться терминальным нулём. Комментарий намекает нам, что, наверное, хотели отрезать косую черту в конце. Тогда, видимо код должен быть такой:
section[strlen(section) - 1] = '';Один счётчик на два цикла
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.2xn", 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=%dn", 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.c
Location: 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 windown");
...
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.c
Location: 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.c
Location: 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.c
Location: 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.c
Location: 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) не досчитываются последнего элемента, как при инициализации, так и при использовании. В месте, где выдал сообщение Clang, как раз берётся восьмой элемент с индексом 7, так как выражение (curStream + 1 >= nStreams) будет истинно при значениях curStream==6 и nStreams==7.Обращение к массиву pInStreams[curStream + 1] даст последний неинициализированный ранее элемент.
Нулевой путь
File: dlls/crypt32/rootstore.c
Location: line 413, column 10
Description: 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.c
Location: 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.
Автор: SvyatoslavMC
