# Есть ли баги в маленьких проектах, или как PVS\-Studio проверял Blend2D Мы часто проверяем большие проекты, потому что в них проще найти ошибки\. А что же PVS\-Studio сможет найти в небольшом проекте? Мы взяли Blend2D – библиотеку для векторной 2D\-графики – и проверили своим анализатором\. Предлагаем ознакомиться с тем, что из этого вышло\. ![0894_Blend2d_ru/image1.png](https://import.viva64.com/docx/blog/0894_Blend2d_ru/image1.png) ## Введение Ни для кого не секрет, что в больших проектах всегда легче находить интересные ошибки\. Это обусловлено не только простым рассуждением "больше кода – больше ошибок", но и тем, что с ростом кодовой базы [растёт и плотность ошибок](https://pvs-studio.ru/ru/blog/posts/0158/)\. Поэтому и проверять мы любим большие проекты, чтобы потом порадовать себя и читателя разнообразием "жирнющих" и заковыристых ошибок и опечаток\. Да и в целом всегда интересно поковыряться в огромном проекте с кучей зависимостей, legacy и прочих вкусняшек\. В связи с этим наблюдением, я захотел отойти от нашей традиции брать большие проекты для статьи\. Решил взять небольшой проект и посмотреть, что в нём сможет найти PVS\-Studio\. Мой выбор пал на Blend2D\. Я проверил ветку master, коммит [c484790\.](https://github.com/blend2d/blend2d/tree/c4847906ea9423fe365ccafcaff62a37df5de3aa) ## Blend2D Blend2D — это движок для векторной 2D\-графики\. Это небольшая библиотека, написанная на C\+\+ и содержащая около 70'000 строк кода: ```cpp --------------------------------------------------------------------- Language files blank comment code --------------------------------------------------------------------- C++ 97 12924 9481 43372 C/C++ Header 137 8305 12971 25225 ``` Эта библиотека позволяет создавать любые 2D\-изображения\. Чтобы достичь высокой производительности, разработчики проекта используют многопоточный рендеринг и самописный растеризатор\. Blend2D предоставляет C и C\+\+ API\. Более подробно узнать про проект и возможности этой библиотеки вы можете на их [сайте](https://blend2d.com/)\. Я же перейду к ошибкам, которые обнаружил PVS\-Studio в исходном коде Blend2D\. ## Всегда ложное выражение [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'h \=\= 0' is always false\. jpegcodec\.cpp 252 ```cpp BLResult blJpegDecoderImplProcessMarker(....) noexcept { uint32_t h = blMemReadU16uBE(p + 1); // .... if (h == 0) return blTraceError(BL_ERROR_JPEG_UNSUPPORTED_FEATURE); // .... impl->delayedHeight = (h == 0); // <= // .... } ``` В данном фрагменте кода в переменную _h_ присваивается результат вызова функции _blMemReadU16uBE_\. Затем, если проверка _h \=\= 0_ оказалась истинной, происходит выход из функции\. Значит, при инициализации _impl\-\>delayedHeight_ в переменной _h_ находится ненулевое значение\. Поэтому в _impl\-\>delayedHeight_ присвоится _false_\. ## Опечатка в сигнатуре функции V557 \[CERT\-ARR30\-C\] Array overrun is possible\. The '3' index is pointing beyond array bound\. geometry\_p\.h 552 ```cpp static BL_INLINE bool blIsCubicFlat(const BLPoint p[3], double f) { if (p[3] == p[0]) { // .... } // .... } ``` В данном фрагменте кода в сигнатуре функции _blIsCubicFlat_ переменная _p_ объявляется как массив размера 3\. А затем в теле функции _blMemReadU16uBE_ вычисляется _p\[3\]_\. Вообще объявление аргумента _const BLPoint p\[3\]_ в сигнатуре функции эквивалентно объявлению _const BLPoint \*p_, а указание размера является подсказкой программисту и никак не используется компилятором\. Поэтому выход за границу массива произойдет, только если в функцию передать массив размера 3 или меньше\. Если же в функцию _blIsCubicFlat_ передать в качестве аргумента _p_ массив размера 4 или более, то выхода за границу массива не произойдёт и код будет работать стабильно\. Я посмотрел на [вызов функции _blIsCubicFlat_](https://github.com/blend2d/blend2d/blob/c4847906ea9423fe365ccafcaff62a37df5de3aa/src/blend2d/pathstroke.cpp) и понял, что как раз массив размера 4 в эту функцию и передаётся\. Значит, ошибку допустили в сигнатуре функции, а именно опечатались в значении размера массива\. ## Лишнее вычисление из\-за неправильного оператора V792 The '\_isTagged' function located to the right of the operator '&' will be called regardless of the value of the left operand\. Perhaps, it is better to use '&&'\. style\.h 209 ```cpp BL_NODISCARD BL_INLINE bool isObject() const noexcept { return (data.type > BL_STYLE_TYPE_SOLID) & _isTagged(); } ``` В данном фрагменте кода анализатор предлагает использовать логическое && вместо побитового &\. Дело в том, что при использовании побитого & оба его аргумента будут вычисляться вне зависимости от того, какие значения получатся\. Например, если проверка _\(data\.type \> BL\_STYLE\_TYPE\_SOLID\) _ложная, то результатом побитого & будет 0 при любом значении второго аргумента\. Однако функция _\_isTagged_ всё равно будет вызвана\. Если проверка _\(data\.type \> BL\_STYLE\_TYPE\_SOLID\) _ложная, то результатом логического && также будет 0, независимо от второго аргумента\. И здесь уже функция _\_isTagged_ не будет вызвана\. Осталось выяснить, а хотим ли мы вызывать функцию _\_isTagged_ всегда или только когда это нужно для вычисления результата? Ведь эта функция может иметь побочные эффекты, которые, возможно, мы хотим получить вне зависимости от результата вычислений\. Чтобы это понять, я посмотрел на код функции _\_isTagged_: ```cpp BL_NODISCARD BL_INLINE bool _isTagged(uint32_t styleType) const noexcept { ``` Как видно по сигнатуре функции, _\_isTagged_ имеет модификатор _const_, а значит, побочных эффектов у этой функции нет\. Таким образом, использование логического && вместо побитого & в данном фрагменте кода позволит не делать лишний вызов функции и уменьшит время выполнения программы\. ## Лишняя проверка V595 \[CERT\-EXP12\-C\] The '\_threadPool' pointer was utilized before it was verified against nullptr\. Check lines: 158, 164\. rasterworkermanager\.cpp 158 ```cpp class BLRasterWorkerManager { public: BLThreadPool* _threadPool; uint32_t _workerCount; // .... } // .... void BLRasterWorkerManager::reset() noexcept { // .... if (_workerCount) { // .... _threadPool->releaseThreads(_workerThreads, _workerCount); _workerCount = 0; // .... } if (_threadPool) { _threadPool->release(); _threadPool = nullptr; } // .... } ``` Указатель _\_threadPool_ разыменовывается, а затем ниже проверяется на равенство _nullptr_\. Вопрос: это ошибка или просто лишняя проверка\. Давайте попробуем разобраться\. Рассмотрев код, я пришёл к выводу, что это лишняя проверка и код можно немного упростить\. Дело в том, что для класса _BLRasterWorkerManager_ выполняется следующий инвариант: указатель _\_threadPool_ нулевой, только когда поле _\_workerCount_ равно 0\. Кроме метода _reset_, поля _workerCount_ и _\_threadPool_ модифицируются в двух местах: в конструкторе и в методе _init_\. Начнём с конструктора: ```cpp BL_INLINE BLRasterWorkerManager() noexcept : // .... _threadPool(nullptr), // .... _workerCount(0), // .... {} ``` Здесь всё просто: полю _\_workerCount_ присваивается значение 0, а указателю _\_threadPool_ – _nullptr_\. Инвариант, очевидно, выполняется\. С методом _init_ всё сложнее: ```cpp BLResult BLRasterWorkerManager::init(....) noexcept { // .... uint32_t workerCount = threadCount - 1; // .... if (workerCount) { // .... BLThreadPool* threadPool = nullptr; if (initFlags & BL_CONTEXT_CREATE_FLAG_ISOLATED_THREAD_POOL) { threadPool = blThreadPoolCreate(); if (!threadPool) return blTraceError(BL_ERROR_OUT_OF_MEMORY); } else { threadPool = blThreadPoolGlobal(); } // .... uint32_t n = threadPool->acquireThreads(workerThreads, workerCount, acquireThreadFlags, &reason); // .... if (!n) { threadPool->release(); threadPool = nullptr; // .... } // .... _threadPool = threadPool; // .... _workerCount = n; } else { // .... } } ``` В этом методе сначала вычисляется значение локальной переменной _workerCount_ \(не путать с полем _\_workerCount_\!\)\. Далее если её значение 0, то исполняется else\-ветка, в которой оба интересующих нас поля не изменяются\. Поэтому рассмотрим только случай, когда _workerCount_ не равна 0 и выполняется then\-ветка\. В этом случае указатель _threadPool_ \(не путать с _\_threadPool_\!\) сначала занулится\. Потом, в зависимости от некоторого условия, он инициализируется результатом вызова одной из двух функций: _blThreadPoolCreate_ или _blThreadPoolGlobal_\. Если вызвалась _blThreadPoolCreate_ и вернула _nullptr_, то вызовется no\-return функция _blTraceError_, и дальнейшее исполнение нам неинтересно\. Функция _blThreadPoolGlobal_ выглядит так: ```cpp static BLWrap blGlobalThreadPool; BLThreadPool* blThreadPoolGlobal() noexcept { return &blGlobalThreadPool; } ``` Значит, функция _blThreadPoolGlobal_ возвращает ненулевой указатель\. Итого: либо управление будет утеряно, либо указатель _threadPool_ является ненулевым\. Идём дальше: ```cpp uint32_t n = threadPool->acquireThreads(workerThreads, workerCount, acquireThreadFlags, &reason); ``` Здесь в переменную _n_ записывается значение потоков, которое удалось захватить\. Значение может быть как ненулевым, так и нулевым\. Если _n_ равно 0, то указатель _threadPool_ зануляется\. Указатель _\_threadPool_ также зануляется, полю _\_workerCount_ присваивается значение переменной _n_, равное 0\. Итого: _\_threadPool \= nullptr, \_workerCount \= 0\._ В этом случае, как видим, инвариант выполняется\. Пусть теперь _n_ не равно 0\. Тогда указатель _threadPool_ остаётся ненулевым, его значение записывается в указатель _\_threadPool_\. Полю _\_workerCount_ присваивается ненулевое значение _n_\. Итого: _\_threadPool _не равен_ nullptr, \_workerCount _не равен_ 0\._ В этом случае, как видим, инвариант тоже выполняется\. Итак, инвариант действительно выполняется, и мы можем использовать его и сказать, что проверки _\(\_workerCount\)_ и _\(\_threadPool\)_ одновременно или обе истинные, или обе ложные\. Значит, код можно упростить, объединив две проверки в одну, например так: ```cpp void BLRasterWorkerManager::reset() noexcept { // .... if (_workerCount) { assert(_threadPool); for (uint32_t i = 0; i < _workerCount; i++) _workDataStorage[i]->~BLRasterWorkData(); _threadPool->releaseThreads(_workerThreads, _workerCount); _workerCount = 0; _workerThreads = nullptr; _workDataStorage = nullptr; _threadPool->release(); _threadPool = nullptr; } // .... } ``` ## Использование неинициализированной переменной V573 \[CERT\-EXP53\-CPP\] Uninitialized variable 'n' was used\. The variable was used to initialize itself\. pixelconverter\.cpp 2210 ```cpp static BLResult BL_CDECL bl_convert_multi_step(...., uint32_t w, ....) { for (uint32_t y = h; y; y--) { uint32_t i = w; workOpt.origin.x = baseOriginX; dstData = dstLine; srcData = srcLine; while (i) { uint32_t n = blMin(n, intermediatePixelCount); srcToIntermediate(&ctx->first, intermediateData, 0, srcData, srcStride, n, 1, nullptr); intermediateToDst(&ctx->second, dstData, dstStride, intermediateData, 0, n, 1, &workOpt); dstData += n * dstBytesPerPixel; srcData += n * srcBytesPerPixel; workOpt.origin.x += int(n); i -= n; } } ``` Здесь анализатор выдал предупреждение на строчку _uint32\_t n \= blMin\(n, intermediatePixelCount\);_\. Действительно, довольно странно при объявлении переменной использовать её неинициализированное значение\. Похоже, что программист хотел написать вот так: _uint32\_t n \= blMin\(i, intermediatePixelCount\);_\. Это выглядит логично, потому что переменная _i_ модифицируется в цикле, а также используется в условии остановки цикла\. ## Всегда истинная проверка [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'x \>\= 5' is always true\. pngcodec\.cpp 588 ```cpp static void blPngDeinterlaceBits(....) noexcept { // .... uint32_t x = w; // .... switch (n) { case 2: { // .... if (x <= 4) break; if (x >= 5) b = uint32_t(*d5++); // .... } // .... } // .... } ``` Допустим, значение переменной _n_ равно 2 и мы зашли в соответствующую ветку switch\. Если при этом значение переменной _x_ меньше _5_, то произойдёт выход из цикла\. Значит, проверка _x \>\= 5_ всегда истинна\. Сложно сказать, в чём именно здесь ошибка\. Возможно, эта проверка лишняя и её нужно просто убрать\. Может быть, здесь нужно было сравнить переменную _x_ c другим значением\. Вот одно из возможных исправлений: ```cpp static void blPngDeinterlaceBits(....) noexcept { .... uint32_t x = w; .... switch (n) { case 2: { // .... if (x <= 4) break; b = uint32_t(*d5++); // .... } // .... } // .... } ``` ## Ошибка копипасты [V524](https://pvs-studio.ru/ru/docs/warnings/v524/) It is odd that the body of 'end' function is fully equivalent to the body of 'begin' function\. string\.h 258 ```cpp class BLString : public BLStringCore { public: // .... BL_NODISCARD BL_INLINE const char* begin() const noexcept { return impl->data + impl->size; } BL_NODISCARD BL_INLINE const char* end() const noexcept { return impl->data + impl->size; } // .... } ``` Здесь налицо ошибка копипасты\. Когда программист реализовывал метод _begin_, он скопировал метод _end_ и забыл изменить тело метода\. Исправленный вариант: ```cpp BL_NODISCARD BL_INLINE const char* begin() const noexcept { return impl->data; } ``` Предвижу возражение от внимательных читателей: "Но, подождите, как же так? Мы же обычно пишем код сверху вниз, так почему же вы утверждаете, что скопирован был именно метод end и переименован в begin, а не наоборот?"\. Такое возражение звучит вполне логично, поэтому предлагаю вашему вниманию небольшое расследование данного предупреждения анализатора\. Во\-первых, у класса _BLString_ есть метод _data_, который выглядит вот так: ```cpp BL_NODISCARD BL_INLINE const char* data() const noexcept { return impl->data; } ``` И вот сколько раз он используется: ![0894_Blend2d_ru/image3.png](https://import.viva64.com/docx/blog/0894_Blend2d_ru/image3.png) В то же время метод _begin_ не используется совсем: ![0894_Blend2d_ru/image5.png](https://import.viva64.com/docx/blog/0894_Blend2d_ru/image5.png) Во\-вторых, вот такой комментарий я нашел перед методом _begin_: ```cpp //! Returns a pointer to the beginning of string data (iterator compatibility) ``` Теперь, когда все улики предъявлены, расскажу свою версию произошедшего\. Сначала у класса _BLString_ были методы _data_ и _end_, они и использовались\. Всё прекрасно работало, и все были счастливы, но тут разработчики Blend2d задумались об _iterator compatibility\._ В частности, чтобы заработал такой код: ```cpp BLString str; for( auto symb : str ) { .... } ``` нужно, чтобы у класса _BLString_ были методы _begin_ и _end_\. Поэтому разработчики пошли и написали недостающий метод _begin_\. Конечно, логично было скопировать метод _data_, который делает то же самое, что и _begin_\. Но, когда программист занимается поддержкой _iterator compatibility_, он вообще не думает о методе _data_\. Этот метод тут ни при чём\. Зато разработчик думает о методе _end_\. Он тоже необходим для _iterator compatibility_, и он уже реализован\. Так почему же не скопировать его? Так и сделали, забыли поменять тело и получили ошибку\. К чему это приведет? Скорее всего, напрямую метод _begin_ вызываться не будет, вместо него по\-прежнему будут использовать метод _data_\. В то же время range\-based цикл for \(пример рассмотрен выше\) по\-прежнему не будет работать\. Код будет компилироваться, но итераций по строке не будет\. ## Ещё одна ошибка копипасты V523 The 'then' statement is equivalent to the 'else' statement\. pixelconverter\.cpp 1215 ```cpp template static BLResult BL_CDECL bl_convert_argb32_from_prgb_any(....) { for (uint32_t y = h; y != 0; y--) { if (!AlwaysUnaligned && blIsAligned(srcData, PixelAccess::kSize)) { for (uint32_t i = w; i != 0; i--) { uint32_t pix = PixelAccess::fetchA(srcData); uint32_t r = (((pix >> rShift) & rMask) * rScale) >> 16; uint32_t g = (((pix >> gShift) & gMask) * gScale) >> 8; uint32_t b = (((pix >> bShift) & bMask) * bScale) >> 8; uint32_t a = (((pix >> aShift) & aMask) * aScale) >> 24; BLPixelOps::unpremultiply_rgb_8bit(r, g, b, a); blMemWriteU32a(dstData, (a << 24) | (r << 16) | (g << 8) | b); dstData += 4; srcData += PixelAccess::kSize; } } else { for (uint32_t i = w; i != 0; i--) { uint32_t pix = PixelAccess::fetchA(srcData); uint32_t r = (((pix >> rShift) & rMask) * rScale) >> 16; uint32_t g = (((pix >> gShift) & gMask) * gScale) >> 8; uint32_t b = (((pix >> bShift) & bMask) * bScale) >> 8; uint32_t a = (((pix >> aShift) & aMask) * aScale) >> 24; BLPixelOps::unpremultiply_rgb_8bit(r, g, b, a); blMemWriteU32a(dstData, (a << 24) | (r << 16) | (g << 8) | b); dstData += 4; srcData += PixelAccess::kSize; } } // .... } } ``` Ещё один пример ошибки копипасты\. В данном фрагменте кода then и else\-ветки полностью совпадают\. Ясно, что программист забыл поменять код одной из веток, однако предложить исправление в данном случае довольно сложно\. ## Идемпотентный цикл [V1044](https://pvs-studio.ru/ru/docs/warnings/v1044/) Loop break conditions do not depend on the number of iterations\. otcmap\.cpp 59 ```cpp #if defined(__GNUC__) #define BL_LIKELY(...) __builtin_expect(!!(__VA_ARGS__), 1) #define BL_UNLIKELY(...) __builtin_expect(!!(__VA_ARGS__), 0) #else #define BL_LIKELY(...) (__VA_ARGS__) #define BL_UNLIKELY(...) (__VA_ARGS__) #endif .... static BLResult BL_CDECL mapTextToGlyphsFormat0(....) noexcept { // .... uint32_t* ptr = content; uint32_t* end = content + count; // .... while (ptr != end) { uint32_t codePoint = content[0]; uint32_t glyphId = codePoint < 256 ? uint32_t(glyphIdArray[codePoint].value()) : uint32_t(0); content[0] = glyphId; if (BL_UNLIKELY(glyphId == 0)) { if (!undefinedCount) state->undefinedFirst = (size_t)(ptr - content); undefinedCount++; } } // .... } ``` В этом фрагменте кода может произойти зацикливание\. Переменные _ptr_ и _end_ не изменяются внутри цикла\. Поэтому если условие _ptr \!\= end_ было верным, то получится бесконечный цикл\. Похоже, что программист забыл добавить инкремент указателя _ptr_\. Я предлагаю такое исправление: ```cpp while (ptr != end) { uint32_t codePoint = content[0]; uint32_t glyphId = codePoint < 256 ? uint32_t(glyphIdArray[codePoint].value()) : uint32_t(0); content[0] = glyphId; if (BL_UNLIKELY(glyphId == 0)) { if (!undefinedCount) state->undefinedFirst = (size_t)(ptr - content); undefinedCount++; } ++ptr; } ``` На этот цикл указывает ещё одно предупреждение анализатора: [V776](https://pvs-studio.ru/ru/docs/warnings/v776/) Potentially infinite loop\. The variable in the loop exit condition 'ptr \!\= end' does not change its value between iterations\. otcmap\.cpp 59 ## Заключение Безусловно, удалось найти не так много ошибок по сравнению с нашей привычной проверкой проекта, содержащего под миллион строк кода\. В этом плане ожидания оправдались\. Тем не менее, ошибки нашлись и очень даже интересные\. Что из этого следует? Во\-первых, даже в небольших проектах ошибки есть\. А значит, их нужно находить и исправлять :\) Во\-вторых, маленький размер кодовой базы не является гарантией того, что все ошибки будут найдены в процессе code review\. Порой даже после нескольких просмотров небольшого фрагмента кода на 20 строк не удаётся выявить досадную опечатку\. В то же время инструменты статического анализа не подвержены усталости и невнимательности\. Статический анализатор готов искать ошибки в коде в любое время суток\. Ему не нужен перерыв\. И главное – от его всевидящего ока не спрячется ни одна опечатка в коде\! Если вас заинтересовал статический анализ в целом и PVS\-Studio в частности – самое время его попробовать\. Для этого достаточно [скачать бесплатную версию](https://pvs-studio.ru/ru/pvs-studio/try-free/?promo=blend2d_article) анализатора\. Спасибо за внимание\!