# Статический анализ: ошибки в медиаплеере и безглючная аська Продолжу экскурсию по ошибкам в программах и демонстрацию полезности статического анализа кода\. Это мой последний пост про пока недоступную для скачивания версию [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Планирую, что через неделю вы уже сможете попробовать первую beta\-версию с новым набором правил общего назначения\. Рассмотрим два проекта\. Первый — [Fennec Media Project](https://sourceforge.net/projects/fennec/)\. Это универсальный медиаплеер, ориентированный на воспроизведение аудио и видео в высоком разрешении\. В комплект исходных кодов входит множество модулей расширения \(plugins\) и кодеков, но анализироваться будет только сам плеер\. Исходный код последней на данный момент версии 1\.2 Alpha доступен [здесь](https://sourceforge.net/projects/fennec/)\. Второй проект — qutIM\. Это кроссплатформенный клиент мгновенного обмена сообщениями с открытым исходным кодом\. Был проанализирован код на момент начала ноября 2010 года\. Набор исходных кодов был предоставлен мне одним из разработчиков, но вы также можете скачать исходный код с официального сайта\. Fennec Media Project\. Небольшой нормальный проект, содержащий нормальное количество ошибок\. Вот первая ошибка\. Или первые две ошибки, смотря как считать\. В общем, в двух местах вместо переменной 'b' используется переменная 'a'\. ```cpp int fennec_tag_item_compare(struct fennec_audiotag_item *a, struct fennec_audiotag_item *b) { int v; if(a->tsize && a->tsize) v = abs(str_cmp(a->tdata, a->tdata)); else v = 1; return v; } ``` PVS\-Studio указал на этот код, так как условие "a\-\>tsize && a\-\>tsize" явно подозрительно\. Диагностическое сообщение и местоположение ошибки в коде: V501 There are identical sub\-expressions to the left and to the right of the '&&' operator: a \-\> tsize && a \-\> tsize media library\.c 1076 А теперь родное и милое сердцу каждого программиста \- лишние точки с запятой\. Вот первый фрагмент: ```cpp int settings_default(void) { ... for(i=0; i<16; i++); for(j=0; j<32; j++) { settings.conversion.equalizer_bands.boost[i][j] = 0.0; settings.conversion.equalizer_bands.preamp[i] = 0.0; } } ``` Сообщение PVS\-Studio и местоположение коде: V529 Odd semicolon ';' after 'for' operator\. settings\.c 483 Второй фрагмент: ```cpp int trans_rest(transcoder_settings *trans) { ... for(i=0; i<16; i++); { trans->eq.eq.preamp[i] = 0.0; for(j=0; j<32; j++) { trans->eq.eq.boost[i][j] = 0.0; } } } ``` Сообщение PVS\-Studio и местоположение коде: V529 Odd semicolon ';' after 'for' operator\. settings\.c 913 Есть еще третий и четвертый фрагмент с ';'\. Но приводить здесь не буду\. Все однотипно и неинтересно\. Дальше не совсем ошибка, но почти\. Вместо функции \_beginthreadex используется CreateThread\. Вызовов CreateThread в Fennec несколько, но приведу только один пример: ```cpp t_sys_thread_handle sys_thread_call(t_sys_thread_function cfunc) { unsigned long tpr = 0; unsigned long tid = 0; return (t_sys_thread_handle) CreateThread(0, 0, cfunc, &tpr, 0,&tid); } ``` Предупреждение PVS\-Studio и местоположение коде: V513 Use \_beginthreadex/\_endthreadex functions instead of CreateThread/ExitThread functions\. system\.c 331 Вдаваться сейчас вглубь вопроса, почему следует использовать \_beginthreadex/\_endthreadex вместо CreateThread/ExitThread, не буду\. Напишу совсем кратко, а подробные обсуждения данного вопроса можно почитать [здесь](https://stackoverflow.com/questions/331536/windows-threading-beginthread-vs-beginthreadex-vs-createthread-c), [здесь](https://forums.codeguru.com/showthread.php?371305.html) и [здесь](https://social.msdn.microsoft.com/Forums/vstudio/en-US/c727ae29-5a7a-42b6-ad0b-f6b21c1180b2/createthread-vs-beginthreadex?forum=vclanguage)\. В священном писании \(в MSDN\) сказано: A thread in an executable that calls the C run\-time library \(CRT\) should use the \_beginthreadex and \_endthreadex functions for thread management rather than CreateThread and ExitThread; this requires the use of the multi\-threaded version of the CRT\. If a thread created using CreateThread calls the CRT, the CRT may terminate the process in low\-memory conditions\. В общем лучше подстраховаться и всегда вызывать именно \_beginthreadex/\_endthreadex\. Кстати именно так рекомендует поступать и Джеффри Рихтера в шестой главе "Windows для профессионалов: создание эффективных Win32\-приложений с учетом специфики 64\-разрядной версии Windows" / Пер\. с англ\. \- 4\-е изд\. Обнаружилось несколько неудачных использований функции memset\. Кстати, я до недавнего времени думал, что беспокойство, связанное с использованием memset, memcmp, memcpy – дело прошлого\. Мол, это раньше так писали, но сейчас все знают про опасности, аккуратны с этими функциями, используют sizeof\(\), используют контейнеры из STL и так далее\. А сейчас все розовое и мягкое\. Оказывается, что нет\. Я за последний месяц столько ляпов с этими функциями насмотрелся\. Так что все эти ошибки по\-прежнему цветут и пахнут\. Вернемся в Fennec\. Первый memset: ```cpp #define uinput_size 1024 typedef wchar_t letter; letter uinput_text[uinput_size]; string basewindows_getuserinput(const string title, const string cap, const string dtxt) { memset(uinput_text, 0, uinput_size); ... } ``` Диагностика PVS\-Studio и местоположение коде: V512 A call of the 'memset' function will lead to a buffer overflow or underflow\. base windows\.c 151 На первый взгляд с "memset\(uinput\_text, 0, uinput\_size\);" все хорошо\. И возможно даже и было хорошо, в те времена, когда тип 'letter' был 'char'\. Но теперь это 'wchar\_t' и как результат мы чистим только половину буфера\. Второй неудачный memset: ```cpp typedef wchar_t letter; letter name[30]; int Conv_EqualizerProc(HWND hwnd,UINT uMsg, WPARAM wParam,LPARAM lParam) { ... memset(eqp.name, 0, 30); ... } ``` Воистину магические числа – это зло\. Вроде и не сложно написать "sizeof\(eqp\.name\)"\. Но упорно не пишем и продолжаем вновь и вновь отстреливать себе ногу :\)\. Диагностика PVS\-Studio и местоположение коде: V512 A call of the 'memset' function will lead to a buffer overflow or underflow\. base windows\.c 2892 Ну и еще в одном месте такая шибка есть: V512 A call of the 'memset' function will lead to a buffer overflow or underflow\. transcode settings\.c 588 Возможно, иногда в каких\-то программах вы замечали, что диалоги открытия/сохранения файлов работают со странностями или в полях доступных расширений присутствует какая\-то чушь\. Сейчас вы узнаете, откуда у этого растут ноги\. В Windows API есть структуры, в которых указатели на строки должны заканчиваться двойным нулем\. Наиболее используемым является член lpstrFilter в структуре OPENFILENAME\. Этот параметр на самом деле указывает на набор строк, разделенных символом '\\0'\. А для того чтобы узнать, что строки закончились и нужны два нуля в конце\. Вот только это очень просто забыть\. Фрагмент кода: ```cpp int JoiningProc(HWND hwnd,UINT uMsg, WPARAM wParam,LPARAM lParam) { ... OPENFILENAME lofn; memset(&lofn, 0, sizeof(lofn)); ... lofn.lpstrFilter = uni("All Files (*.*)\0*.*"); ... } ``` Сообщение PVS\-Studio и местоположение коде: V540 Member 'lpstrFilter' should point to string terminated by two 0 characters\. base windows\.c 5309 Будет диалог работать нормально или нет, зависит от того, что будет расположено в памяти после строки "All Files \(\*\.\*\)\\0\*\.\*"\. По правильному здесь следовало написать "All Files \(\*\.\*\)\\0\*\.\*\\0"\. Один ноль явно указали мы, еще один ноль добавит компилятор\. Аналогичная беда и с другими диалогами\. ```cpp int callback_presets_dialog(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { ... // SAVE OPENFILENAME lofn; memset(&lofn, 0, sizeof(lofn)); ... lofn.lpstrFilter = uni("Equalizer Preset (*.feq)\0*.feq"); ... ... // LOAD ... lofn.lpstrFilter = uni("Equalizer Preset (*.feq)\0*.feq"); ... } int localsf_show_save_playlist(void) { OPENFILENAME lofn; memset(&lofn, 0, sizeof(lofn)); ... lofn.lpstrFilter = uni("Text file (*.txt)\0*.txt\0M3U file\0*.m3u"); ... } ``` Диагностика PVS\-Studio и местоположение в коде: V540 Member 'lpstrFilter' should point to string terminated by two 0 characters\. base windows\.c 986 V540 Member 'lpstrFilter' should point to string terminated by two 0 characters\. base windows\.c 1039 V540 Member 'lpstrFilter' should point to string terminated by two 0 characters\. shared functions\.c 360 Теперь подозрительная функция\. Очень подозрительная\. Впрочем, действительно тут ошибка или просто неудачно написано, я не знаю: ```cpp unsigned long ml_cache_getcurrent_item(void) { if(!mode_ml) return skin.shared->audio.output.playlist.getcurrentindex(); else return skin.shared->audio.output.playlist.getcurrentindex(); } ``` Диагностика PVS\-Studio и местоположение в коде: V523 The 'then' statement is equivalent to the 'else' statement\. media library window\.c 430 Я не стал заниматься анализом разнообразный модулей расширений, идущих вместе с Fennec\. Но там не меньше разных грустных мест\. Приведу только пару примеров\. Фрагмент кода из проекта Codec ACC\. ```cpp void MP4RtpHintTrack::GetPayload(...) { ... if (pSlash != NULL) { pSlash++; if (pSlash != '\0') { length = strlen(pRtpMap) - (pSlash - pRtpMap); *ppEncodingParams = (char *)MP4Calloc(length + 1); strncpy(*ppEncodingParams, pSlash, length); } } ``` Как следует из диагностического сообщения PVS\-Studio: V528 It is odd that pointer to 'char' type is compared with the '\\0' value\. Probably meant: \*pSlash \!\= '\\0'\. rtphint\.cpp 346 здесь забыли разыменовать указатель\. Получается, что мы делаем бессмысленное сравнение указателя с 0\. Должно было быть: "if \(\*pSlash \!\= '\\0'\)"\. Фрагмент кода из проекта Decoder Mpeg Audio: ```cpp void* tag_write_setframe(char *tmem, const char *tid, const string dstr) { ... if(lset) { fhead[11] = '\0'; fhead[12] = '\0'; fhead[13] = '\0'; fhead[13] = '\0'; } ... } ``` Диагностическое сообщение PVS\-Studio и местоположение в коде: V525 The code containing the collection of similar blocks\. Check items '11', '12', '13', '13' in lines 716, 717, 718, 719\. id3 editor\.c 716 Вот оно зло метода Copy\-Paste :\)\. В целом на проекте Fennec Media Project анализ общего назначения в PVS\-Studio показал себя очень хорошо\. Анализ был выполнен с низким процентом ложных срабатываний\. Всего PVS\-Studio указал на 31 фрагмент кода\. При этом в 19 местах код действительно следует поправить\. Теперь перейдем к проекту qutIM\. Вот с эти проектом PVS\-Studio потерпел поражение\. Несмотря на то, что проект достаточно крупный \(около 200 тысяч сток\), анализатор PVS\-Studio не смог выявить в нем ошибок\. Хотя они конечно есть\. Они везде и всегда есть :\)\. И разработчики qutIM с этим не спорят, так как в некоторых случаях qutIM умудряется падать\. Приходится засчитать одно очко "команде ошибок"\. Что это означает? Это означает: 1\) Проект qutIM очень качественный проект\. И хотя он тоже содержит ошибки, но они весьма редки и слишком высокого уровня для статического анализа \(по крайней мере, для PVS\-Studio\)\. 2\) PVS\-Studio предстоит еще долгий путь развития и обучения более высокоуровневым диагностикам\. Теперь стало более очевидно к чему стремиться\. Цель \- найти в qutIM хотя бы парочку настоящих ошибок\. Выдал ли что\-то PVS\-Studio для проекта qutIM? Выдал\. Но немного и почти все ложные срабатывания\. Их представляющего хоть какой\-то интерес, можно выделить только следующее\. A\) Используются функции CreateThread\. B\) Найдено несколько странных функций\. Потом один из авторов qutIM сообщил, что это забытые заглушки\. Странность в том, что одна имеет имя save\(\), другая cancel\(\), но их содержимое идентично: ```cpp void XSettingsWindow::save() { QWidget *c = p->stackedWidget->currentWidget(); while (p->modifiedWidgets.count()) { SettingsWidget *widget = p->modifiedWidgets.takeFirst(); widget->save(); if (widget != c) widget->deleteLater(); } p->buttonBox->close(); } void XSettingsWindow::cancel() { QWidget *c = p->stackedWidget->currentWidget(); while (p->modifiedWidgets.count()) { SettingsWidget *widget = p->modifiedWidgets.takeFirst(); widget->save(); if (widget != c) widget->deleteLater(); } p->buttonBox->close(); } ``` Диагностика PVS\-Studio: V524 It is odd that the 'cancel' function is fully equivalent to the 'save' function \(xsettingswindow\.cpp, line 256\)\. xsettingswindow\.cpp 268 Надеюсь было интересно, и вы скоро захотите попробовать PVS\-Studio 4\.00 Beta\. Конечно, PVS\-Studio пока находит мало ошибок общего вида, но ведь это только начало\. При этом исправление даже одной единственной ошибки на этапе кодирования может сэкономить массу нервов заказчикам, тестерам и программистам\.