Только что, я сел и проверил проект VirtualDub с помощью PVS-Studio. Выбор был случаен. Я считаю, самое главное регулярно проверять/перепроверять различные проекты, чтобы показать, как развивается анализатор кода PVS-Studio. А какой проект будет проверен, не так важно. Ошибки есть везде. Проект VirtualDub мы уже проверяли в 2011 году, но тогда почти ничего интересного не нашлось. Вот я и решил посмотреть, как обстоят дела, спустя 2 года.
Я скачал с сайта VirtualDub архив VirtualDub-1.10.3-src.7z. Для анализа я использовал PVS-Studio версии 5.10. На анализ я потратил около часа, так что не судите строго. Наверняка, я что-то пропустил. И наоборот, мог поcчитать корректный код за подозрительный. Прошу, тех, кто поддерживает проект VirtualDub не полагаться на мой отчет, а произвести самостоятельную проверку. Мы всегда идём на встречу open-source сообществу и готовы выделить регистрационный ключ.
Ещё я хочу попросить Эвери Ли отнестись к этой статье с пониманием. В прошлый раз, он очень болезненно воспринял упоминание VirtualDub в одной из моих статей. Я вовсе не хотел, и не хочу сказать, что какая-то программа является глючной. Программные ошибки есть везде. Моя цель показать пользу, которую может давать использование статического анализа кода. Попутно, это сделает open-source проекты немного надёжнее. Это замечательно.
Конечно, разовые проверки малоэффективны. Но с этим, к сожалению, я ничего поделать не могу. Использовать или не использовать регулярно инструменты статического анализа, зависит от разработчиков. Я только могу пытаться объяснить, в чем польза регулярного использования. Вот одна из интересных заметок на эту тему: Лев Толстой и статический анализ кода.
Впрочем, эта статья про ошибки, а не методологию использования статического анализа. Давайте посмотрим, что нового интересного нашел анализатор PVS-Studio в VirtualDub.
Виртуальные деструкторы
В языке программирования C++ деструктор полиморфного базового класса должен объявляться виртуальным. Только так обеспечивается корректное разрушение объекта производного класса через указатель на соответствующий базовый класс.
Я знаю, что все это знают. Однако это не мешает забыть объявить деструктор виртуальным.
В VirtualDub есть класс VDDialogBaseW32:
class VDDialogBaseW32 { .... ~VDDialogBaseW32(); .... virtual INT_PTR DlgProc(....) = 0; virtual bool PreNCDestroy(); .... }Как видите, он содержит виртуальные функции. Однако деструктор не объявлен виртуальным. Естественно, он него наследуются классы. Например, VDDialogAudioFilterFormatConvConfig:
class VDDialogAudioFilterFormatConvConfig : public VDDialogBaseW32 { .... };Ошибка уничтожения объекта выглядит вот так:
INT_PTR CALLBACK VDDialogBaseW32::StaticDlgProc(....) { VDDialogBaseW32 *pThis = (VDDialogBaseW32 *)GetWindowLongPtr(hwnd, DWLP_USER); .... delete pThis; .... }Предупреждение выданное PVS-Studio: V599 The destructor was not declared as a virtual one, although the ‘VDDialogBaseW32’ class contains virtual functions. VirtualDub gui.cpp 997
Как видите, для уничтожения объекта используется указатель на базовый класс. Такое удаление объекта приведет к неопределенному поведению программы.
Аналогичная беда существует и с классом VDMPEGAudioPolyphaseFilter.
Ещё о неопределённом поведении
По поводу ошибок, связанных с виртуальным деструктором, вопросов не возникает. Намного более скользкой темой являются операции сдвига. Вот один из таких примеров:
void AVIVideoGIFOutputStream::write(....) { { .... for(int i=0; i<palsize; ++i) dict[i].mPrevAndLastChar = (-1 << 16) + i; .... }Можно сколько угодно утверждать, что это совершенно надёжный код, который успешно используется уже больше 10 лет. Однако, всё равно, здесь мы имеем дело с неопределенным поведением программы. Вот что стандарт говорит на тему таких конструкций:
The shift operators << and >> group left-to-right.
shift-expression << additive-expression
shift-expression >> additive-expression
The operands shall be of integral or unscoped enumeration type and integral promotions are performed.
1. The type of the result is that of the promoted left operand. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand.
2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 has an unsigned type, the value of the result is E1 * 2^E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.
3. The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.
То, что код работает, это везение. При осваивании нового компилятора или других ключей для оптимизации, программа может неожиданно изменить своё поведение. Подробнее про сдвиги и о том, стоит или не стоит править код, можно почитать в статье "Не зная брода, не лезь в воду. Часть третья".
Вот полный список мест, где анализатор PVS-Studio обнаружил Undefined behavior или Unspecified behavior в проекте VirtualDub.
Опечатки
static ModuleInfo *CrashGetModules(void *&ptr) { .... while(*pszHeap++); if (pszHeap[-1]=='.') period = pszHeap-1; .... }Диагностическое сообщение выданное PVS-Studio: V529 Odd semicolon ‘;’ after ‘while’ operator. VirtualDub crash.cpp 462
Обратите внимание на точку с запятой, стоящую после ‘while’. Здесь или ошибка или код неверно отформатирован. Я думаю, это именно ошибка. Цикл «while(*pszHeap++);» пройдет до конца строки и в результате, переменная ‘pszHeap’ будет указывать на память после терминального нуля. Проверка «if (pszHeap[-1]==’.’)» не имеет смысла. По адресу «pszHeap[-1]» всегда находится терминальный ноль.
Рассмотрим другую опечатку, при работе со строками.
void VDBackfaceService::Execute(...., char *s) { .... if (*s == '"') { while(*s && *s != '"') ++s; } else { .... }Диагностическое сообщение выданное PVS-Studio: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 183, 184. VirtualDub backface.cpp 183
Этот код должен пропустить всё, что содержится в кавычках. По крайней мере, мне так кажется. Однако, условие (*s && *s != ‘"’) сразу является ложным. Возможно, код должен был выглядеть так:
if (*s == '"') { ++s; while(*s && *s != '"') ++s; }
Оператор new генерирует исключения при ошибке выделения памяти
В старом коде, часто можно встретить проверку того, что вернул оператор new. Выглядит это так:
int *p = new int[10]; if (!p) return false;Современные компиляторы, поддерживающие стандарт языка Си++, должны сгенерировать исключение, если не удалось выделить память. Можно сделать, чтобы оператор ‘new’ не генерировал исключения, но к рассматриваемым случаям, сейчас это отношения не имеет.
Таким образом, проверка if (!p) является лишней. В целом такой код безобиден. Лишняя проверка. Ничего страшного.
Однако старый код может привести к неприятным последствиям. Рассмотрим фрагмент из проекта VirtualDub. void HexEditor::Find(HWND hwndParent) { .... int *next = new int[nFindLength+1]; char *searchbuffer = new char[65536]; char *revstring = new char[nFindLength]; .... if (!next || !searchbuffer || !revstring) { delete[] next; delete[] searchbuffer; delete[] revstring; return; } .... }Диагностическое сообщение выданное PV-Studio: V668 There is no sense in testing the ‘next’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. VirtualDub hexviewer.cpp 2012
Если в строке «char *revstring = new char[nFindLength];» возникнет исключение, то произойдет утечка памяти. Операторы delete[] не будут вызваны. Не критичная ошибка, но всё равно стоит про неё упомянуть.
Здесь, перечислены все места, где в VirtualDub проверяется указатель после вызова оператора ‘new’.
Ссылка на уничтоженный объект
vdlist_iterator& operator--(int) { vdlist_iterator tmp(*this); mp = mp->mListNodePrev; return tmp; }Диагностическое сообщение выданное PVS-Studio: V558 Function returns the reference to temporary local object: tmp. VirtualDub vdstl.h 460
Функция реализована неправильно. Она возвращает ссылку на локальный объект ‘tmp’. После выхода из функции, он будет уже разрушен. Работа с этой ссылкой, приведёт к неопределенному поведению.
Кстати, оператор ++, находящийся рядом, реализован правильно.
В начале используем, потом проверяем
В разных программах, часто можно встретить ошибку, когда указатель сначала разыменовывается, а уже только потом, сравнивается с NULL. Такие ошибки не проявляют себя очень долгое время, так как равенство указателя NULL, это нештатная редкая ситуация. Эти недочёты, существуют и в коде VirtualDub. Пример:
LRESULT YUVCodec::DecompressGetFormat(BITMAPINFO *lpbiInput, BITMAPINFO *lpbiOutput) { BITMAPINFOHEADER *bmihInput = &lpbiInput->bmiHeader; BITMAPINFOHEADER *bmihOutput = &lpbiOutput->bmiHeader; LRESULT res; if (!lpbiOutput) return sizeof(BITMAPINFOHEADER); .... }Диагностическое сообщение выданное PVS-Studio: V595 The ‘lpbiOutput’ pointer was utilized before it was verified against nullptr. Check lines: 82, 85. VirtualDub yuvcodec.cpp 82
В начале указатель «lpbiOutput» разыменовывается. Затем проверяется: «if (!lpbiOutput)». Такие ошибка обычно возникаю в процессе рефакторинга. Новый код вставляется до нужных проверок. Чтобы исправить приведённый код, нужно поменять последовательность действий:
LRESULT YUVCodec::DecompressGetFormat(BITMAPINFO *lpbiInput, BITMAPINFO *lpbiOutput) { if (!lpbiOutput) return sizeof(BITMAPINFOHEADER); BITMAPINFOHEADER *bmihInput = &lpbiInput->bmiHeader; BITMAPINFOHEADER *bmihOutput = &lpbiOutput->bmiHeader; LRESULT res; .... }Другие места, где анализатор выдает предупреждение V595, перечислены здесь.
Работа с типом HRESULT
VDPosition AVIReadTunnelStream::TimeToPosition(VDTime timeInUs) { AVISTREAMINFO asi; if (AVIStreamInfo(pas, &asi, sizeof asi)) return 0; return VDRoundToInt64(timeInUs * (double)asi.dwRate / (double)asi.dwScale * (1.0 / 1000000.0)); }Диагностическое сообщение выданное PVS-Studio: V545 Such conditional expression of ‘if’ operator is incorrect for the HRESULT type value ‘AVIStreamInfoA(pas, & asi, sizeof asi)’. The SUCCEEDED or FAILED macro should be used instead. VirtualDub avireadhandlertunnelw32.cpp 230
Функция AVIStreamInfo() возвращает значение типа HRESULT. Этот тип нельзя интерпретировать как ‘bool’. Информация, хранящаяся в переменной типа HRESULT, имеет достаточно сложную структуру. Для проверки значения HRESULT необходимо использовать макрос SUCCEEDED или FAILED, объявленные в «WinError.h». Вот как устроены эти макросы:
#define FAILED(hr) (((HRESULT)(hr)) < 0) #define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)Правильный вариант кода:
if (FAILED(AVIStreamInfo(pas, &asi, sizeof asi)))Аналогичные предупреждения PVS-Studio выдаёт на следующие строки:
- avireadhandlertunnelw32.cpp 238
- avireadhandlertunnelw32.cpp 335
- inputfileavi.cpp 440
- context_d3d11.cpp 959
Магические числа
Задание длины строки с помощью числа не очень хорошая идея. Очень легко ошибиться, подсчитывая символы. Пример:
bool VDOpenGLBinding::Attach(....) { .... if (!memcmp(start, "GL_EXT_blend_subtract", 20)) .... }Диагностическое сообщение выданное PVS-Studio: V512 A call of the ‘memcmp’ function will lead to underflow of the buffer ‘«GL_EXT_blend_subtract»’. Riza opengl.cpp 393
Длина строки «GL_EXT_blend_subtract» не 20, а 21 символ. Ошибка некритична. На практике коллизий не возникнет. Тем не менее, следует избегать таких магических чисел. Лучше использовать специальный макрос для подсчета длины строки. Пример:
#define LiteralStrLen(S) (sizeof(S) / sizeof(S[0]) - 1)В Си++ можно сделать более безопасную шаблонную функцию:
template <typename T, size_t N> char (&ArraySizeHelper(T (&array)[N]))[N]; template <typename T, size_t N> size_t LiteralStrLen(T (&array)[N]) { return sizeof(ArraySizeHelper(array)) - 1; }Преимущество второго варианта — нельзя случайно передать в качестве аргумента простой указатель. Подробнее этот приём описан в статье "PVS-Studio vs Chromium".
Абсолютные пути
VDDbgHelpDynamicLoaderW32::VDDbgHelpDynamicLoaderW32() { hmodDbgHelp = LoadLibrary( "c:\\program files\\debugging tools for windows\\dbghelp"); if (!hmodDbgHelp) { hmodDbgHelp = LoadLibrary("c:\\program files (x86)\\...... .... }Диагностическое сообщение выданное PVS-Studio: V631 Consider inspecting the ‘LoadLibraryA’ function call. Defining an absolute path to the file or directory is considered a poor style. VirtualDub leaks.cpp 67, 69
Думаю понятно, чем этот код плох. Конечно, код связан с отладкой, и вряд ли как-то негативно скажется на конечных пользователях. Но всё равно, было бы лучше получить правильный путь до Program Files.
Неправильный аргумент
sint64 rva; void tool_lookup(....) { .... printf("%08I64x %s + %x [%s:%d]\n", addr, sym->name, addr-sym->rva, fn, line); .... }Диагностическое сообщение выданное PVS-Studio: V576 Incorrect format. Consider checking the fourth actual argument of the ‘printf’ function. The argument is expected to be not greater than 32-bit. Asuka lookup.cpp 56
Переменная ‘rva’ является 64-битным типом. Это значит, что в стек будет помещено 8 байт. Функция printf() является функцией с переменным количеством аргументов. Тип данных, которые она должна обработать, задаётся с помощью строки форматирования. В данном случае, переменная ‘rva’ будет обработана как 32-битная переменная ("%x").
Приведёт данная ошибка к сбою или нет, зависит от того, как организует передачу аргументов компилятор и от разрядности платформы. Например, в Win64 все целочисленные типы вначале приводятся к 64-битному типу, и уже затем помещаются в стек. Проблемы, что переменная займет в стеке больше места, чем надо, не будет.
Впрочем, если переменная ‘rva’ хранит значения более INT_MAX, её значение в любом случае будет распечатано некорректно.
Аналогичные предупреждения анализатор выдаёт здесь:
- dubstatus.cpp 360
- lookup.cpp 58
Неправильные сравнения
void VDVideoCompressorVCM::GetState(vdfastvector<uint8>& data) { DWORD res; .... res = ICGetState(hic, data.data(), size); .... if (res < 0) throw MyICError("Video compression", res); }Диагностическое сообщение выданное PVS-Studio: V547 Expression ‘res < 0’ is always false. Unsigned type value is never < 0. Riza w32videocodecpack.cpp 828
Переменная ‘res’ имеем беззнаковый тип DWORD. Это значит, что выражение «res < 0» всегда равно значению ‘false’.
Аналогичная проверка содержится здесь: w32videocodec.cpp 284.
Рассмотрим ещё одну схожую ошибку.
#define ICERR_CUSTOM -400L static const char *GetVCMErrorString(uint32 icErr) { .... if (icErr <= ICERR_CUSTOM) err = "A codec-specific error occurred."; .... }Диагностическое сообщение выданное PVS-Studio: V605 Consider verifying the expression: icErr <= — 400L. An unsigned value is compared to the number -400. system error_win32.cpp 54
Переменная ‘ icErr’ имеет тип ‘unsigned’. Следовательно, перед сравнением число ‘-400’ будет неявно преобразовано в ‘unsigned’. Значение ‘-400’ превратится в 4294966896. Таким образом, сравнение (icErr <= -400) эквивалентно (icErr <= 4294966896). По всей видимости, это не то, что хотел программист.
Разное странное
void AVIOutputFile::finalize() { .... if (stream.mChunkCount && hdr.dwScale && stream.mChunkCount) .... }Диагностическое сообщение выданное PVS-Studio: V501 There are identical sub-expressions ‘stream.mChunkCount’ to the left and to the right of the ‘&&’ operator. VirtualDub avioutputfile.cpp 761
Два раза проверяется переменная ‘stream.mChunkCount’. Одна проверка лишняя или забыли проверить что-то ещё.
void VDVideoCompressorVCM::Start(const void *inputFormat, uint32 inputFormatSize, const void *outputFormat, uint32 outputFormatSize, const VDFraction& frameRate, VDPosition frameCount) { this->hic = hic; .... }Диагностическое сообщение выданное PVS-Studio: V570 The ‘this->hic’ variable is assigned to itself. Riza w32videocodecpack.cpp 253
void VDDialogAudioConversionW32::RecomputeBandwidth() { .... if (IsDlgButtonChecked(mhdlg, IDC_PRECISION_NOCHANGE)) { if (mbSourcePrecisionKnown && mbSource16Bit) bps *= 2; else bps = 0; } if (IsDlgButtonChecked(mhdlg, IDC_PRECISION_16BIT)) bps *= 2; .... }Диагностическое сообщение выданное PVS-Studio: V646 Consider inspecting the application’s logic. It’s possible that ‘else’ keyword is missing. VirtualDub optdlg.cpp 120
Возможно, код неправильно отформатирован. А возможно, забыто ключевое слово ‘else’.
bool VDCaptureDriverScreen::Init(VDGUIHandle hParent) { .... mbAudioHardwarePresent = false; mbAudioHardwarePresent = true; .... }Диагностическое сообщение выданное PVS-Studio: V519 The ‘mbAudioHardwarePresent’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 274, 275. VDCapture cap_screen.cpp 275
Заключение
Как видите, даже при разовом запуске, статический анализ может быть полезен. Но намного полезнее запускать его регулярно. Ведь предупреждения (warnings) в компиляторе, программисты включают не один раз перед релизом, а пользуются ими постоянно. Та же самая ситуация и с инструментами статического анализа. Постоянное их использование позволяет оперативно устранять ошибки. Можно рассматривать PVS-Studio как некую надстройку над компилятором, которая выдаёт больше интересных предупреждений. Наилучший вариант, это использование инкрементального анализа кода. Вы обнаружите новые ошибки сразу после компиляции измененных файлов.
ссылка на оригинал статьи http://habrahabr.ru/company/pvs-studio/blog/198060/
Добавить комментарий