Мы часто проверяем большие проекты, потому что в них легче найти ошибки.
Что может найти PVS-Studio в маленьком проекте? Мы взяли Blend2D — библиотеку для 2D-векторной графики — и проверили ее с помощью нашего анализатора.
Предлагаем вам ознакомиться с тем, что из этого получилось.
Введение
Ни для кого не секрет, что в больших проектах всегда легче найти интересные ошибки.Это связано не только с простым рассуждением «больше кода — больше ошибок», но и с тем, что по мере роста кодовой базы плотность ошибок также увеличивается .
Именно поэтому мы любим проверять большие проекты, чтобы потом порадовать себя и читателя разнообразными «жирными» и каверзными ошибками и опечатками.
Да и вообще, всегда интересно поковыряться в огромном проекте с кучей зависимостей, наследства и прочих вкусностей.
В связи с этим наблюдением мне захотелось отойти от нашей традиции браться за большие проекты для статьи.
Я решил взять небольшой проект и посмотреть, что в нем сможет найти PVS-Studio. Мой выбор пал на Blend2D. Я проверил главную ветку, зафиксировал c484790.
Бленд2D
Blend2D — это движок векторной 2D-графики.Это небольшая библиотека, написанная на C++ и содержащая около 70 000 строк кода:
Эта библиотека позволяет создавать любые 2D изображения.--------------------------------------------------------------------- Language files blank comment code --------------------------------------------------------------------- C++ 97 12924 9481 43372 C/C++ Header 137 8305 12971 25225
Для достижения высокой производительности разработчики проекта используют многопоточный рендеринг и собственный растеризатор.
Blend2D предоставляет API C и C++.
Подробнее о проекте и возможностях этой библиотеки вы можете узнать на их странице.
Веб-сайт .
Перейду к ошибкам, которые PVS-Studio обнаружил в исходном коде Blend2D.
Всегда ложное выражение
В547 Выражение 'h == 0' всегда ложно.
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); // <= // .
}
В этом фрагменте кода для переменной час присваивается результат вызова функции blMemReadU16uBE .
Тогда если проверить ч == 0 оказывается правдой, функция завершается.
Это означает, что при инициализации impl-> delayedHeight в переменной час есть ненулевое значение.
Поэтому в impl-> delayedHeight будет назначен ЛОЖЬ .
Опечатка в сигнатуре функции
V557 [CERT-ARR30-C] Возможно переполнение массива.Индекс «3» указывает за пределы массива.
геометрия_p.h 552 static BL_INLINE bool blIsCubicFlat(const BLPoint p[3], double f) {
if (p[3] == p[0]) {
// .
} // .
}
В этом фрагменте кода в сигнатуре функции blIКубическийКвартира переменная п объявляется как массив размером 3. И затем в теле функции blMemReadU16uBE рассчитанный п[3] .
В общем, объявляя аргумент константа BLPoint p[3] в сигнатуре функции эквивалентно объявлению const BLPoint *p , а указание размера является подсказкой для программиста и никак не используется компилятором.
Следовательно, переполнение массива произойдет только в том случае, если в функцию будет передан массив размером 3 или меньше.
Если в функции blIКубическийКвартира передать как аргумент п Если массив имеет размер 4 и более, то массив не выйдет за границы и код будет работать стабильно.
я посмотрел на вызов функции blIКубическийКвартира и понял, что этой функции передается массив размером 4. Это означает, что в сигнатуре функции допущена ошибка, а именно опечатка в значении размера массива.
Ненужный расчет из-за неправильного оператора
V792 Функция «_isTagged», расположенная справа от оператора «&», будет вызываться независимо от значения левого операнда.Возможно, лучше использовать «&&».
стиль.
h 209 BL_NODISCARD BL_INLINE bool isObject() const noexcept
{
return (data.type > BL_STYLE_TYPE_SOLID) & _isTagged();
}
В этом фрагменте кода анализатор предлагает использовать логическое && вместо побитового &.
Дело в том, что при использовании ломаного & оба его аргумента будут оценены независимо от того, какие значения будут получены.
Например, если проверить (тип данных > BL_STYLE_TYPE_SOLID) false, то результат сломанного & будет 0 для любого значения второго аргумента.
Однако функция _isTagged еще позвонят. Если проверить (тип данных > BL_STYLE_TYPE_SOLID) false, то результат логического && также будет равен 0, независимо от второго аргумента.
И вот функция _isTagged не будет вызван.
Еще неизвестно, хотим ли мы вызвать функцию _isTagged всегда или только тогда, когда нужно посчитать результат? Ведь у этой функции могут быть побочные эффекты, которые мы можем захотеть получить независимо от результата расчета.
Чтобы понять это, я посмотрел на код функции _isTagged : BL_NODISCARD BL_INLINE bool _isTagged(uint32_t styleType) const noexcept {
Как видно из сигнатуры функции, _isTagged имеет модификатор константа , что означает, что эта функция не имеет побочных эффектов.
Таким образом, использование логического && вместо ломаного & в этом фрагменте кода позволит избежать ненужного вызова функции и сократит время выполнения программы.
Дополнительная проверка
V595 [CERT-EXP12-C] Указатель «_threadPool» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 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 разыменовывается, а затем проверяется на равенство ниже нульптр .
Вопрос: это ошибка или просто ненужная проверка.
Давайте попробуем разобраться.
Просмотрев код, я пришел к выводу, что это лишняя проверка и код можно было бы немного упростить.
Дело в том, что для класса Блрастерворкерменеджер выполняется следующий инвариант: указатель _threadPool ноль только тогда, когда поле _workerCount равно 0. Помимо метода перезагрузить , поля WorkCount И _threadPool изменяются в двух местах: в конструкторе и в методе в этом .
Начнем с конструктора: BL_INLINE BLRasterWorkerManager() noexcept
: // .
_threadPool(nullptr), // .
_workerCount(0), // .
{}
Здесь все просто: поле _workerCount присваивается значение 0, а указатель _threadPool – нульптр .
Инвариант, очевидно, выполнен.
С методом в этом все сложнее: 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 { // .
}
}
В этом методе сначала вычисляется значение локальной переменной.
WorkCount (не путать с полем _workerCount !).
Далее, если его значение равно 0, то выполняется ветка else, в которой оба интересующих нас поля не изменяются.
Поэтому мы будем рассматривать только случай, когда WorkCount не равно 0, и выполняется переход затем.
В этом случае указатель пул потоков (не путать с _threadPool !) будет обнулен первым.
Затем, в зависимости от некоторого условия, он инициализируется результатом вызова одной из двух функций: blThreadPoolCreate или blThreadPoolGlobal .
Если вы вызвались добровольцем blThreadPoolCreate и вернулся нульптр , то будет вызвана невозвратная функция блтрацееррор , и дальнейшее выполнение нас не интересует. Функция blThreadPoolGlobal выглядит так: static BLWrap<BLInternalThreadPool> blGlobalThreadPool;
BLThreadPool* blThreadPoolGlobal() noexcept { return &blGlobalThreadPool; }
Итак, функция blThreadPoolGlobal возвращает ненулевой указатель.
Итого: либо управление потеряется, либо указатель пул потоков не равно нулю.
Вперед, продолжать: uint32_t n = threadPool->acquireThreads(workerThreads, workerCount,
acquireThreadFlags, &reason);
Здесь в переменной н Значение захваченных потоков записывается.
Значение может быть ненулевым или нулевым.
Если н равен 0, то указатель пул потоков аннулируется.
Указатель _threadPool также обнулено, поле _workerCount значение присваивается переменной н , равное 0. Итого: _threadPool = nullptr, _workerCount = 0. В этом случае, как мы видим, инвариант выполняется.
Пусть это сейчас н не равен 0. Тогда указатель пул потоков остается ненулевым, его значение записывается в указатель _threadPool .
Поле _workerCount присвоено ненулевое значение н .
Общий: _threadPool не равный нульптр, _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] Использовалась неинициализированная переменная «n».Переменная использовалась для инициализации самой себя.
пиксельконвертер.
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, промежуточныйPixelCount); .
Действительно, довольно странно использовать при объявлении переменной ее неинициализированное значение.
Похоже программист хотел написать что-то вроде этого: uint32_t n = blMin(i, промежуточныйPixelCount); .
Это кажется логичным, поскольку переменная я модифицируется в цикле и также используется в условии остановки цикла.
Всегда настоящее испытание
В547 Выражение 'x > = 5' всегда истинно.
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++); // .
} // .
} // .
}
Допустим, значение переменной н равно 2 и мы пошли в соответствующую ветку коммутатора.
Если значение переменной Икс меньше 5 , то цикл завершается.
Так что проверьте х > = 5 всегда правда.
Трудно сказать, что именно здесь не так.
Возможно, эта проверка ненужна и ее просто нужно убрать.
Возможно, здесь следовало сравнить переменную Икс с другим смыслом.
Вот одно из возможных исправлений: static void blPngDeinterlaceBits(.
) noexcept { .
uint32_t x = w; .
switch (n) { case 2: { // .
if (x <= 4) break; b = uint32_t(*d5++); // .
} // .
} // .
}
Ошибка копирования-вставки
В524 Странно, что тело функции end полностью эквивалентно телу функции start. строка.
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; } // .
}
Здесь ошибка копирования-вставки.
Когда программист реализовал метод начинать , он скопировал метод конец и забыл изменить тело метода.
Исправленная версия: BL_NODISCARD BL_INLINE const char* begin() const noexcept
{
return impl->data;
}
Предвижу возражение со стороны внимательных читателей: «Но подождите, как же так? Обычно мы пишем код сверху вниз, так почему же вы утверждаете, что именно конечный метод был скопирован и переименован в начало, а не наоборотЭ» Это возражение звучит вполне логично, поэтому предлагаю вам небольшое расследование данного предупреждения анализатора.
Во-первых, класс BLString есть метод данные , который выглядит следующим образом: BL_NODISCARD
BL_INLINE const char* data() const noexcept { return impl->data; }
И вот сколько раз это использовалось: 
В то же время метод начинать вообще не используется: 
Во-вторых, я нашел этот комментарий перед методом начинать : //! Returns a pointer to the beginning of string data (iterator compatibility)
Теперь, когда все доказательства представлены, я расскажу вам свою версию произошедшего.
Сначала в классе BLString были методы данные И конец , их использовали.
Все работало отлично, и все были довольны, но тут разработчики Blend2d задумались о совместимость итераторов.
В частности, чтобы работал следующий код: BLString str;
for( auto symb : str ) { .
}
класс нуждается BLString были методы начинать И конец .
Вот разработчики пошли и написали недостающий метод начинать .
Конечно, логично было скопировать метод данные , который делает то же самое, что и начинать .
Но когда поддержкой занимается программист совместимость итератора , он вообще не думает о методе данные .
Этот метод не имеет к этому никакого отношения.
Но разработчик думает о методе конец .
Это также необходимо для совместимость итератора , и это уже реализовано.
Так почему бы не скопировать? Так и сделали, забыли сменить тело и вылезла ошибка.
Куда это ведет? Скорее всего, прямой метод начинать не будет вызываться, вместо этого метод все равно будет использоваться данные .
В то же время цикл for на основе диапазона (пример, рассмотренный выше) по-прежнему не будет работать.
Код скомпилируется, но не будет перебирать строку.
Очередная ошибка копирования
V523 Оператор then эквивалентен оператору else. пиксельконвертер.
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 полностью идентичны.
Понятно, что программист забыл изменить код одной из веток, но предложить исправление в этом случае довольно сложно.
Идемпотентная петля
В1044 Условия выхода из цикла не зависят от количества итераций.
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++; } } // .
}
В этом фрагменте кода может возникнуть цикл.
Переменные ПТР И конец не меняйте внутри цикла.
Следовательно, если условие ПТР != конец было правдой, это привело бы к бесконечному циклу.
Похоже, программист забыл добавить приращение указателя ПТР .
Я предлагаю это исправление: 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;
}
На этот цикл указывает еще одно предупреждение анализатора: В776 Потенциально бесконечный цикл.
Переменная в условии выхода цикла «ptr != end» не меняет свое значение между итерациями.
otcmap.cpp 59
Заключение
Конечно, нам удалось найти не так много ошибок по сравнению с обычной проверкой проекта, содержащего почти миллион строк кода.В этом плане ожидания оправдались.
Тем не менее, были и очень интересные ошибки.
Что из этого следует? Во-первых, даже в небольших проектах есть ошибки.
Значит их нужно найти и исправить :) Во-вторых, небольшой размер кодовой базы не является гарантией того, что в процессе проверки кода будут найдены все ошибки.
Иногда даже после нескольких просмотров небольшого 20-строчного фрагмента кода не удается обнаружить досадную опечатку.
В то же время инструменты статического анализа не подвержены усталости и невнимательности.
Статический анализатор готов искать ошибки в коде в любое время суток.
Ему не нужен перерыв.
А самое главное, ни одна опечатка в коде не может скрыться от его всевидящего ока! Если вас интересует статический анализ вообще и PVS-Studio в частности, сейчас самое время попробовать.
Достаточно для этого скачать бесплатную версию анализатор.
Спасибо за внимание! Если вы хотите поделиться этой статьей с англоязычной аудиторией, воспользуйтесь ссылкой для перевода: Александр Куренев.
Даже в небольших проектах есть ошибки, или как PVS-Studio проверял Blend2D .
Теги: #с открытым исходным кодом #c++ #C++ #статический анализатор кода #статический анализ #статический анализ кода #статический анализ #статический анализатор кода #статический анализатор
-
Социальная Инженерия На Phdays 2012
19 Oct, 24
