# Even small projects have bugs, or how PVS\-Studio checked Blend2D We often check large projects because it's easier to find bugs there\. What if we try PVS\-Studio on a small project? In this article we analyze Blend2D — a library for vector 2D graphics\. Let's look at what we found\. ![0894_Blend2d/image1.png](https://import.viva64.com/docx/blog/0894_Blend2d/image1.png) ## Introduction It's no secret that large projects have fascinating errors\. It's not just "the larger the codebase is – the more errors we can find"\. It's also a known fact that [the density of errors grows](https://pvs-studio.com/en/blog/posts/0158/) along with the codebase\. That's why we love checking large projects — to treat you with a variety of "yummy" and tricky errors and typos\. Besides, it's always interesting to search through a huge project with lots of dependencies, legacy code, and other stuff\. Today I'm moving away from this tradition\. I decided to take a small project and see what PVS\-Studio can find there\. I chose Blend2D — branch _master_, commit [c484790](https://github.com/blend2d/blend2d/tree/c4847906ea9423fe365ccafcaff62a37df5de3aa)\. ## Blend2D Blend2D is a 2D vector graphics engine\. This small library written in C\+\+ contains about 70,000 lines of code: ```cpp --------------------------------------------------------------------- Language files blank comment code --------------------------------------------------------------------- C++ 97 12924 9481 43372 C/C++ Header 137 8305 12971 25225 ``` This library allows you to create 2D images\. To achieve high performance, the library developers used multithreaded rendering and a self\-written rasterizer\. Blend2D provides C and C\+\+ API\. You can read more about the project and capabilities of this library on the [website](https://blend2d.com/)\. Now let's proceed to the errors that PVS\-Studio found in the Blend2D source code\. ## An always\-false expression [V547](https://pvs-studio.com/en/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); // <= // .... } ``` In this code fragment, the result of the _blMemReadU16uBE_ function call is assigned to the _h_ variable\. Then if the _h \=\= 0_ check is true, we exit from the function's body\. During initialization _impl\-\>delayedHeight_, the _h_ variable has non\-zero value\. Thus, _impl\-\>delayedHeight_ is _false_\. ## A typo in the function's signature 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]) { // .... } // .... } ``` In the signature of the _blIsCubicFlat_ function, the _p_ variable is declared as an array of 3 elements\. Then, _p\[3\]_ is calculated in the body of the _blMemReadU16uBE_ function\. Declaring the _const BLPoint p\[3\]_ argument in the function's signature equals declaring _const BLPoint \*p_\. The specified size is a hint to the developer\. The compiler doesn't use the size in any way\. Thus, array index out of bounds happens only if we pass an array of 3 or fewer elements to the function\. If _blIsCubicFlat_ receives an array of 4 elements or more, there is no array index out of bounds and the code works in a defined way\. I looked at the [_blIsCubicFlat_ function call](https://github.com/blend2d/blend2d/blob/c4847906ea9423fe365ccafcaff62a37df5de3aa/src/blend2d/pathstroke.cpp) and realized that the array of 4 elements is passed to this function\. This means that there's a mistake in the function's signature — a typo in the value of the array size\. ## An extra evaluation due to an incorrect operator 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(); } ``` Here the analyzer suggests using the && logical operator instead of bitwise &\. The thing is, when we use bitwise &, both of its arguments are calculated regardless of what values are obtained\. For example, if _\(data\.type \> BL\_STYLE\_TYPE\_SOLID\)_ is false, bitwise & returns 0 for any value of the second argument\. However, the _\_isTagged_ function is called anyway\. If _\(data\.type \> BL\_STYLE\_TYPE\_SOLID\)_ is false, then the result of the && logical operator is also 0, regardless of the second argument\. Here the _\_isTagged_ function is not called\. The only question is, do we want to call the _\_isTagged_ function always or only when it is necessary to calculate the result? This function may have some side effects, which we may want to use regardless of the calculation\. To answer this question, I looked at the _\_isTagged_ function code: ```cpp BL_NODISCARD BL_INLINE bool _isTagged(uint32_t styleType) const noexcept { ``` As you see from the function's signature, _\_isTagged_ has the _const_ modifier\. This means that the function has no side effects\. Thus, using logical && instead of bitwise & in this code fragment allows us to avoid an unnecessary function call and reduces the program's execution time\. ## A redundant check 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; } // .... } ``` The _\_threadPool_ pointer is dereferenced and then it's checked for _nullptr_\. The question is: is it an error or just a redundant check? Let's try to figure it out\. When I examined the code, I realized the check was indeed redundant\. We can simplify the code a bit\. The following invariant is executed for the _BLRasterWorkerManage_ class: the _\_threadPool_ pointer is null only when the _\_workerCount_ field equals 0\. Besides the _reset_ method, fields _workerCount_ and _\_threadPool_ are modified in two places: in the constructor and in the _init_ method\. Let's start with the constructor: ```cpp BL_INLINE BLRasterWorkerManager() noexcept : // .... _threadPool(nullptr), // .... _workerCount(0), // .... {} ``` Everything is easy here: we assign 0 to the _\_workerCount_ field, and _nullptr_ to the _\_threadPool_ pointer\. Invariant is obviously executed\. Not so easy with the _init_ method: ```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 { // .... } } ``` First, we calculate the value of the _workerCount_ local variable\. Don't confuse it with the _\_workerCount_ field\! If the variable's value is 0, then the else branch is executed\. In this branch, both fields remain unchanged\. So, we'll look only at the case where _workerCount_ is not equal to 0 and the then branch is executed\. In this case, first, the _threadPool_ pointer \(not _\_threadPool_\!\) becomes equal to 0\. Then, depending on a condition, this pointer is initialized by the result of calling either _blThreadPoolCreate_ or _blThreadPoolGlobal_\. If it's the _blThreadPoolCreate_ function and it returns _nullptr_, then the no\-return _blTraceError_ function is called\. We are not interested in the further execution\. The _blThreadPoolGlobal_ function looks like this: ```cpp static BLWrap blGlobalThreadPool; BLThreadPool* blThreadPoolGlobal() noexcept { return &blGlobalThreadPool; } ``` This means that the _blThreadPoolGlobal_ function returns a non\-null pointer\. Consequently, either we lose control over the code, or the _threadPool_ pointer is not null\. Let's go further: ```cpp uint32_t n = threadPool->acquireThreads(workerThreads, workerCount, acquireThreadFlags, &reason); ``` Here, the value of the threads acquired is written to the _n_ variable\. The value may or may not be zero\. If _n_ equals 0, the _threadPool_ pointer is nulled\. The _\_threadPool_ pointer is also nulled, the _\_workerCount_ field is assigned the value of the _n_ variable — 0\. As a result: _\_threadPool \= nullptr, \_workerCount \= 0\._ In this case, the invariant is true\. Now let's assume _n_ is not 0\. In this case, the _threadPool_ pointer remains non\-null and its value is written to the _\_threadPool_ pointer\. The _\_workerCount_ field is assigned non\-zero value of _n_\. As a result: _\_threadPool_ is not equal to _nullptr; \_workerCount_ is not equal to_ 0\._ In this case the invariant is also true\. So, the invariant is really true\. We can use it and say that checks _\(\_workerCount\)_ and _\(\_threadPool\)_ are always both true or both false\. So, we can simplify the code by combining two checks into one\. Like that, for example: ```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; } // .... } ``` ## Using an uninitialized variable 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; } } ``` The following line triggered the analyzer: _uint32\_t n \= blMin\(n, intermediatePixelCount\);_\. Agree, it's quite strange to declare a variable and use its uninitialized value\. Looks like the developer wanted to write something like this: _uint32\_t n \= blMin\(i, intermediatePixelCount\);_\. This looks better — the _i_ variable is modified in the loop and is also used in the condition of breaking the loop\. ## An always\-true check [V547](https://pvs-studio.com/en/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++); // .... } // .... } // .... } ``` Let's assume that the value of the _n_ variable is 2 and we go to the corresponding _switch_ branch\. If the value of the _x_ variable is less than _5_, the loop breaks\. This means that check _x \>\= 5_ is always true\. It's hard to say where the error is\. Maybe this check is redundant and we need to remove it\. Maybe the developer intended to compare _x_ with another value\. Here's one of the possible fixes: ```cpp static void blPngDeinterlaceBits(....) noexcept { .... uint32_t x = w; .... switch (n) { case 2: { // .... if (x <= 4) break; b = uint32_t(*d5++); // .... } // .... } // .... } ``` ## A copy\-paste error [V524](https://pvs-studio.com/en/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; } // .... } ``` Obviously, a copy\-paste error\. When a developer implemented the _begin_ method, they copied the _end_ method and forgot to change the method's body\. Corrected version: ```cpp BL_NODISCARD BL_INLINE const char* begin() const noexcept { return impl->data; } ``` I suppose the readers have a question: "Wait, how did it happen? We usually write code from top to bottom\. Why do you claim that the end method was copied and renamed into begin, and not vice versa?" This question is quite logical, so I present a small investigation of this warning\. First, the _BLString_ has the _data_ method\. It looks like this: ```cpp BL_NODISCARD BL_INLINE const char* data() const noexcept { return impl->data; } ``` And look at how many times it's used: ![0894_Blend2d/image3.png](https://import.viva64.com/docx/blog/0894_Blend2d/image3.png) At the same time the _begin_ method is not used at all: ![0894_Blend2d/image5.png](https://import.viva64.com/docx/blog/0894_Blend2d/image5.png) Second, I found the following comment before the _begin_ method: ```cpp //! Returns a pointer to the beginning of string data (iterator compatibility) ``` Now when we found all the evidence, let me tell you what happened\. The _BLString_ class had the _data_ and _end_ methods\. Everything was great\. But then the Blend2D developers thought about _iterator compatibility\._ In particular, they wanted to make the following fragment work: ```cpp BLString str; for( auto symb : str ) { .... } ``` The _BLString_ class needed to have methods _begin_ and _end_\. So, the developers wrote the missing _begin_ method\. It's more logical to copy the _data_ method\. It does the same thing as _begin_\. But when developers support _iterator compatibility_, they don't think about the _data_ method at all\. This method has nothing to do with it\. Developers think about the _end_ method\. They need it for _iterator compatibility_, and it's already implemented\. So why not copy it? They did copy it, they forgot to change the body, and they got an error\. What does it lead to? Most likely, the _begin_ method is not called directly, the _data_ method is used instead\. At the same time, the range\-based _for_ loop \(the example above\) still doesn't work\. The code is compiled but does not iterate through the string\. ## Another copy\-paste error 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; } } // .... } } ``` Another example of a copy\-paste error\. In this code fragment, branches _else_ and _then_ are completely identical\. Obviously, the developer forgot to change the code on one of the branches, but I can't offer any fix here\. ## An idempotent loop [V1044](https://pvs-studio.com/en/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++; } } // .... } ``` This code fragment may cause looping\. Variables _ptr_ and _end_ don't change within the loop\. If condition _ptr \!\= end_ is true, we get an infinite loop\. Looks like the developer forgot to add the _ptr_ pointer increment\. We can fix the code like this: ```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; } ``` The analyzer issued another warning for this loop: [V776](https://pvs-studio.com/en/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 ## Conclusion Of course, this project doesn't have as many errors as large projects with about a million code lines\. But we expected that\. However, this project has some impressive errors\. What does this mean? First, even small projects have errors\. Which means, we need to find them and fix them :\) Second, a small codebase is not a guarantee that all errors will be found during code review\. Sometimes developers miss an error after reading the code several times\. But static analysis tools don't miss them\. A static analyzer is ready to search for errors in code at any time of the day\. It doesn't need to rest\. And most importantly — its all\-seeing eye spies every typo in code\! If you are interested in static analysis and PVS\-Studio \- it's high time to try it\. Just download a [free version](https://pvs-studio.com/en/pvs-studio/try-free/?promo=blend2d_article) of the analyzer\. Thank you for reading\!