Simple, yet easy-to-miss errors in code

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!

Comments

[Thibaut Andrieu]

To avoid this kind of bugs, it's a good practice to always check against NULL. It makes the intention explicit. And it also clearly state that pointers are involved:

if(myPointer != NULL) is more readable than if(myPointer).

But it seems this question is not even decided by the spec:

"Abbreviations" such as if(p), though perfectly legal, are considered by some to be bad style (and by others to be good style)

I've heard that some old embedded system define NULL as #define NULL 0xffffffff. So if(myPointer) may be true even if myPointer is NULL. But that's really corner cases, and I think this doesn't even respect the C standard 😂