A good compiler and coding style aren’t always enough

The fragment is taken from PostgreSQL. The error is detected by the following diagnostic: V575 The ‘memcmp’ function processes ‘0’ elements. Inspect the third argument.

Cppcheck analyzer issues such a warning: Invalid memcmp() argument nr 3. A non-boolean value is required.

Datum pg_stat_get_activity(PG_FUNCTION_ARGS)
{
  ....
  if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
             sizeof(zero_clientaddr) == 0))
  ....
}

Explanation

A closing parenthesis is put in a wrong place. It’s just a typo, but unfortunately it completely alters the meaning of the code.

The sizeof(zero_clientaddr) == 0 expression always evaluates to false as the size of any object is always larger than 0. The false value turns to 0, which results in the memcmp() function comparing 0 bytes. Having done so, the function assumes that the arrays are equal and returns 0. It means that the condition in this code sample can be reduced to if (false).

Correct code

if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
           sizeof(zero_clientaddr)) == 0)

Recommendation

It’s just the case when we can’t suggest any safe coding technique to avoid typos. The only thing we can think of is “Yoda conditions“, when constants are written to the left of the comparison operator:

if (0 == memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
                sizeof(zero_clientaddr)))

But we won’t recommend this style. We don’t like and don’t use it for two reasons:

First, it makes conditions less readable. We don’t know how to put it exactly, but it’s not without reason that this style is called after Yoda.

Second, they don’t help anyway if we deal with parentheses put in a wrong place. There are lots of ways you can make a mistake. Here’s an example of code where using the Yoda conditions didn’t prevent the incorrect arrangement of parentheses:

if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR,
        UnknownError,
        sizeof(UnknownError) / sizeof(UnknownError[0] -
        20)))

This fragment is taken from the ReactOS project. The error is difficult to notice, so let’s point it out: sizeof(UnknownError[0] – 20).

So Yoda conditions are useless here.

We could invent some artificial style to ensure that every closing parenthesis stands under the opening one. But it will make the code too bulky and ugly, and no one will be willing to write it that way.

So, again, there is no coding style we could recommend to avoid writing closing parentheses in wrong places.

скачанные файлы (5)

And here’s where the compiler should come in handy and warn us about such a strange construct, shouldn’t it? Well, it should but it doesn’t. We run Visual Studio 2015, specify the /Wall switch… and don’t get any warning. But we can’t blame the compiler for that, it has enough work to do as it is.

The most important conclusion for us to draw from this post is that good coding style and compiler (and we do like the compiler in VS2015) do not always make it. We sometimes hear statements like, “You only need to set the compiler warnings at the highest level and use good style, and everything’s going to be OK” No, it’s not like that. We don’t mean to say some programmers are bad at coding; it’s just that every programmer makes mistakes. Everyone, no exceptions. Many of your typos are going to sneak past the compiler and good coding style.

So the combo of good style + compiler warnings is important but not sufficient. That’s why we need to use a variety of bug search methods. There’s no silver bullet; the high quality of code can be only achieved through a combination of several techniques.

The error we are discussing here can be found by means of the following methods:

  • code review;
  • unit-tests;
  • manual testing;
  • static code analysis;
  • etc.

Some people don’t get it that having skill isn’t enough to avoid mistakes. Everyone makes them – it’s inevitable. Even super-guru make silly typos every now and then. And since it’s inevitable, it doesn’t make sense blaming programmers, bad compilers, or bad style. It’s just not going to help. Instead, we should use a combination of various software quality improving techniques.

Written by Andrey Karpov.

This error was found with PVS-Studio static analysis tool.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s