DEV Community

Cover image for Top 10 bugs found in C++ projects in 2022
Unicorn Developer
Unicorn Developer

Posted on • Originally published at pvs-studio.com

Top 10 bugs found in C++ projects in 2022

#cpp #programming #opensource #tooling

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.

Image description

It is worth noting that this year there were not as many articles about project checks as in past years. Articles on our blog have become more varied, and translations of articles by guest authors have also appeared. However, I, as one of our blog's readers (and as the author of some articles 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 [CWE-570] There are identical sub-expressions '(SrcTy.isPointer() && DstTy.isScalar())' to the left and to the right of the '||' operator. CallLowering.cpp 1198

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());
}
Enter fullscreen mode Exit fullscreen mode

We see a classic typo in this code fragment:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(DstTy.isScalar()  && SrcTy.isPointer())
Enter fullscreen mode Exit fullscreen mode

Here is the error:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(DstTy.isScalar()  && SrcTy.isPointer())
Enter fullscreen mode Exit fullscreen mode

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:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isPointer() && DstTy.isScalar())
Enter fullscreen mode Exit fullscreen mode

The correct option:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isScalar()  && DstTy.isPointer())
Enter fullscreen mode Exit fullscreen mode

This error entered the top of the article: "Examples of errors that PVS-Studio found in LLVM 15.0."

Ninth place: Where would we be without array overrun

V557 Array overrun is possible. The 'j' index is pointing beyond array bound. OgreAnimationTrack.cpp 219

void AnimationTrack::_buildKeyFrameIndexMap(
  const std::vector<Real>& keyFrameTimes)
{

  // ....

  size_t i = 0, j = 0;
  while (j <= keyFrameTimes.size())                    // <=
  {
    mKeyFrameIndexMap[j] = static_cast<ushort>(i);
    while (i < mKeyFrames.size()
      && mKeyFrames[i]->getTime() <= keyFrameTimes[j]) // <=
      ++i;
    ++j;
  }
}
Enter fullscreen mode Exit fullscreen mode

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:

while (j < keyFrameTimes.size())
{
  // ....
}
Enter fullscreen mode Exit fullscreen mode

I took this error from the article: "Checking the Ogre3D framework with the PVS-Studio static analyzer."

Eighth place: A clear example of a null pointer dereference

V522 [CERT-EXP34-C] Dereferencing of the null pointer 'document' might take place. TextBlockCursor.cpp 332

auto BlockCursor::begin() -> std::list<TextBlock>::iterator
{
  return document == nullptr 
            ? document->blocks.end() : document->blocks.begin();
}
Enter fullscreen mode Exit fullscreen mode

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".

Seventh place: Learning to count to seven

V557 [CWE-787] Array overrun is possible. The 'dynamicStateCount ++' index is pointing beyond array bound. VltGraphics.cpp 157

VkPipeline VltGraphicsPipeline::createPipeline(/* .... */) const
{
  // ....
  std::array<VkDynamicState, 6> 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;
  // ....
}
Enter fullscreen mode Exit fullscreen mode

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()):

bool useDynamicDepthBias() const
{
  return rs.depthBiasEnable();
}

VkBool32 depthBiasEnable() const
{
  return VkBool32(m_depthBiasEnable);
}
Enter fullscreen mode Exit fullscreen mode

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?."

Sixth place: Throwing an exception from the noexcept function

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

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());
}
Enter fullscreen mode Exit fullscreen mode

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 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".

Fifth place: Incorrect work with dynamic memory

PVS-Studio issued two warnings at once for the code snippet presented below:

  • 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 [CERT-MEM31-C, CERT-MEM51-CPP] The function was exited without releasing the 'worldImporter' pointer. A memory leak is possible. PhysicsServerCommandProcessor.cpp 4742 
bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  btMultiBodyWorldImporter* worldImporter = new btMultiBodyWorldImporter(....);
  ....
  const unsigned char* heightfieldData = 0;
  ....
  heightfieldData = new unsigned char[width * height * sizeof(btScalar)];
  ....
  delete heightfieldData;
  return ....;
}
Enter fullscreen mode Exit fullscreen mode

Let's start with the 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 V611warning 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 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 written by my colleague.

Here is the fixed version:

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 ....;
}
Enter fullscreen mode Exit fullscreen mode

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. The code is shorter and more reliable. It will protect against unreleased memory errors in case of early exit from the function:

bool PhysicsServerCommandProcessor::processCreateCollisionShapeCommand(....)
{
  auto worldImporter = std::make_unique<btMultiBodyWorldImporter> ();
  ....
  std::unique_ptr<unsigned char[]> heightfieldData;
  ....
  heightfieldData = std::make_unique_for_overwrite<unsigned char[]>
                                (width * height * sizeof(btScalar));
  ....
  return ....;
}
Enter fullscreen mode Exit fullscreen mode

You can find this error in the article: "In the world of anthropomorphic animals: PVS-Studio checked Overgrowth".

Fourth place: The power of Symbolic execution

V560 [CWE-570] A part of conditional expression is always false: DefaultCC == ToCC. SemaType.cpp 7856

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;
  ....
}
Enter fullscreen mode Exit fullscreen mode

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:

A = ....;
B = ....;
if (A == B) return;
C = ....;
if (A != C || C == B)
Enter fullscreen mode Exit fullscreen mode

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, 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."

Third place: hmm, how we love std::optional

A couple of analyzer warnings:

  • V571 Recurring check. The 'if (activeInput)' condition was already verified in line 249. ServiceAudio.cpp 250
  • V547 Expression 'activeInput' is always true. ServiceAudio.cpp 250 
std::optional<AudioMux::Input *> AudioMux::GetActiveInput();

....

auto Audio::handleSetVolume(....) -> std::unique_ptr<AudioResponseMessage>
{
  ....
  if (const auto activeInput = audioMux.GetActiveInput(); activeInput) 
  {
    if (activeInput) 
    {
      retCode = activeInput.value()->audio->SetOutputVolume(clampedValue);
    }
  }
  ....
}
Enter fullscreen mode Exit fullscreen mode

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:

std::optional<AudioMux::Input *> AudioMux::GetActiveInput();

....

auto Audio::handleSetVolume(....) -> std::unique_ptr<AudioResponseMessage>
{
  ....
  if (const auto activeInput = audioMux.GetActiveInput(); activeInput) 
  {
    if (*activeInput) 
    {
      retCode = (*activeInput)->audio->SetOutputVolume(clampedValue);
    }
  }
  ....
}
Enter fullscreen mode Exit fullscreen mode

I took this error from the article: "MuditaOS: Will your alarm clock go off? Part I".

Second place: Incorrect use of the flag

V547 [CWE-570] Expression 'nOldFlag & VMPF_NOACCESS' is always false. PlatMemory.cpp 22

#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;
}
Enter fullscreen mode Exit fullscreen mode

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:

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;
}
Enter fullscreen mode Exit fullscreen mode

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:

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;
  }
}
Enter fullscreen mode Exit fullscreen mode

This bug is from the article: "Checking the GPCS4 emulator: will we ever be able to play "Bloodborne" on PC?."

First place: PVS-Studio prevents rash code changes!

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.

#define CLAMP(a, b, c) \
  { \
    if ((a) < (b)) { \
      (a) = (b); \
    } \
    else if ((a) > (c)) { \
      (a) = (c); \
    } \
  } \
  (void)0

template <typename T> 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;
}
Enter fullscreen mode Exit fullscreen mode

Everything was good. And then a developer decided to abandon the custom CLAMP macro and use the standard std::clamp function. And the commit that supposed to make the code better looked like this:

template <typename T, int Size>
inline vec_base<T, Size>
  clamp(const vec_base<T, Size> &a, const T &min, const T &max)
{
  vec_base<T, Size> result = a;
  for (int i = 0; i < Size; i++) {
    std::clamp(result[i], min, max);
  }
  return result;
}
Enter fullscreen mode Exit fullscreen mode

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 function doesn't change the value of the element in the container:

template <class T>
constexpr const T&
clamp( const T& v, const T& lo, const T& hi );
Enter fullscreen mode Exit fullscreen mode

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 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.
  2. How PVS-Studio prevents rash code changes, example N5.
  3. How PVS-Studio prevents rash code changes, example N4.
  4. How PVS-Studio prevents rash code changes, example N3.
  5. How PVS-Studio prevents rash code changes, example N2.
  6. How PVS-Studio prevents rash code changes.
  7. 0,1,2, Freddy came for Blender

The same error entered the top of the article: "How PVS-Studio prevents rash code changes, example N4".

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.

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, 2017, 2018, 2019, 2020, 2021.

Top comments (0)

Read next

ssukhpinder profile image

Edition 3 — Monday Memes

Sukhpinder Singh -

parkerava profile image

Unlock 100s of Microservices in 5 Easy Steps with Spring Cloud Config & Kotlin!

Ava Parker -

gaundergod profile image

Lucia Auth is getting deprected

Gleb Kotovsky -

dm8ry profile image

Python code that generates a stock price chart for the last n days.

Dmitry Romanoff -