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


## Введение

Ни для кого не секрет, что в больших проектах всегда легче находить интересные ошибки\. Это обусловлено не только простым рассуждением "больше кода – больше ошибок", но и тем, что с ростом кодовой базы [растёт и плотность ошибок](https://pvs-studio.com/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.com/ru/w/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<BLInternalThreadPool> 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.com/ru/w/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.com/ru/w/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; }

```

И вот сколько раз он используется:


В то же время метод *begin* не используется совсем:


Во\-вторых, вот такой комментарий я нашел перед методом *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<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](https://pvs-studio.com/ru/w/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.com/ru/w/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.com/blend2d) анализатора\. Спасибо за внимание\!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexander Kurenev. [Even small projects have bugs, or how PVS\-Studio checked Blend2D](https://pvs-studio.com/en/blog/posts/cpp/0894/).