Проект Miranda NG получает приз «дикие указатели» (часть первая)
Я добрался до проекта Miranda NG и проверил его с помощью анализатора кода PVS-Studio. К сожалению, с точки зрения работы с памятью и указателями это самый неаккуратный проект из виданных мной. Хотя я внимательно не анализировал результаты, ошибок столь много, что я решил разбить собранный материал на 2 статьи. Первая статья будет посвящена указателям, а вторая всему остальному. Желаю приятного чтения, и не забудьте взять попкорн.О проверке Miranda NGПроект Miranda NG — это преемник популярного мультипротокольного IM-клиента для Windows, Miranda IM.Вообще, изначально я не собирался проверять Miranda NG. Нам просто нужно несколько активно развивающихся проектов, чтобы тестировать новую фичу PVS-Studio. Можно использовать специальную базу, в которой хранится информация о сообщениях, которые не надо выдавать. Подробнее про это рассказано здесь. Идея в следующем. Иногда сложно внедрить статический анализ в большой проект, так как выдаётся слишком много предупреждений и непонятно, что с ними делать и кому это поручить. Но хочется начать получать пользу от статического анализа уже сейчас. Поэтому для начала можно скрыть все предупреждения и рассматривать только новые, которые появляются в процессе написания нового кода или рефакторинга. А уж потом, когда есть силы и желание, можно потихоньку править ошибки в старом коде.
Одним из активно развивающихся проектов оказался Miranda NG. Но когда я посмотрел, что выдал PVS-Studio при первом запуске, я понял, что у меня есть материал на новую статью.
Итак, давайте посмотрим, что нашел статический анализатор кода PVS-Studio в исходных кодах Miranda NG.
Для проверки Miranda NG из репозитория был взят Trunk. Хочу отметить, что я смотрел отчёт анализатора весьма поверхностно и наверняка многое упустил. Я пробежался только по диагностикам общего назначения 1 и 2-ого уровня. Третий уровень я даже не стал смотреть. Мне и первых двух хватило с избытком.
Часть первая. Указатели и работа с памятью Разыменовывание нулевого указателя void CMLan: OnRecvPacket (u_char* mes, int len, in_addr from) { … TContact* cont = m_pRootContact; … if (! cont) RequestStatus (true, cont→m_addr.S_un.S_addr); … } Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'cont' might take place. EmLanProto mlan.cpp 342Здесь все просто. Раз указатель равен NULL, то давайте его разыменуем и посмотрим, что весёлого из этого получится.
В начале используем указатель, потом проверяем Таких ошибок в Mirannda NG тьма тьмущая, как и в любом приложении. Обычно такой код работает, так как в функцию приходит ненулевой указатель. А вот если нулевой, то функции к этому не готовы.Типичный пример:
void TSAPI BB_InitDlgButtons (TWindowData *dat) { … HWND hdlg = dat→hwnd; … if (dat == 0 || hdlg == 0) { return; } … } Предупреждение PVS-Studio: V595 The 'dat' pointer was utilized before it was verified against nullptr. Check lines: 428, 430. TabSRMM buttonsbar.cpp 428Если в функцию BB_InitDlgButtons () передать NULL, то проверка будет сделана слишком поздно. Анализатор выдал на код проекта Mirannda NG ещё 164 таких предупреждения. Нет смысла приводить их в статье. Вот все они списком: MirandaNG-595.txt.
Потенциально неинициализированный указатель BSTR IEView: getHrefFromAnchor (IHTMLElement *element) { … if (SUCCEEDED (…) { VARIANT variant; BSTR url; if (SUCCEEDED (element→getAttribute (L«href», 2, &variant) && variant.vt == VT_BSTR)) { url = mir_tstrdup (variant.bstrVal); SysFreeString (variant.bstrVal); } pAnchor→Release (); return url; } … } Предупреждение PVS-Studio: V614 Potentially uninitialized pointer 'url' used. IEView ieview.cpp 1117Если условие if (SUCCEEDED (…)) не выполняется, то переменная 'url' остаётся неинициализированной и функция должна вернуть непонятно что. Но не всё так просто. Код содержит ещё одну ошибку. Неправильно поставлена закрывающаяся скобка. В результате макрос SUCCEEDED применяется к выражению типа 'bool', что не имеет смысла.
Второй баг компенсирует первый. Посмотрим, что такое макрос SUCCEEDED:
#define SUCCEEDED (hr) (((HRESULT)(hr)) >= 0) Выражение типа 'bool' равно 0 или 1. В свою очередь, 0 или 1 всегда >= 0. Получается, что макрос SUCCEEDED всегда возвращает истину и переменная 'url' всегда инициализируется.Только что мы воочию увидели красивый пример, когда один баг компенсирует другой. Если исправить условие, проявит баг с неинициализированной переменной.
Если иcправить 2 ошибки, то код будет выглядеть так:
BSTR url = NULL; if (SUCCEEDED (element→getAttribute (L«href», 2, &variant)) && variant.vt == VT_BSTR) Анализатор подозревает неладное ещё в 20 местах. Вот они: MirandaNG-614.txt.Путаница между размером массива и количеством элементов Количество элементов в массиве и размер массива в байтах — две разные сущности. Однако, если быть недостаточно внимательным, их вполне можно перепутать. Проект Mirannda NG демонстрирует множество способов, как может выглядеть такая путаница.Больше всего вреда нанёс макрос SIZEOF:
#define SIZEOF (X) (sizeof (X)/sizeof (X[0])) Этот макрос вычисляет количество элементов в массиве. Но, видимо, кто-то из программистов считает, что это аналог оператора sizeof (). Правда, не понятно, зачем он тогда использует макрос, а не стандартный sizeof (). Так что есть и другой вариант — кто-то не знает, как использовать функцию memcpy ().Типичный случай:
int CheckForDuplicate (MCONTACT contact_list[], MCONTACT lparam) { MCONTACT s_list[255] = { 0 }; memcpy (s_list, contact_list, SIZEOF (s_list)); for (int i = 0;; i++) { if (s_list[i] == lparam) return i; if (s_list[i] == 0) return -1; } return 0; } Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to underflow of the buffer 's_list'. Sessions utils.cpp 288Функция memcpy () скопирует только часть массива, так как третий аргумент задаёт размер массива в байтах.
Точно так же неправильно макрос SIZEOF () используется ещё в 8 местах: MirandaNG-512–1.txt.
Следующая беда. Часто программисты забывают поправить вызовы memset ()/memcpy () когда внедряют в программу Unicode:
void checkthread (void*) { … WCHAR msgFrom[512]; WCHAR msgSubject[512]; ZeroMemory (msgFrom,512); ZeroMemory (msgSubject,512); … } Предупреждения PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'msgFrom'. LotusNotify lotusnotify.cpp 760 V512 A call of the 'memset' function will lead to underflow of the buffer 'msgSubject'. LotusNotify lotusnotify.cpp 761 Функция ZeroMemoty () очистит только половину буфера, так как символы типа WCHAR занимают 2 байта.А вот пример частичного копирования строки:
INT_PTR CALLBACK DlgProcMessage (…) { … CopyMemory (tr.lpstrText, _T («mailto:»), 7); … } Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to underflow of the buffer 'L«mailto:»'. TabSRMM msgdialog.cpp 2085Будет скопирована только часть строки. Каждый символ строки занимает 2 байта. Значит скопировать надо 14 байт, а не 7.
Аналогично:
userdetails.cpp 206 weather_conv.cpp 476 dirent.c 138 Следующая ошибка допущена просто по невнимательности: #define MSGDLGFONTCOUNT 22
LOGFONTA logfonts[MSGDLGFONTCOUNT + 2];
void TSAPI CacheLogFonts () { int i; HDC hdc = GetDC (NULL); logPixelSY = GetDeviceCaps (hdc, LOGPIXELSY); ReleaseDC (NULL, hdc);
ZeroMemory (logfonts, sizeof (LOGFONTA) * MSGDLGFONTCOUNT + 2); … } Предупреждение PVS_Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'logfonts'. TabSRMM msglog.cpp 134Кто-то поспешил и смешал в кучу размер объекта и их количество. Прибавлять двойку нужно до умножения. Корректный код:
ZeroMemory (logfonts, sizeof (LOGFONTA) * (MSGDLGFONTCOUNT + 2)); В следующем примере хотели, как лучше, использовали sizeof (), но вновь запутались с размерами. Получили значение больше, чем нужно. BOOL HandleLinkClick (…) { … MoveMemory (tr.lpstrText + sizeof (TCHAR)* 7, tr.lpstrText, sizeof (TCHAR)*(tr.chrg.cpMax — tr.chrg.cpMin + 1)); … } Предупреждение PVS-Studio: V620 It’s unusual that the expression of sizeof (T)*N kind is being summed with the pointer to T type. Scriver input.cpp 387Переменная 'tr.lpstrText' указывает на строку, состоящую из символов типа wchat_t. Если хочется пропустить 7 символов, то нужно просто прибавить 7. Не нужно умножать на sizeof (wchar_t).
Такая же ошибка здесь: ctrl_edit.cpp 351
К сожалению, это ещё не конец. Ещё можно ошибиться так:
INT_PTR CALLBACK DlgProcThemeOptions (…) { … str = (TCHAR *)malloc (MAX_PATH+1); … } Предупреждение PVS-Studio: V641 The size of the allocated memory buffer is not a multiple of the element size. KeyboardNotify options.cpp 718Забыли умножить на sizeof (TCHAR). В этом же файле можно увидеть ещё 2 ошибки в строках 819 и 1076.
И последний фрагмент кода на тему количества элементов:
void createProcessList (void) { … ProcessList.szFileName[i] = (TCHAR *)malloc (wcslen (dbv.ptszVal) + 1);
if (ProcessList.szFileName[i]) wcscpy (ProcessList.szFileName[i], dbv.ptszVal); … } Предупреждения PVS-Studio: V635 Consider inspecting the expression. The length should probably be multiplied by the sizeof (wchar_t). KeyboardNotify main.cpp 543Ещё стоит дописать умножение на sizeof (TCHAR) здесь: options.cpp 1177, options.cpp 1204.
С размерами разобрались, перейдём к другим способам выстрелить себе в ногу указателем.
Выход за границу массива INT_PTR CALLBACK DlgProcFiles (…) { … char fn[6], tmp[MAX_PATH]; … SetDlgItemTextA (hwnd, IDC_WWW_TIMER, _itoa (db_get_w (NULL, MODNAME, strcat (fn,»_timer»), 60), tmp, 10)); … } V512 A call of the 'strcat' function will lead to overflow of the buffer 'fn'. NimContact files.cpp 290Строка »_timer» не влезает в массив 'fn'. Хотя в строке всего 6 символов, нужно не забывать про терминальный 0. Теоретически здесь имеет место неопределённое поведение. На практике получится, что будет задет массив 'tmp'. В нулевой элемент массива 'tmp' будет записан '0'.
Следующий пример ещё печальней. Здесь испортится HANDLE какой-то иконки:
typedef struct { int cbSize; char caps[0×10]; HANDLE hIcon; char name[MAX_CAPNAME]; } ICQ_CUSTOMCAP;
void InitCheck () { … strcpy (cap.caps, «GPG AutoExchange»); … } Предупреждение PVS-Studio: V512 A call of the 'strcpy' function will lead to overflow of the buffer 'cap.caps'. New_GPG main.cpp 2246Вновь не учтён терминальный 0. Возможно, здесь уместнее было бы воспользоваться функцией memcpy ().
Другие места с проблемным кодом:
main.cpp 2261 messages.cpp 541 messages.cpp 849 utilities.cpp 547 Великая и ужасная функция strncat () Многие слышали про опасность функции strcat () и использую на их взгляд более безопасную strncat (). Вот только редко кто умеет обращаться с этой функцией правильно. Она намного опаснее, чем может показаться. Дело в том, что третий аргумент задаёт не максимальную длину буфера, а сколько в буфере осталось свободного места.Вот такой код совершенно некорректен:
BOOL ExportSettings (…) { … char header[512], buff[1024], abuff[1024]; … strncat (buff, abuff, SIZEOF (buff)); … } Предупреждение PVS-Studio: V645 The 'strncat' function call could lead to the 'buff' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. Miranda fontoptions.cpp 162Если, 'buff' занят на половину, то такой код не обратит на это внимание и позволит добавить ещё 1000 символов. И таким образом произойдет выход за границу массива. Причем очень существенный. С таким же успехом можно было просто писать strcat ().
Вообще, запись strncat (…, …, SIZEOF (X)) в принципе неверна. Она означает, что в массиве ВСЕГДА есть ещё свободное место.
В Miranda NG есть ещё 48 мест где так неправильно используется strncat (). Вот они: MirandaNG-645–1.txt.
Кстати, эти места в коде можно рассматривать как потенциальные уязвимости.
В оправдание программистов из Miranda NG, стоит отметить, что некоторые всё таки читали описание функции strncat (). Они пишут так:
void __cdecl GGPROTO: dccmainthread (void*) { … strncat (filename, (char*)local_dcc→file_info.filename, sizeof (filename) — strlen (filename)); … } Предупреждение PVS-Studio: V645 The 'strncat' function call could lead to the 'filename' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. GG filetransfer.cpp 273К сожалению, это опять неправильно. По крайней мере так можно испортить 1 байт за пределами массива. И я думаю, вы уже догадались, что причина в злосчастном терминальном нуле. Он не учтён.
Поясним эту ошибку на простом примере:
char buf[5] = «ABCD»; strncat (buf, «E», 5 — strlen (buf)); В буфере уже нет места для новых символов. В нём находится 4 символа и терминальный ноль. Выражение »5 — strlen (buf)» равно 1. Функция strncpy () скопирует символ «E» в последний элемент массива 'buf'. Терминальный 0 будет записан уже за пределами буфера.Остальные 34 места кода собраны здесь: MirandaNG-645–2.txt.
Эротика с использованием new[] и delete Кто-то систематически забывает писать квадратные скобки для оператора delete: extern «C» int __declspec (dllexport) Load (void) { int wdsize = GetCurrentDirectory (0, NULL); TCHAR *workingDir = new TCHAR[wdsize]; GetCurrentDirectory (wdsize, workingDir); Utils: convertPath (workingDir); workingDirUtf8 = mir_utf8encodeT (workingDir); delete workingDir; … } Предупреждение PVS-Studio: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It’s probably better to use 'delete [] workingDir;'. IEView ieview_main.cpp 68Вот здесь ещё 20 таких мест: MirandaNG-611–1.txt.
Впрочем, такие ошибки часто обходятся без видимых последствии. Именно поэтому я отнёс их к разделу «эротика». Более жесткое зрелище ожидает ниже.
Разврат с использованием new, malloc, delete и free Путаются способы выделения и освобождения памяти: void CLCDLabel: UpdateCutOffIndex () { … int *piWidths = new int[(*--m_vLines.end ()).length ()]; … free (piWidths); … } Предупреждение PVS-Studio: V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'piWidths' variable. MirandaG15 clcdlabel.cpp 209Ещё 11 поз из Камасутры здесь: MirandaNG-611–2.txt.
Бессмысленные проверки В случае нехватки памяти обыкновенный оператор 'new' генерирует исключение. Поэтому нет смысла проверять указатель, который вернул оператор 'new' на равенство нулю.Обычно лишняя проверка безобидна. Однако, иногда встречается вот такой код:
int CIcqProto: GetAvatarData (…) { … ar = new avatars_request (ART_GET); // get avatar if (! ar) { // out of memory, go away m_avatarsMutex→Leave (); return 0; } … } Предупреждение PVS-Studio: V668 There is no sense in testing the 'ar' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ICQ icq_avatar.cpp 608В случае ошибки следует освободить мьютекс. Но этого не произойдёт. В случае ошибки создания объекта события будут развиваться совсем не так, как планирует программист.
Предлагаю разработчикам проверить следующие 83 аналогичных предупреждений анализатора: MirandaNG-668.txt.
Путаница между SIZEOF () и _tcslen () #define SIZEOF (X) (sizeof (X)/sizeof (X[0])) … TCHAR *ptszVal; … int OnButtonPressed (WPARAM wParam, LPARAM lParam) { … int FinalLen = slen + SIZEOF (dbv.ptszVal) + 1; … } Предупреждение PVS-Studio: V514 Dividing sizeof a pointer 'sizeof (dbv.ptszVal)' by another value. There is a probability of logical error presence. TranslitSwitcher layoutproc.cpp 827Здесь написано что-то странное. Макрос SIZEOF () применяется к указателю, что не имеет никакого смысла. Есть подозрения, что хотели подсчитать длину строки. Тогда для этого следует использовать функцию _tcslen ().
Аналогично:
layoutproc.cpp 876 layoutproc.cpp 924 main.cpp 1300 Порча vptr class CBaseCtrl { … virtual void Release () { } virtual BOOL OnInfoChanged (MCONTACT hContact, LPCSTR pszProto); … };
CBaseCtrl: CBaseCtrl () { ZeroMemory (this, sizeof (*this)); _cbSize = sizeof (CBaseCtrl); } Предупреждение PVS-Studio: V598 The 'memset' function is used to nullify the fields of 'CBaseCtrl' class. Virtual method table will be damaged by this. UInfoEx ctrl_base.cpp 77Программист поленился и для обнуления полей класса воспользовался функцией ZeroMemory (). Он не учёл, что этот класс содержит указатель на таблицу виртуальных методов. В базовом классе многие методы объявлены как виртуальные. Порча указателя на таблицу виртуальных методов приведёт к неопределённому поведению при работе с объектом, инициализированным таким кустарным методом.
Аналогично:
ctrl_base.cpp 87 ctrl_base.cpp 103. Время жизни объектов static INT_PTR CALLBACK DlgProcFindAdd (…) { … case IDC_ADD: { ADDCONTACTSTRUCT acs = {0};
if (ListView_GetSelectedCount (hwndList) == 1) { … } else { … PROTOSEARCHRESULT psr = { 0 }; <<<--- psr.cbSize = sizeof(psr); psr.flags = PSR_TCHAR; psr.id = str;
acs.psr = &psr; <<<--- acs.szProto = (char*)SendDlgItemMessage(....); } acs.handleType = HANDLE_SEARCHRESULT; CallService(MS_ADDCONTACT_SHOW, (WPARAM)hwndDlg, (LPARAM)&acs); } break; .... } Предупреждение PVS-Studio: V506 Pointer to local variable 'psr' is stored outside the scope of this variable. Such a pointer will become invalid. Miranda findadd.cpp 777Объект 'psr' перестанет существовать, когда произойдёт выход из else-ветви. Однако, указатель на этот объект был сохранён и будет в дальнейшем использоваться. Пример настоящего «дикого указателя». К чему приведёт работа с ним — неизвестно.
Ещё один аналогичный пример:
HMENU BuildRecursiveMenu (…) { … if (GetKeyState (VK_CONTROL) & 0×8000) { TCHAR str[256]; mir_sntprintf (str, SIZEOF (str), _T (»%s (%d, id %x)»), mi→pszName, mi→position, mii.dwItemData);
mii.dwTypeData = str; } … } Предупреждение PVS-Studio: V507 Pointer to local array 'str' is stored outside the scope of this array. Such a pointer will become invalid. Miranda genmenu.cpp 973Текст распечатывается во временный массив, который тут же уничтожается. Однако, указатель на этот массив будет использован где-то в другой части программы.
Удивительно, как такие программы вообще работают. Ещё 9 мест, где обитают дикие указатели: MirandaNG-506–507.txt.
Мучения 64-битных указателей Я не изучал 64-битные диагностики. Посмотрел только предупреждения с номером V220. Почти каждое такое предупреждение — самая настоящая ошибка.Пример некорректного кода с точки зрения 64-битности:
typedef LONG_PTR LPARAM;
LRESULT WINAPI SendMessageA ( __in HWND hWnd, __in UINT Msg, __in WPARAM wParam, __in LPARAM lParam);
static INT_PTR CALLBACK DlgProcOpts (…) { … SendMessageA (hwndCombo, CB_ADDSTRING, 0, (LONG)acc[i].name); … } Предупреждение PVS-Studio: V220 Suspicious sequence of types castings: memsize → 32-bit integer → memsize. The value being casted: 'acc[i].name'. GmailNotifier options.cpp 55Куда-то нужно передать 64-битный указатель. Для этого, его нужно превратить в тип LPARAM. Однако, вместо этого указатель насильственно превращают в 32-битный тип LONG. И только потом он будет автоматически расширен до LONG_PTR. Эта ошибка пришла из 32-битных времён, когда типы LONG и LPARAM совпадали. Теперь это не так. Будут испорчены старшие 32 бита в 64-битном указателе.
Такие баги неприятны тем, что очень неохотно проявляют себя. Будет везти, пока память выделяется в младших адресах памяти.
Вот ещё 20 мест, где портятся 64-битные указатели: MirandaNG-220.txt.
Неочищенные приватные данные void CAST256:: Base: UncheckedSetKey (…) { AssertValidKeyLength (keylength); word32 kappa[8]; … memset (kappa, 0, sizeof (kappa)); } Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'kappa' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. Cryptlib cast.cpp 293В Release версии компилятор удалит вызов функции memset (). Почему так, можно узнать из описания диагностики.
Не затрутся приватные данные ещё в 6 местах: MirandaNG-597.txt.
Разное Есть ещё пара предупреждений анализатора, которые я свалю в одну кучу. void LoadStationData (…, WIDATA *Data) { … ZeroMemory (Data, sizeof (Data)); … } Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'Data'. Weather weather_ini.cpp 250Выражение 'sizeof (Data)' возвращает размер указателя, а не WIDATA. Будет обнулена только часть объекта. Правильно будет написать: sizeof (*Data).
void CSametimeProto: CancelFileTransfer (HANDLE hFt) { … FileTransferClientData* ftcd = …;
if (ftcd) { while (mwFileTransfer_isDone (ftcd→ft) && ftcd) ftcd = ftcd→next; … } Предупреждение PVS-Studio: V713 The pointer ftcd was utilized in the logical expression before it was verified against nullptr in the same logical expression. Sametime files.cpp 423В условии цикла указатель 'ftcd' в начале разыменовывается, а только потом проверяется. Видимо, выражение стоит переписать так:
while (ftcd && mwFileTransfer_isDone (ftcd→ft)) Заключение Работа с указателями и памятью — не единственное, из чего состоят программы на Си++. В следующей статье мы рассмотрим другие разновидности ошибок, обнаруженных в Miranda NG. Их поменьше, но всё равно достаточно много.Эта статья на английском Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Miranda NG Project to Get the «Wild Pointers» Award (Part 1).
