Мы часто проверяем большие проекты, потому что в них проще найти ошибки. А что же PVS-Studio сможет найти в небольшом проекте? Мы взяли Blend2D – библиотеку для векторной 2D-графики – и проверили своим анализатором. Предлагаем ознакомиться с тем, что из этого вышло.
Ни для кого не секрет, что в больших проектах всегда легче находить интересные ошибки. Это обусловлено не только простым рассуждением "больше кода – больше ошибок", но и тем, что с ростом кодовой базы растёт и плотность ошибок. Поэтому и проверять мы любим большие проекты, чтобы потом порадовать себя и читателя разнообразием "жирнющих" и заковыристых ошибок и опечаток. Да и в целом всегда интересно поковыряться в огромном проекте с кучей зависимостей, legacy и прочих вкусняшек.
В связи с этим наблюдением, я захотел отойти от нашей традиции брать большие проекты для статьи. Решил взять небольшой проект и посмотреть, что в нём сможет найти PVS-Studio. Мой выбор пал на Blend2D. Я проверил ветку master, коммит c484790.
Blend2D — это движок для векторной 2D-графики. Это небольшая библиотека, написанная на C++ и содержащая около 70'000 строк кода:
---------------------------------------------------------------------
Language files blank comment code
---------------------------------------------------------------------
C++ 97 12924 9481 43372
C/C++ Header 137 8305 12971 25225Эта библиотека позволяет создавать любые 2D-изображения. Чтобы достичь высокой производительности, разработчики проекта используют многопоточный рендеринг и самописный растеризатор. Blend2D предоставляет C и C++ API. Более подробно узнать про проект и возможности этой библиотеки вы можете на их сайте. Я же перейду к ошибкам, которые обнаружил PVS-Studio в исходном коде Blend2D.
V547 Expression 'h == 0' is always false. jpegcodec.cpp 252
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
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 и понял, что как раз массив размера 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
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:
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
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. Начнём с конструктора:
BL_INLINE BLRasterWorkerManager() noexcept
: // ....
_threadPool(nullptr),
// ....
_workerCount(0),
// ....
{}Здесь всё просто: полю _workerCount присваивается значение 0, а указателю _threadPool – nullptr. Инвариант, очевидно, выполняется.
С методом init всё сложнее:
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 выглядит так:
static BLWrap<BLInternalThreadPool> blGlobalThreadPool;
BLThreadPool* blThreadPoolGlobal() noexcept { return &blGlobalThreadPool; }Значит, функция blThreadPoolGlobal возвращает ненулевой указатель. Итого: либо управление будет утеряно, либо указатель threadPool является ненулевым. Идём дальше:
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) одновременно или обе истинные, или обе ложные. Значит, код можно упростить, объединив две проверки в одну, например так:
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
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 Expression 'x >= 5' is always true. pngcodec.cpp 588
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 другим значением. Вот одно из возможных исправлений:
static void blPngDeinterlaceBits(....) noexcept {
....
uint32_t x = w;
....
switch (n) {
case 2: {
// ....
if (x <= 4) break;
b = uint32_t(*d5++);
// ....
}
// ....
}
// ....
}V524 It is odd that the body of 'end' function is fully equivalent to the body of 'begin' function. string.h 258
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 и забыл изменить тело метода. Исправленный вариант:
BL_NODISCARD BL_INLINE const char* begin() const noexcept
{
return impl->data;
}Предвижу возражение от внимательных читателей: "Но, подождите, как же так? Мы же обычно пишем код сверху вниз, так почему же вы утверждаете, что скопирован был именно метод end и переименован в begin, а не наоборот?". Такое возражение звучит вполне логично, поэтому предлагаю вашему вниманию небольшое расследование данного предупреждения анализатора.
Во-первых, у класса BLString есть метод data, который выглядит вот так:
BL_NODISCARD
BL_INLINE const char* data() const noexcept { return impl->data; }И вот сколько раз он используется:
В то же время метод begin не используется совсем:
Во-вторых, вот такой комментарий я нашел перед методом begin:
//! Returns a pointer to the beginning of string data (iterator compatibility)Теперь, когда все улики предъявлены, расскажу свою версию произошедшего.
Сначала у класса BLString были методы data и end, они и использовались. Всё прекрасно работало, и все были счастливы, но тут разработчики Blend2d задумались об iterator compatibility. В частности, чтобы заработал такой код:
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
template<typename PixelAccess, bool AlwaysUnaligned>
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 Loop break conditions do not depend on the number of iterations. otcmap.cpp 59
#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. Я предлагаю такое исправление:
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 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 в частности – самое время его попробовать. Для этого достаточно скачать бесплатную версию анализатора. Спасибо за внимание!