Александр Куренев

03 Дек 2021

Теги:

#Cpp

Есть ли баги в маленьких проектах, или как PVS-Studio проверял Blend2D

03 Дек 2021

Автор: Александр Куренев

Введение
Blend2D
Всегда ложное выражение
Опечатка в сигнатуре функции
Лишнее вычисление из-за неправильного оператора
Лишняя проверка
Использование неинициализированной переменной
Всегда истинная проверка
Ошибка копипасты
Ещё одна ошибка копипасты
Идемпотентный цикл
Заключение

Мы часто проверяем большие проекты, потому что в них проще найти ошибки. А что же PVS-Studio сможет найти в небольшом проекте? Мы взяли Blend2D – библиотеку для векторной 2D-графики – и проверили своим анализатором. Предлагаем ознакомиться с тем, что из этого вышло.

Введение

Ни для кого не секрет, что в больших проектах всегда легче находить интересные ошибки. Это обусловлено не только простым рассуждением "больше кода – больше ошибок", но и тем, что с ростом кодовой базы растёт и плотность ошибок. Поэтому и проверять мы любим большие проекты, чтобы потом порадовать себя и читателя разнообразием "жирнющих" и заковыристых ошибок и опечаток. Да и в целом всегда интересно поковыряться в огромном проекте с кучей зависимостей, legacy и прочих вкусняшек.

В связи с этим наблюдением, я захотел отойти от нашей традиции брать большие проекты для статьи. Решил взять небольшой проект и посмотреть, что в нём сможет найти PVS-Studio. Мой выбор пал на Blend2D. Я проверил ветку master, коммит c484790.

Blend2D

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 в частности – самое время его попробовать. Для этого достаточно скачать бесплатную версию анализатора. Спасибо за внимание!

#Cpp

Теги:

#Cpp

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Тип желаемой лицензии:

Есть ли баги в маленьких проектах, или как PVS-Studio проверял Blend2D

Введение

Blend2D

Всегда ложное выражение

Опечатка в сигнатуре функции

Лишнее вычисление из-за неправильного оператора

Лишняя проверка

Использование неинициализированной переменной

Всегда истинная проверка

Ошибка копипасты

Ещё одна ошибка копипасты

Идемпотентный цикл

Заключение

Комментарии (0)

Хотите получить ключ на использование анализатора в пробный период?

Достижения

PVS-Studio

Лицензирование

Компания