# Top 10 bugs found in C\+\+ projects in 2022 New Year is coming\! It means, according to tradition, it's time to recall 10 of the most interesting warnings that PVS\-Studio found during 2022\. ![1021_Top_10_CPP_Bugs_2022/image1.png](https://import.viva64.com/docx/blog/1021_Top_10_CPP_Bugs_2022/image1.png) It is worth noting that this year there were not as many articles about project checks as in past years\. Articles on our [blog](https://pvs-studio.com/en/blog/) have become more varied, and [translations](https://pvs-studio.com/en/blog/posts/?tag=guestarticle) of articles by guest authors have also appeared\. However, I, as one of our blog's readers \(and as the [author of some articles](https://pvs-studio.com/en/blog/posts/?author=vladislav-stolyarov) from it\), still made the top list of bugs found by the PVS\-Studio analyzer that are described in our articles on project checks\. Let me point out that this top is rather subjective — it has many bugs from articles written by me :\)\. If you, dear readers, have your own vision of how this top should look, please, share it in the comments\. So, let's get started, enjoy reading\! ## Tenth place: A classic typo [V501](https://pvs-studio.com/en/docs/warnings/v501/) \[CWE\-570\] There are identical sub\-expressions '\(SrcTy\.isPointer\(\) && DstTy\.isScalar\(\)\)' to the left and to the right of the '\|\|' operator\. CallLowering\.cpp 1198 ```cpp static bool isCopyCompatibleType(LLT SrcTy, LLT DstTy) { if (SrcTy == DstTy) return true; if (SrcTy.getSizeInBits() != DstTy.getSizeInBits()) return false; SrcTy = SrcTy.getScalarType(); DstTy = DstTy.getScalarType(); return (SrcTy.isPointer() && DstTy.isScalar()) || (DstTy.isScalar() && SrcTy.isPointer()); } ``` We see a classic typo in this code fragment: ```cpp (SrcTy.isPointer() && DstTy.isScalar()) || (DstTy.isScalar() && SrcTy.isPointer()) ``` Here is the error: ```cpp (SrcTy.isPointer() && DstTy.isScalar()) || (DstTy.isScalar() && SrcTy.isPointer()) ``` A developer simultaneously changed _Src_ to _Dst_ and _Pointer_ to _Scalar_ in the second line\. As a result, it turns out that the same thing is checked twice\! The code above is equivalent to: ```cpp (SrcTy.isPointer() && DstTy.isScalar()) || (SrcTy.isPointer() && DstTy.isScalar()) ``` The correct option: ```cpp (SrcTy.isPointer() && DstTy.isScalar()) || (SrcTy.isScalar() && DstTy.isPointer()) ``` This error entered the top of the article: "[Examples of errors that PVS\-Studio found in LLVM 15\.0](https://pvs-studio.com/en/blog/posts/cpp/1003/)\." ## Ninth place: Where would we be without array overrun [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The 'j' index is pointing beyond array bound\. OgreAnimationTrack\.cpp 219 ```cpp void AnimationTrack::_buildKeyFrameIndexMap( const std::vector& keyFrameTimes) { // .... size_t i = 0, j = 0; while (j <= keyFrameTimes.size()) // <= { mKeyFrameIndexMap[j] = static_cast(i); while (i < mKeyFrames.size() && mKeyFrames[i]->getTime() <= keyFrameTimes[j]) // <= ++i; ++j; } } ``` The _j_ index that gives us access to the elements of the _keyFrameTimes_ container is incremented to a value equal to the container size\. We can fix it: ```cpp while (j < keyFrameTimes.size()) { // .... } ``` I took this error from the article: "[Checking the Ogre3D framework with the PVS\-Studio static analyzer\.](https://pvs-studio.com/en/blog/posts/cpp/0927/)" ## Eighth place: A clear example of a null pointer dereference [V522](https://pvs-studio.com/en/docs/warnings/v522/) \[CERT\-EXP34\-C\] Dereferencing of the null pointer 'document' might take place\. TextBlockCursor\.cpp 332 ```cpp auto BlockCursor::begin() -> std::list::iterator { return document == nullptr ? document->blocks.end() : document->blocks.begin(); } ``` I remember that the first time I saw this piece of code, I couldn't help but facepalm\. Let's figure out what's going on here\. We see an explicit check of the _document_ pointer for _nullptr_ and its dereference in both branches of the ternary operator\. The code is correct only if the developer aimed to crash the program\. I took this error from the article: "[MuditaOS: Will your alarm clock go off? Part I](https://pvs-studio.com/en/blog/posts/cpp/0914/)"\. [![getTrialImageLink](https://wcdn.pvs-studio.com/media/get_trial_insert.png)](https://pvs-studio.com/en/pvs-studio-download/) ## Seventh place: Learning to count to seven [V557](https://pvs-studio.com/en/docs/warnings/v557/) \[CWE\-787\] Array overrun is possible\. The 'dynamicStateCount \+\+' index is pointing beyond array bound\. VltGraphics\.cpp 157 ```cpp VkPipeline VltGraphicsPipeline::createPipeline(/* .... */) const { // .... std::array dynamicStates; uint32_t dynamicStateCount = 0; dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_VIEWPORT; dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_SCISSOR; if (state.useDynamicDepthBias()) dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BIAS; if (state.useDynamicDepthBounds()) { dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BOUNDS; dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BOUNDS_TEST_ENABLE; } if (state.useDynamicBlendConstants()) dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_BLEND_CONSTANTS; if (state.useDynamicStencilRef()) dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_STENCIL_REFERENCE; // .... } ``` The analyzer warns that an overflow of the _dynamicStates_ array may occur\. There are 4 checks in this code fragment: * if \(state\.useDynamicDepthBias\(\)\) * if \(state\.useDynamicDepthBounds\(\)\) * if \(state\.useDynamicBlendConstants\(\)\) * if \(state\.useDynamicStencilRef\(\)\) Each of these checks is a check of one of the independent flags\. For example, the check of _if \(state\.useDynamicDepthBias\(\)\)_: ```cpp bool useDynamicDepthBias() const { return rs.depthBiasEnable(); } VkBool32 depthBiasEnable() const { return VkBool32(m_depthBiasEnable); } ``` It turns out that all these 4 checks can be true at the same time\. Then 7 lines of the_ 'dynamicStates\[dynamicStateCount\+\+\] \=\.\.\.\.'_ kind will be executed\. On the seventh such line, there will be a call to _dynamicStates\[6\]_\. It's an array index out of bounds\. This bug is from the top of the article: ["Checking the GPCS4 emulator: will we ever be able to play "Bloodborne" on PC?](https://pvs-studio.com/en/blog/posts/cpp/0955/)\." ## Sixth place: Throwing an exception from the noexcept function [V509](https://pvs-studio.com/en/docs/warnings/v509/) \[CERT\-DCL57\-CPP\] The noexcept function '\=' calls function 'setName' which can potentially throw an exception\. Consider wrapping it in a try\.\.catch block\. Device\.cpp 48 ```cpp struct Device { static constexpr auto NameBufferSize = 240; .... void setName(const std::string &name) { if (name.size() > NameBufferSize) { throw std::runtime_error("Requested name is bigger than buffer size"); } strcpy(this->name.data(), name.c_str()); } .... } .... Devicei &Devicei::operator=(Devicei &&d) noexcept { setName(d.name.data()); } ``` Here the analyzer detected that a function, marked as _noexcept_, calls a function that throws an exception\. If an exception arises from the nothrow function's body, the nothrow function calls _std::terminate_, and the program crashes\. It could make sense to wrap the _setName_ function in the [_function\-try\-block_](https://en.cppreference.com/w/cpp/language/function-try-block) and process the exceptional situation there — or one could use something else instead of generating the exception\. I took this error from the article: "[MuditaOS: Will your alarm clock go off? Part I](https://pvs-studio.com/en/blog/posts/cpp/0914/)"\. ## Fifth place: Incorrect work with dynamic memory PVS\-Studio issued two warnings at once for the code snippet presented below: * [V611](https://pvs-studio.com/en/docs/warnings/v611/) \[CERT\-MEM51\-CPP\] The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] heightfieldData;'\. PhysicsServerCommandProcessor\.cpp 4741 * [V773](https://pvs-studio.com/en/docs/warnings/v773/) \[CERT\-MEM31\-C, CERT\-MEM51\-CPP\] The function was exited without releasing the 'worldImporter' pointer\. A memory leak is possible\. PhysicsServerCommandProcessor\.cpp 4742 ```cpp bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....) { btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....); .... const unsigned char* heightfieldData = 0; .... heightfieldData = new unsigned char[width * height * sizeof(btScalar)]; .... delete heightfieldData; return ....; } ``` Let's start with the [V773](https://pvs-studio.com/en/docs/warnings/v773/) warning\. The memory for the _worldImporter _pointer was allocated using the _new_ operator and was not released upon exiting the function\. Let's move on to the [V611](https://pvs-studio.com/en/docs/warnings/v611/) warning and the _heightfieldData_ buffer\. Here, the developer cleared the dynamically allocated memory, but did so in the wrong way\. Instead of calling _delete\[\]_ to release previously allocated memory with the _new\[\]_ operator, they called a simple _delete_\. According to the standard, such code will lead to undefined behavior — here is a [link](https://timsong-cpp.github.io/cppwp/n4861/expr.delete#2) to the corresponding item\. By the way, you can read about why arrays need to be deleted by delete\[\], and about how it all works in general in [article](https://pvs-studio.com/en/blog/posts/cpp/0973/) written by my colleague\. Here is the fixed version: ```cpp bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....) { btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....); .... const unsigned char* heightfieldData = 0; .... heightfieldData = new unsigned char[width * height * sizeof(btScalar)]; .... delete worldImporter; delete[] heightfieldData; return ....; } ``` Also, it's possible to avoid problems with manual memory cleanup by writing more modern code\. For example, automatically free up memory by using [_std::unique\_ptr_](https://en.cppreference.com/w/cpp/memory/unique_ptr)\. The code is shorter and more reliable\. It will protect against unreleased memory errors in case of early exit from the function: ```cpp bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....) { auto worldImporter = std::make_unique (); .... std::unique_ptr heightfieldData; .... heightfieldData = std::make_unique_for_overwrite (width * height * sizeof(btScalar)); .... return ....; } ``` You can find this error in the article: "[In the world of anthropomorphic animals: PVS\-Studio checked Overgrowth](https://pvs-studio.com/en/blog/posts/cpp/0957/)"\. ## Fourth place: The power of Symbolic execution [V560](https://pvs-studio.com/en/docs/warnings/v560/) \[CWE\-570\] A part of conditional expression is always false: DefaultCC \=\= ToCC\. SemaType\.cpp 7856 ```cpp void Sema::adjustMemberFunctionCC(QualType &T, bool IsStatic, bool IsCtorOrDtor, SourceLocation Loc) { .... CallingConv CurCC = FT->getCallConv(); CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic); if (CurCC == ToCC) return; .... CallingConv DefaultCC = Context.getDefaultCallingConvention(IsVariadic, IsStatic); if (CurCC != DefaultCC || DefaultCC == ToCC) return; .... } ``` Let's figure out why the analyzer decided that the right part of the condition is always false\. For convenience, let's remove all superfluous details and replace the names: ```cpp A = ....; B = ....; if (A == B) return; C = ....; if (A != C || C == B) ``` Let's look at how this code works: * we have 3 variables A, B, C and values are unknown to us; * but we know that if A \=\= B, there is an exit from the function; * therefore, if the function continues to execute, then A \!\= B; * if A \!\= C, then, due to the [short\-circuit evaluation](https://en.wikipedia.org/wiki/Short-circuit_evaluation), the right subexpression is not evaluated; * if the right subexpression "C \=\= B" is evaluated, then A \=\= C; * if A \!\= B and A \=\= C, then C cannot be equal to B\. This error entered the top of the article: "[Examples of errors that PVS\-Studio found in LLVM 15\.0](https://pvs-studio.com/en/blog/posts/cpp/1003/)\." ## Third place: hmm, how we love std::optional A couple of analyzer warnings: * [V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'if \(activeInput\)' condition was already verified in line 249\. ServiceAudio\.cpp 250 * [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'activeInput' is always true\. ServiceAudio\.cpp 250 ```cpp std::optional AudioMux::GetActiveInput(); .... auto Audio::handleSetVolume(....) -> std::unique_ptr { .... if (const auto activeInput = audioMux.GetActiveInput(); activeInput) { if (activeInput) { retCode = activeInput.value()->audio->SetOutputVolume(clampedValue); } } .... } ``` _activeInput_ is an _std::optional_ entity from the pointer to _AudioMux::Input_\. The nested _if_ statement contains the _value _member function call_\._ The function is guaranteed to return the pointer and will not throw an exception\. After, the result is dereferenced\. However, the function may return either a valid — or a null pointer\. The plan for the nested _if_ statement was probably to check this pointer\. Hm, I also like wrapping pointers and boolean values in _std::optional_\! And then going through the same grief each time :\)\. Let's see how we can fix it: ```cpp std::optional AudioMux::GetActiveInput(); .... auto Audio::handleSetVolume(....) -> std::unique_ptr { .... if (const auto activeInput = audioMux.GetActiveInput(); activeInput) { if (*activeInput) { retCode = (*activeInput)->audio->SetOutputVolume(clampedValue); } } .... } ``` I took this error from the article: "[MuditaOS: Will your alarm clock go off? Part I](https://pvs-studio.com/en/blog/posts/cpp/0914/)"\. ## Second place: Incorrect use of the flag [V547](https://pvs-studio.com/en/docs/warnings/v547/) \[CWE\-570\] Expression 'nOldFlag & VMPF\_NOACCESS' is always false\. PlatMemory\.cpp 22 ```cpp #define PAGE_NOACCESS 0x01 #define PAGE_READONLY 0x02 #define PAGE_READWRITE 0x04 #define PAGE_EXECUTE 0x10 #define PAGE_EXECUTE_READ 0x20 #define PAGE_EXECUTE_READWRITE 0x40 enum VM_PROTECT_FLAG { VMPF_NOACCESS = 0x00000000, VMPF_CPU_READ = 0x00000001, VMPF_CPU_WRITE = 0x00000002, VMPF_CPU_EXEC = 0x00000004, VMPF_CPU_RW = VMPF_CPU_READ | VMPF_CPU_WRITE, VMPF_CPU_RWX = VMPF_CPU_READ | VMPF_CPU_WRITE | VMPF_CPU_EXEC, }; inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag) { uint32_t nNewFlag = 0; do { if (nOldFlag & VMPF_NOACCESS) { nNewFlag = PAGE_NOACCESS; break; } if (nOldFlag & VMPF_CPU_READ) { nNewFlag = PAGE_READONLY; } if (nOldFlag & VMPF_CPU_WRITE) { nNewFlag = PAGE_READWRITE; } if (nOldFlag & VMPF_CPU_EXEC) { nNewFlag = PAGE_EXECUTE_READWRITE; } } while (false); return nNewFlag; } ``` The _GetProtectFlag_ function converts a flag with file access permission from one format to another\. However, the function does this incorrectly\. The developer did not take into account that the value of _VMPF\_NOACCESS_ is zero\. Because of this, the _if \(nOldFlag & VMPF\_NOACCESS\)_ condition is always false and the function will never return the _PAGE\_NOACCESS_ value\. In addition, the _GetProtectFlag_ function incorrectly converts not only the _VMPF\_NOACCESS_ flag, but also other flags\. For example, the _VMPF\_CPU\_EXEC_ flag will be converted to the _PAGE\_EXECUTE\_READWRITE_ flag\. When a developer who wrote the article was thinking how to fix that issue, his first thought was to write something like this: ```cpp inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag) { uint32_t nNewFlag = PAGE_NOACCESS; if (nOldFlag & VMPF_CPU_READ) { nNewFlag |= PAGE_READ; } if (nOldFlag & VMPF_CPU_WRITE) { nNewFlag |= PAGE_WRITE; } if (nOldFlag & VMPF_CPU_EXEC) { nNewFlag |= PAGE_EXECUTE; } return nNewFlag; } ``` However, in this case, this approach does not work\. The thing is, _PAGE\_NOACCESS_, _PAGE\_READONLY_, and other flags are Windows flags and they have their own specifics\. For example, there is no _PAGE\_WRITE _flag among them\. It is assumed that if there are write permissions, then at least there are also read permissions\. For the same reasons, there is no _PAGE\_EXECUTE\_WRITE_ flag\. In addition, the bitwise "OR" of two Windows flags does not result in a flag corresponding to the sum of the permissions: _PAGE\_READONLY \| PAGE\_EXECUTE \!\= PAGE\_EXECUTE\_READ_\. Therefore, you need to iterate through all possible flag combinations: ```cpp inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag) { switch (nOldFlag) { case VMPF_NOACCESS: return PAGE_NOACCESS; case VMPF_CPU_READ: return PAGE_READONLY; case VMPF_CPU_WRITE: // same as ReadWrite case VMPF_CPU_RW: return PAGE_READWRITE; case VMPF_CPU_EXEC: return PAGE_EXECUTE; case VMPF_CPU_READ | VMPF_CPU_EXEC: return PAGE_EXECUTE_READ: case VMPF_CPU_WRITE | VMPF_CPU_EXEC: // same as ExecuteReadWrite case VMPF_CPU_RWX: return PAGE_EXECUTE_READWRITE; default: LOG("unknown PS4 flag"); return PAGE_NOACCESS; } } ``` This bug is from the article: "[Checking the GPCS4 emulator: will we ever be able to play "Bloodborne" on PC?](https://pvs-studio.com/en/blog/posts/cpp/0955/)\." ## First place: PVS\-Studio prevents rash code changes\! [V530](https://pvs-studio.com/en/docs/warnings/v530/) \[CWE\-252\]: The return value of function 'clamp' is required to be utilized\. BLI\_math\_vector\.hh 88 So, once upon a time there was code that processed a vector of values\. It prevented values from going beyond a certain range\. ```cpp #define CLAMP(a, b, c) \ { \ if ((a) < (b)) { \ (a) = (b); \ } \ else if ((a) > (c)) { \ (a) = (c); \ } \ } \ (void)0 template inline T clamp(const T &a, const bT &min_v, const bT &max_v) { T result = a; for (int i = 0; i < T::type_length; i++) { CLAMP(result[i], min_v, max_v); } return result; } ``` Everything was good\. And then a developer decided to abandon the custom _CLAMP_ macro and use the standard [_std::clamp_](https://en.cppreference.com/w/cpp/algorithm/clamp) function\. And the [commit](https://github.com/blender/blender/commit/399168f3c13fadb41c9fbec8a1b5c56cb6609343) that supposed to make the code better looked like this: ```cpp template inline vec_base clamp(const vec_base &a, const T &min, const T &max) { vec_base result = a; for (int i = 0; i < Size; i++) { std::clamp(result[i], min, max); } return result; } ``` It seems like the developer was in a hurry\. Do you see the error? Maybe yes, maybe no\. Anyway, the developer who wrote the code, didn't notice that it was broken\. The point being — the [_std::clamp_](https://en.cppreference.com/w/cpp/algorithm/clamp) function doesn't change the value of the element in the container: ```cpp template constexpr const T& clamp( const T& v, const T& lo, const T& hi ); ``` The _CLAMP_ macro used to change the value, but the standard function did not\. Now the code is broken and is waiting for someone to notice an error and look for its cause\. Let me point out that we [regularly check](https://pvs-studio.com/en/blog/posts/cpp/0799/) several open\-source projects\. Sometimes it allows you to write quick articles and show interesting bugs in code just written\. Although we already written many articles on this topic, I described the warning that seemed the most striking for me\. By the way, if you haven't read these articles, then I strongly recommend you do\! As the author of this compilation, I want to give the first place not only to this error, but to the whole series of articles on the topic\. In fact, this is a great work of one well\-known person \(Andrey, hi\!\) who started a separate genre of articles on our blog that delighted us throughout the year 2022\. 1. [How PVS\-Studio prevents rash code changes, example N6](https://pvs-studio.com/en/blog/posts/cpp/1001/)\. 1. [How PVS\-Studio prevents rash code changes, example N5](https://pvs-studio.com/en/blog/posts/cpp/0936/)\. 1. [How PVS\-Studio prevents rash code changes, example N4](https://pvs-studio.com/en/blog/posts/cpp/0924/)\. 1. [How PVS\-Studio prevents rash code changes, example N3](https://pvs-studio.com/en/blog/posts/cpp/0922/)\. 1. [How PVS\-Studio prevents rash code changes, example N2](https://pvs-studio.com/en/blog/posts/cpp/0910/)\. 1. [How PVS\-Studio prevents rash code changes](https://pvs-studio.com/en/blog/posts/cpp/0817/)\. 1. [0,1,2, Freddy came for Blender](https://pvs-studio.com/en/blog/posts/cpp/1004/) The same error entered the top of the article: "[How PVS\-Studio prevents rash code changes, example N4](https://pvs-studio.com/en/blog/posts/cpp/0924/)"\. ## Conclusion This year was not as prolific in terms of articles on C\+\+ project checks as the past ones\. However, I hope that we were still able to delight you with a selection of interesting bugs\. We have also written many articles in other genres this year\. You can find them on our [blog](https://pvs-studio.com/en/blog/posts/cpp/)\. It has become a tradition to post such articles on New Year's Eve, so here are the articles with the top 10 bugs found in C and C\+\+ projects of the past years: [2016](https://pvs-studio.com/en/blog/posts/cpp/0483/), [2017](https://pvs-studio.com/en/blog/posts/cpp/0565/), [2018](https://pvs-studio.com/en/blog/posts/cpp/0619/), [2019](https://pvs-studio.com/en/blog/posts/cpp/0700/), [2020](https://pvs-studio.com/en/blog/posts/cpp/0784/), [2021](https://pvs-studio.com/en/blog/posts/cpp/0901/)\.