PVS-Studio Team headed by a
C++ Microsoft MVP Andrey Karpov has checked
more than 220 open-source C/C++ projects

Now we would like to share the most common mistakes and
suggest ways to correct them with other C/C++ developers


Get 1 hint every week to your email





C++Hints is taking a break

Posted by Andrey Karpov    |    On January 22, 2016    |    Permalink

We made a decision to take a break in maintaining C++Hints for some time or permanently. We are taking into account that our readers sent us positive feedback and we hope that they enjoyed this project. Big thanks to everybody who supported us.

But unfortunately, at this point our company doesn't have enough resources to keep running several projects at the same time.

We'll still post articles in our company blog about the projects we check, giving some recommendations of how to avoid bugs http://www.viva64.com/en/b/.

RSS of the blog: http://feeds.feedburner.com/viva64-blog-en

We also suggest following our CTO, Andrey Karpov, @Code_Analysis on Twitter to be in the know of the latest C++ news.

Use nullptr instead of NULL from now on

Posted by Andrey Karpov    |    On January 15, 2016    |    Permalink

New C++ standards brought quite a lot of useful changes. There are things that I would not rush into using straight away, but there are some changes which need to be applied immediately, as they will bring significant benefits.

One such modernization is the keyword 'nullptr', which is intended to replace the NULL macro.

Let me remind you that in C++ the definition of NULL is 0, nothing more.

Of course, it may seem that this is just some syntactic sugar. And what's the difference, if we write nullptr or NULL? But there is a difference! Using nullptr helps to avoid a big variety of errors.

I'll show it using examples.

Suppose, there are two overloaded functions:

void Foo(int x, int y, const char *name);
void Foo(int x, int y, int ResourceID);

A programmer might write the following call:

Foo(1, 2, NULL);

And that same programmer might be sure that he is in fact calling the first function by doing this. It is not so. As NULL is nothing more than 0, and zero is known to have 'int' type, the second function will be called instead of the first.

However, if the programmer had used 'nullptr' no such error would occur and the first function would have been called.

Another common enough use of NULL is to write code like this:

if (unknownError)
  throw NULL;

To my mind it is suspicious to generate an exception passing the pointer. Nevertheless sometimes people do so. Apparently the developer needed to write the code in this way. However, discussions whether it is a good or bad practice to do so go beyond the scope of this note.

What is important is that the programmer decided to generate an exception in the case of an unknown error and "send" a null pointer into the outher world.

In fact it is not a pointer but 'int'. As a result the exception handling will happen in the way that the programmer didn't expect it to.

"throw nullptr;" code saves us from misfortune, but this does not mean that I believe this code to be totally acceptable.

In some cases, if you use 'nullptr', the incorrect code will not compile.

Suppose that some WinApi function returns an HRESULT type. The HRESULT type has nothing to do with the pointer. However, it is quite possible to write nonsensical code like this:

if (WinApiFoo(a, b, c) != NULL)

This code will compile, because NULL is 0 and of 'int' type, and HRESULT is a 'long' type. It is quite possible to compare type values 'int' and 'long'. If you use 'nullptr', then the following code will not compile:

If (WinApiFoo (a, b, c)! = nullptr)

Thus, the error will be immediately noticed and fixed.

I think you get the idea. There are plenty of such examples. But these are mostly synthetic examples. And it is always not very convincing. So are there any real examples? Yes, there are. Here is one of them. The only thing - it's not very graceful or short.

This code is taken from the MTASA project.

We know that there exists RtlFillMemory() This can be a real function or simply a macro. It doesn't matter. It is similar to the memset() function, but the 2nd and 3rd argument switched their places. Here's how this macro can be declared:

#define RtlFillMemory(Destination,Length,Fill) \
  memset((Destination),(Fill),(Length))

There is also FillMemory(), which is nothing more than RtlFillMemory():

#define FillMemory RtlFillMemory

Yes, everything is long and complicated. But at least it is an example or real erroneous code.

And here's the code that uses the FillMemory macro.

LPCTSTR __stdcall GetFaultReason ( EXCEPTION_POINTERS * pExPtrs )
{
  ....
  PIMAGEHLP_SYMBOL pSym = (PIMAGEHLP_SYMBOL)&g_stSymbol ;
  FillMemory ( pSym , NULL , SYM_BUFF_SIZE ) ;
  ....
}

This code fragment has even more bugs. We clearly see that at least the 2 and 3 argument are confused here. That's why the analyzer issues 2 warnings V575:

  • V575 The 'memset' function processes value '512'. Inspect the second argument. crashhandler.cpp 499
  • V575 The 'memset' function processes '0' elements. Inspect the third argument. crashhandler.cpp 499

The code compiled because NULL is 0. As a result, 0 array elements get filled.

But in fact the error is not only about this. NULL is in general not appropriate here. The memset() function works with bytes, so there's no point in trying to make it fill the memory with NULL values. This is some absurdity. Correct code should be like this :

FillMemory(pSym, SYM_BUFF_SIZE, 0);

Or like this:

ZeroMemory(pSym, SYM_BUFF_SIZE);

But it's not the main point, which is that this meaningless code compiles successfully. However, if the programmer had gotten into the habit of using ''nullptr'' instead of ''NULL'' and written this instead:

FillMemory(pSym, nullptr, SYM_BUFF_SIZE);

the complier would have emitted a diagnostic, and the programmer would realize that they did something wrong and would pay more attention to the way they code.

Note. I understand that in this case NULL is not to blame. However, it is because of NULL the code compiles without any warnings.

Recommendation

Start using nullptr. Right now. And make necessary changes in the coding standard of your company.

Using nullptr will help to avoid stupid errors and thus will speed up the development process.


  

PVS-Studio analyzer is expanding the horizons

Posted by Andrey Karpov    |    On December 23, 2015    |    Permalink

This bug was found in GIT source code.

Incorrect code:

public int GetMethodsInDocument(
  ISymUnmanagedDocument document,
  int bufferLength, 
  out int count,
  ....)
{
  ....
  if (bufferLength > 0)
  {
    ....
    count = actualCount;
  }
  else
  {
    count = extentsByMethod.Length;
  }
  count = 0;
  return HResult.S_OK;
}

Explanation:

The function always returns 0 through the 'count' variable. Most likely the last line "count = 0" is redundant and accidentally got into the code.

You may say "Wait a second, this is C#!" You're absolutely right. This time let me speak on a different topic.

There happened a big event in our company. We have released the 6th version of PVS-Studio analyzer that now has C# support. So we would like to share this news. I hope you won't be offended that I wrote about it here.

There is no better way to show the abilities of the analyzer than to provide examples of real bugs found. We have several articles about checks of C# projects that I suggest reading. I suppose that among our readers there are not only C++ programmers, but those who also code in C#.

Umbraco project check: http://www.viva64.com/en/b/0357/

SharpDevelop project check: http://www.viva64.com/en/b/0359/

Microsoft Code Contracts project check: http://www.viva64.com/en/b/0361/

Roslyn: http://www.viva64.com/en/b/0363/

To those who may be interested - the new analyzer is available here. You can download and run it on your project.

Recommendation:

It is important to use static code analysis if you want to have safe and bug-free code. This is the only way to truly experience the benefits that it brings.

I am sure that when you check your project, many of the bugs will be related to the tests or error handlers. This is natural. Errors in frequently used code fragments often get corrected by thorough testing and reviews by users. But it is the fact itself that PVS-Studio can catch those bugs that matters. It means that many bugs, which took much time and effort to fix, could be fixed immediately by using PVS-Studio on a regular basis.

The same idea in other words. It is regular use that makes the analyzer valuable, not casual runs. View static code analysis as an extension to compiler warnings. It's not a good idea to enable compiler warnings once a year. You have to address the bugs as soon as they appear. It helps save development time on tracking down silly mistakes through debugging. It's just the same thing with static analyzers.

Of course, it cannot catch all the bugs. But instead of wasting time hunting for typos and slip-ups, spend it on something of a bigger worth. Let PVS-Studio deal with "stupid" errors.

P.S. You don't make stupid mistakes? Well, well! Sometimes it may seem that there are none. Everyone does. Just have a look here.

This error was found by PVS-Studio static analysis tool. The error text: V3008 The 'count' variable is assigned values twice successively. Perhaps this is a mistake.

If something strange is happening to your PC, check its memory.

Posted by Andrey Karpov    |    On December 17, 2015    |    Permalink

A typical situation-your program is not working properly. But you have no idea what's going on.

In such situations I recommend not to rush blaming someone, but focus on your code. In 99.99% of cases the root of the evil is a bug that was brought by someone from your development team. Very often this bug is really stupid and banal. So go ahead and spend some time looking for it!

The fact that the bug occurs from time to time means nothing. You may just have a Heisenbug

Blaming the compiler would be even a worse idea. It may do something wrong, of course, but very rarely. It will be very awkward if you find out that it was a wrong using of sizeof(), for example. I have a post about that in my blog: The compiler is to blame for everything.

But to set the record straight, I should say that there are exceptions. Very seldom the bug has nothing to do with the code. But we should be aware that such a possibility exists. This will help us to stay sane.

I'll demonstrate it using an example of a case that happened once with me. Fortunately, I have the necessary screenshots.

One simple test project that I was preparing for the demonstration of Viva64 (ancestor of PVS-Studio) refused to work properly.

After long and tiresome investigations I saw that one memory slot is causing all this trouble. One bit, to be exact. You can see on the picture that I am in the debug mode, writing the value "3" in this memory cell.

Picture 1

After the memory is changed the debugger reads the values to display in the window. And shows number 2:

Picture 2

See, there is 0x02. Although I've set the "3" value.

The low-order bit is always zero.

Memory test program confirmed the problem. It's strange that the computer was working normally without any problems. Replacement of the memory bank finally let my program work correctly.

I was very lucky. I had to deal with a simple test program. And still I spent a lot of time trying to understand what was happening. I was reviewing the assembler listing for more than two hours, trying to find the cause of the strange behavior. Yes, I was blaming the compiler for that.

I can't imagine how much more effort it would take it were a real program. Thanks God, I didn't have to debug anything else at that moment.

Recommendation

Always look for the error in your code. Do not try to shift responsibility.

However if the bug reoccurs only at your computer for more than a week, it may be a sign that it's not because of your code.

Keep looking for the bug. But before going home, run an overnight RAM test. Perhaps, this simple step will save thousands of your nerve cells.

Good luck to you!

Avoid adding a new library to the project.

Posted by Andrey Karpov    |    On December 10, 2015    |    Permalink

Suppose, you need to implement an X functionality in your project. Theorists of software development will say that you have to take the already existing library Y, and use it to implement the things you need. In fact, it is a classic approach in the software development - reusing your own or others' previously created libraries (third-party libraries). And most of the programmers use this way.

However, those theorists in various articles and books forget to mention what hell it will become to support several dozens of third-party libraries in about 10 years.

I strongly recommend to avoid adding a new library to a project. Please don't get me wrong. I am not saying that you shouldn't use libraries at all and write everything yourself. This would be insufficient, of course. But sometimes a new library is added to the project at the whim of some developer, intending to add a little cool small "feature" to the project. It's not hard to add a new library to the project, but then the whole team will have to carry the load of its support for many years.

Tracking the evolution of several large projects, I have seen quite a lot of problems caused by a large number of third-party libraries. I will probably enumerate only some of the issues, but this list should already provoke some thoughts:

  1. Adding new libraries promptly increases the project size. In our era of fast Internet and large SSD drives, this is not a big problem, of course. But, it's rather unpleasant when the downloading time turns into 10 minutes instead of 1 because of the version control system.
  2. Even if you use just 1% of the library capabilities usually it is included in the project as a whole. As a result, if the libraries are used in the form of compiled modules (for example, DLL), the distribution size grows very fast. If you use the library as source code, then the compile time significantly increases.
  3. Infrastructure connected with the compilation of the project becomes more complicated. Some libraries require additional components. A simple example: we need Python for building. As a result, in some time you'll need to have a lot of additional programs to build a project. So the probability that something will fail increases. It's hard to explain it, you should experience it. In big projects something fails all the time and you have to put a lot of efforts to make everything work and compile.
  4. If you care about vulnerabilities, you must regularly update third-party libraries. It's of interest for violaters to study the code libraries to search for vulnerabilities. Firstly, many libraries are open-source, and secondly, having found a weak point in one of the libraries, you can get a master key to many applications where the library is used.
  5. You will have troubles upgrading to a new version of the compiler. There will definitely be a few libraries that won't be ready to adapt for a new compiler, you'll have to wait or to make your own corrections in the library.
  6. You will have problems when moving to a different compiler. For example, you are using Visual C++ and want to use Intel C++. There will surely be a couple of libraries where something is wrong.
  7. You will have problems moving to a different platform. Not necessarily even a totally different platform. Let's say, you'll decide to port a Win32 application to Win64. You will have the same problems. Most likely, several libraries won't be ready for this and you will wonder what to do with them. It is especially unpleasant when the library is laying somewhere dormant and is no longer developing.
  8. Sooner or later, if you use lots of C libraries, where the types aren't stored in namespace, you'll start having clashes of names. This causes compilation errors or hidden errors. For example, a wrong enum constant can be used instead of the one you've intended to.
  9. If your project uses a lot of libraries, adding another one won't seem harmful. We can draw an analogy with the broken windows theory. But consequently, the growth of the project turns into uncontrolled chaos.
  10. And there could be a lot more other downsides of adding new libraries that I'm probably not aware of. But in any case, additional libraries increase the complexity of project support. Some issues can occur in a fragment where they were least expected to.

Again, I should emphasize. I don't say that we should stop using third-party libraries at all. If we have to work with images in PNG format in the program, we'll take the LibPNG library and not reinvent the wheel.

But even working with PNG we need to stop and think. Do we really need a library? What do we want to do with the images? If the task is just to save an image in *.png file, you can get by with system functions. For example, if you have a Windows application, you could use WIC. And if you're already using a MFC library, there is no need to make the code more sophisticated, because there's a CImage class (see the discussion on StackOverflow). Minus one library-great!

Let me give you an example from my own practice. In the process of developing the PVS-Studio analyzer, we needed to use simple regular expressions in a couple of diagnostics. In general, I am convinced that static analysis isn't a right place for regular expressions. This is an extremely inefficient approach. I even wrote an article regrding this topic. But sometimes you just need to find something in a string with the help of a regular expression.

It was possible to "squeeze in" existing libraries, but it was clear that all of them would be redundant. At the same time we still needed regular expressions and we had to come up with something.

Absolutely coincidentally, exactly at that moment I was reading a book "Beautiful Code" (ISBN 9780596510046). This book is about simple and elegant solutions. And there I came across an extremely simple implementation of regular expressions. Just a few dozen of strings. And that's it!

I decided to use that implementation in PVS-Studio. And you know what? The abilities of this implementation are still enough for us; complex regular expressions are just not necessary for us.

Conclusion. Instead of adding a new library we spent half an hour writing a needed functionality. We suppressed the desire to use one more library. And it turned out to be a great decision; the time showed that we really didn't need that library.

This case really convinced me that the simpler solution, the better. By avoiding adding new libraries (if possible) you make your project simpler.

Probably the readers may be interested to know what was the code for searching regular expressions. We'll type it here from the book.

See how graceful it is. This code was slightly changed when integrating to PVS-Studio, but its main idea remains unchanged. So, the code from the book:

// regular expression format
// c          matches any "c" letter
// .(dot)     matches any (singular) symbol 
// ^          matches the beginning of the input string
// $          matches the end of the input string
// #          matches the occurence of the previous symbol from 0 
//            to several times

int matchhere(char *regexp, char *text);
int matchstar(int c, char *regexp, char *text);

// match: search for regular expression anywhere in text
int match(char *regexp, char *text)
{
  if (regexp[0] == '^')
    return matchhere(regexp+1, text);
  do { /* must look even if string is empty */
   if (matchhere(regexp, text))
     return 1;
  } while (*text++ != '\0');
  return 0;
}

// matchhere: search for regexp at beginning of text 
int matchhere(char *regexp, char *text)
{
   if (regexp[0] == '\0')
     return 1;
   if (regexp[1] == '*')
     return matchstar(regexp[0], regexp+2, text);

   if (regexp[0] == '$' && regexp[1] == '\0')
     return *text == '\0';
   if (*text!='\0' && (regexp[0]=='.' || regexp[0]==*text))
     return matchhere(regexp+1, text+1);
   return 0;
}

// matchstar: search for c* regexp at beginning of text
int matchstar(int c, char *regexp, char *text)
{
  do {   /* a * matches zero or more instances */
            more instances */
    if (matchhere(regexp, text))
      return 1;
  } while (*text != '\0' && (*text++ == c || c == '.'));
  return 0;
}

Recommendation

Don't hurry adding new libraries to the project; add it only when there is no other way to manage without a library.

Here are some possible workarounds:

  1. Have a look if the API of your system or one of the existing libraries has a required functionality. It's a good idea to investigate this question.
  2. If you plan to use a small piece of functionality from the library, then it makes sense to implement it yourself. The argument to add a library "just in case" is no good. Almost certainly, this library won't be used much in the future. Programmers sometimes want to have universality that is actually not needed.
  3. If there are several libraries to resolve your task, choose the simplest one that meets your needs. As I have stated before, get rid of the idea "it's a cool library - let's take it just in case"
  4. Before adding a new library, sit back and think. May be even take a break, get some coffee, discuss with your colleagues. Perhaps you'll see that you can solve the problem in a completely different way, without using third-party libraries.