Simple, yet easy-to-miss errors in code

12 Sep 2023

Author: Andrey Karpov

A user wrote to our support about a strange false positive issued by the PVS-Studio analyzer. Let's see why this case deserves a separate note, and why developers don't notice this simple error.

From time to time, users send us various C++ code. From their point of view, the PVS-Studio analyzer has issued strange/false warnings on these fragments. Then, we enhance diagnostics or write down ideas for future fixes. In other words, a routine support job.

But not this time. The developer who received the email from the user eagerly ran to me with the idea of writing about this case. We can't reveal the user's name or show their code. So, here is a rewritten and shortened version:

char *SomeGlobalPointer {};

void foo()
{
  SomeGlobalPointer = new char[1024];
}

int main()
{
  foo();

  if (!SomeGlobalPointer)
  {
    delete[] SomeGlobalPointer;
  }

  return 0;
}

A developer from the user's team says that PVS-Studio issues a "false-positive" warning:

V575 The null pointer is passed into 'operator delete[]'. Inspect the argument.

So, why do we think that it is a null pointer? Look! It's explicitly initialized in the function.

This is an interesting case when the code seems so simple that the developer doesn't see the error. The code for memory deallocation is dull, which makes it difficult to look into the code carefully during code review. I've described a similar case before. You can read about it in the article: "The evil within the comparison functions".

Actually, here is an inconspicuous typo in the condition:

if (!SomeGlobalPointer)
{
  delete[] SomeGlobalPointer;
}

The operator delete[] is called only if the pointer is null. A memory leak occurs as a result. Indeed, the condition needs to be inverted:

if (SomeGlobalPointer)
{
  delete[] SomeGlobalPointer;
}

In fact, the check is unnecessary. The operator delete[] correctly handles null pointers. Let's simplify the code:

char *SomeGlobalPointer {};

void foo()
{
  SomeGlobalPointer = new char[1024];
}

int main()
{
  foo();
  delete[] SomeGlobalPointer;
  return 0;
}

Since we're talking about refactoring, we also suggest to use a smart pointer here (we can optionally use std::unique_ptr or std::shared_ptr). Sure, the code looks odd. But don't forget that the example given in the article is synthetic. However, we've digressed from the main point.

It's a beautiful typo, isn't it? The error is so simple that the developer missed it even when the analyzer issued a warning for this code. Instead, the developer wrote us an email, saying that the analyzer messed up and issued a false positive because of using a global variable.

If PVS-Studio had not assisted, this error would still remain in the code and lead to a memory leak.

Praise static code analysis! Boost your code review efficiency and let the analyzer assist you!

Here you can find other articles that describe how static analysis helps developers avoid simple, yet easy-to-miss errors in code.

#Cpp