How to Make Fewer Errors at the Stage of Code Writing. Part N4


Abstract

This is the fourth post in which I want to share with you some useful observations on error patterns and the ways of fighting them. This time I will touch upon the subject of handling rare and emergency conditions in programs. While examining a number of applications, I came to the conclusion that error handling code is one of the most unreliable parts in C/C++ programs' sources. What are the consequences of such defects? An application must generate the message "file X is not found" but instead it crashes and forces the user to make guesses about what he/she is doing wrong. A program handling a data base produces an incomprehensible message instead of telling the user that there is just a field filled in incorrectly. Let's try to fight against this type of error that haunt our users.

Introduction

Firstly, here is the information for those readers who are not familiar with my previous posts. You can find them here:

As usual, I won't go into abstract speculations but instead will start with examples. This time I decided to take them from the open source Firefox project. I will try to show you that even in a high-quality and popular application, things are not very good in the code intended for error handling. All the defects have been found with the PVS-Studio 4.50 analyzer.

Error samples

Example N1. Incomplete verification for table integrity

int  AffixMgr::parse_convtable(..., const char * keyword)
{
  ...
  if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
      HUNSPELL_WARNING(stderr,
                       "error: line %d: table is corrupt\n",
                       af->getlinenum());
      delete *rl;
      *rl = NULL;
      return 1;
  }
  ...
}

PVS-Studio diagnostic message: V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. affixmgr.cpp 3708

The programmer tried to verify the table integrity here. Unfortunately, this check may both work and fail. To calculate the length of the key word the sizeof() operator is used, which is certainly incorrect. As a result, whether or not the code works will depend on pure luck (at certain values of the key word and 'keyword' pointer's size in the current data model).

Example 2. Invalid verification for file reading operation

int PatchFile::LoadSourceFile(FILE* ofile)
{
  ...
  size_t c = fread(rb, 1, r, ofile);
  if (c < 0) {
    LOG(("LoadSourceFile: "
         "error reading destination file: " LOG_S "\n",
         mFile));
    return READ_ERROR;
  }
  ...
}

PVS-Studio diagnostic message: V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. updater.cpp 1179

This is an example where the code of error handling was written with the "just letting it to be" approach. The programmer didn't even bother to think over what he/she had written and how it would work. Such a verification is an incorrect one: the fread() function uses an unsigned type to return the number of bytes read. This is the function's prototype:

size_t fread( 
   void *buffer,
   size_t size,
   size_t count,
   FILE *stream 
);

The 'c' variable having the size_t type is naturally used to store the result. Consequently, the result of the (c < 0) check is always false.

This is a good example. It seems, at first glance, that there is some checking here but we find out that it's absolutely useless.

The same error can be found in other places as well:

V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. updater.cpp 2373

V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. bspatch.cpp 107

Example 3. Checking a pointer for NULL only after it had been used

nsresult
nsFrameSelection::MoveCaret(...)
{
  ...
  mShell->FlushPendingNotifications(Flush_Layout);
  if (!mShell) {
    return NS_OK;
  }
  ...
}

PVS-Studio diagnostic message: V595 The 'mShell' pointer was utilized before it was verified against nullptr. Check lines: 1107, 1109. nsselection.cpp 1107

If the pointer is equal to null, we must handle this special occasion and return NS_OK from the function. What confuses me is that the mShell pointer has already been used before this moment.

Probably, this code is operational only because the mShell pointer never equals NULL. I cite this example to demonstrate that one can easily make a mistake even in the simplest of checks. We have it but still it is useless.

Example 4. Checking a pointer for NULL only after it had been used

CompileStatus
mjit::Compiler::performCompilation(JITScript **jitp)
{
  ...
  JaegerSpew(JSpew_Scripts,
    "successfully compiled (code \"%p\") (size \"%u\")\n",
    (*jitp)->code.m_code.executableAddress(),
    unsigned((*jitp)->code.m_size));

  if (!*jitp)
      return Compile_Abort;
  ...
}

PVS-Studio diagnostic message: V595 The '* jitp' pointer was utilized before it was verified against nullptr. Check lines: 547, 549. compiler.cpp 547

By the way, using a pointer before checking it is a wide-spread error. This was one more example of this kind.

Example 5. Incomplete checking of input values

PRBool
nsStyleAnimation::AddWeighted(...)
{
  ...
  if (unit[0] == eCSSUnit_Null || unit[1] == eCSSUnit_Null ||
      unit[0] == eCSSUnit_Null || unit[0] == eCSSUnit_URL) {
    return PR_FALSE;
  }
  ...
}

PVS-Studio diagnostic message: V501 There are identical sub-expressions 'unit [0] == eCSSUnit_Null' to the left and to the right of the '||' operator. nsstyleanimation.cpp 1767

It seems to me that this code fragment contains 2 misprints simultaneously. I cannot tell for sure how exactly the code should look, but the developers most likely intended it to be written as follows:

if (unit[0] == eCSSUnit_Null || unit[1] == eCSSUnit_Null ||
    unit[0] == eCSSUnit_URL  || unit[1] == eCSSUnit_URL) {

The misprints may cause the function to process incorrect input values.

Example 6. Incomplete checking of input values

nsresult PresShell::SetResolution(float aXResolution, float aYResolution)
{
  if (!(aXResolution > 0.0 && aXResolution > 0.0)) {
    return NS_ERROR_ILLEGAL_VALUE;
  }
  ...
}

PVS-Studio diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: aXResolution > 0.0 && aXResolution > 0.0 nspresshell.cpp 5114

Here was one more example of invalid input parameters verification. This time, a misprint doesn't allow the program to check the aYResolution argument's value.

Example 7. A non-dereferenced pointer

nsresult
SVGNumberList::SetValueFromString(const nsAString& aValue)
{
  ...
  const char *token = str.get();
  if (token == '\0') {
    return NS_ERROR_DOM_SYNTAX_ERR; // nothing between commas
  }
  ...
}

PVS-Studio diagnostic message: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *token == '\0'. svgnumberlist.cpp 96

The code checking that there's nothing between the commas does not work. To find out whether or not the string is empty, we can compare the first character to '\0'. But it is the pointer which is compared to null instead of the first character. This pointer is never equal to zero. This is the correct check: (*token == '\0').

Example 8. Incorrect type for storing the index

PRBool 
nsIEProfileMigrator::TestForIE7()
{
  ...
  PRUint32 index = ieVersion.FindChar('.', 0);
  if (index < 0)
    return PR_FALSE;
  ...
}

PVS-Studio diagnostic message: V547 Expression 'index < 0' is always false. Unsigned type value is never < 0. nsieprofilemigrator.cpp 622

The function won't return PR_FALSE if there is no dot in the string and will continue handling incorrect data. The error here is that an unsigned data type was used for the 'index' variable. Checking that (index < 0) is meaningless.

Example 9. Forming a wrong error message

cairo_status_t
_cairo_win32_print_gdi_error (const char *context)
{
  ...
  fwprintf(stderr, L"%s: %S", context, (wchar_t *)lpMsgBuf);
  ...
}

PVS-Studio diagnostic message: V576 Incorrect format. Consider checking the third actual argument of the 'fwprintf' function. The pointer to string of wchar_t type symbols is expected. cairo-win32-surface.c 129

Even if an error was successfully detected, it should be processed correctly. And since nobody tests error handlers either, we may find many interesting things there.

The _cairo_win32_print_gdi_error() function will print some trash. The fwprintf() function awaits a pointer to a unicode-string as the third argument, but instead it gets a string with the 'const char *' format.

Example 10. Dumping error

bool ExceptionHandler::WriteMinidumpForChild(...)
{
  ...
  DWORD last_suspend_cnt = -1;
  ...
  // this thread may have died already, so not opening
  // the handle is a non-fatal error
  if (NULL != child_thread_handle) {
    if (0 <= (last_suspend_cnt =
                SuspendThread(child_thread_handle))) {
  ...
}

PVS-Studio diagnostic message: V547 Expression is always true. Unsigned type value is always >= 0. exception_handler.cc 846

This is another example in the error handler. The result returned by the SuspendThread function is processed incorrectly here. The last_suspend_cnt variable has the DWORD type and therefore is always greater or equal to 0.

About Other Errors in Firefox

Let me digress from the central topic a bit and tell you about the results of checking Firefox in general. The project is of a very high quality, and PVS-Studio had found quite a few errors in it. However, since it's huge one, there is rather big number of errors in a quantitative relation. Unfortunately, I was not able to study the report generated by the PVS-Studio tool thoroughly. The project was analyzed with the console version of PVS-Studio called from the make-file. It is possible to review all the diagnostic messages with opening of the report in Visual Studio. But as there is no project for Visual Studio, it doesn't prompt you for what variables and where are defined and doesn't allow you to navigate to fragments where macros are defined and so on. As a result, analysis of an unknown project is quite labor-intensive, and I managed to study only a fraction of the messages.

The errors are diverse. For example, there are array overruns:

class nsBaseStatis : public nsStatis {
public:
  ...
  PRUint32 mLWordLen[10]; 
  ...
  nsBaseStatis::nsBaseStatis(...)
  {
    ...
    for(PRUint32 i = 0; i < 20; i++)
       mLWordLen[i] = 0;
    ...
  }
  ...
};

PVS-Studio diagnostic message: V557 Array overrun is possible. The value of 'i' index could reach 19. detectcharset.cpp 89

Although this error and other similar errors are interesting, they are not related to the subject of our article. So, if you want to see some other errors, download this file: mozilla-test.txt.

Let's Go Back to Errors in Error Handlers

I decided to cite 10 examples instead of just a couple to convince you that defects in error handlers are a wide-spread issue. Of course, error handlers are not the most crucial and important fragments of a program. But programmers write them, so they hope to improve the program behavior with their help. Unfortunately, my observations convince me that checks and error handlers often fail to work correctly. You see, I had just one project to show you those many errors of this kind.

What should we do with them, what recommendations can we give?

The First Recommendation

We must admit that one might make a mistake even in a simple check. This is the most difficult and important thing to understand. It is because error handlers are considered as simple code fragments that they contain so many misprints and other defects. Error handlers are not tested and checked. Nobody writes tests for them.

Of course, it's difficult and often unreasonable from the economic viewpoint to write tests for error handlers. But if programmers at least know about the danger, it's already an improvement. When you are aware of something, you are already armed to deal with it. There is also an analogy to error handlers that we can refer to.

Statistics tell us that mountain climbers most often fall at the end of ascension. It happens not because of tiredness, but because the person thinks that he/she will soon finish the ascension - he/she relaxes, loses attention and therefore makes more mistakes. Something like that happens to a programmer when he is writing a program. He/she spends much effort and attention on creating an algorithm but does not concentrate much on writing various checks because he/she is sure that he/she in no way can make a mistake there.

So, now you are aware. And I'm sure this thing alone is already good.

If you say that only students and novice programmers make such silly mistakes, you are wrong. Everyone makes misprints. Please read a small post on this topic: "The second myth - expert developers do not make silly mistakes". I can confirm the idea by many examples from various projects. But I think the ones cited here will be enough to make you think it over.

The Second Recommendation

The dumping mechanisms, logging functions and other similar auxiliary mechanisms do deserve creating unit-tests for them.

An inefficient dumping mechanism is not only useless, it only pretends to be able to help you in an emergency situation. If a user sends you a corrupted dump-file, it will not only be unable to help you but will also mislead you and you will spend much more time searching for errors than if the dump-file had never existed at all.

The recommendation looks simple and obvious. But do many of you reading this post have unit-tests to check the WriteMyDump class?

The Third Recommendation

Use static code analyzers. The capability of finding defects in error handlers is one of the strong points of the static analysis methodology. Static analysis covers all the code branches regardless of how often they are used while an application is running. It can detect errors that reveal themselves quite rarely.

In other words, code coverage with static analysis is 100%. It's almost impossible to reach the same code coverage using other types of testing. Code coverage with unit-tests and regression testing usually is less than 80%. The remaining 20% are very difficult to test. These 20% include most error handlers and rare conditions.

The Fourth Recommendation

You may try to use the methodology of Fault injection. The point is that some functions start to return various error codes from time to time, and the program must handle them correctly. For instance, you can write your own function malloc() that will return NULL from time to time even when there is some memory left. It will allow you to know how the application will behave when memory really runs out. The same approach can be applied to such functions as fopen(), CoCreateInstance(), CreateDC(), etc.

There are special programs that allow you to automate this process and do it without manually writing your own functions for causing random failures. Unfortunately, I never dealt with such systems, so I cannot tell you about them in every detail.

Conclusion

Defects in error handlers are very frequent. Unfortunately, I'm not sure that the above given recommendations are enough to avoid them. But I do hope that now this issue is of interest for you and you will invent means to reduce defects in your programs. I, as the other readers as well, will also appreciate it if you could share your ideas and methods on how to avoid errors of the type we have discussed in this article with us.



About the Author

Andrey Karpov

Andrey Karpov is technical manager of the OOO "Program Verification Systems" (Co Ltd) company developing the PVS-Studio tool which is a package of static code analyzers integrating into the Visual Studio development environment.

PVS-Studio is a static analyzer that detects errors in source code of C/C++ applications. There are 3 sets of rules included into PVS-Studio:

  1. Diagnosis of 64-bit errors (Viva64)
  2. Diagnosis of parallel errors (VivaMP)
  3. General-purpose diagnosis

Andrey Karpov is also the author of many articles on the topic of 64-bit and parallel software development. To learn more about the PVS-Studio tool and sources concerning 64-bit and parallel software development, please visit the www.viva64.com site.

Best Articles:

My page on LinkedIn site: http://www.linkedin.com/pub/4/585/6a3

E-mail: karpov@viva64(dot)com

Related Articles

Downloads

Comments

  • Shorter story reveals the unquestionable information about gucci and the way that it could actually harm anyone.

    Posted by emeseesip on 05/07/2013 01:59am

    Just About The Most Comprehensive nike Handbook You Ever Witnessed Or Your Cash Back [url=http://www.guccija.biz/]グッチ 財布[/url] Hmm, awesome item. Users have to find out more about nike immediately whilst it is still up for grabs : ) [url=http://www.guccija.biz/]gucci キーケース[/url] nike assists every one of us by adding a number of exceptional capabilities and functions. Its a unvaluable thing for any follower of gucci. [url=http://www.guccija.biz/]グッチ 財布 新作[/url] Independent piece of writing presents you with 2 brand-new stuff over nike that noone is speaking of. [url=http://www.chanelja.biz/]シャネル バッグ[/url] The key reason why no company is speaking about gucci and therefore things one ought to implement as we speak. [url=http://www.chanelja.biz/]chanel バッグ[/url] Brand new questions about nike addressed and in addition the reason why you must review each and every word in this expose. [url=http://www.chanelja.biz/]財布 chanel[/url] Basic fundamentals of behind adidas you could potentially benefit from getting started today.[url=http://www.nikeja.biz/]ナイキスニーカー[/url] Methods to learn all things there is to know about nike in 3 basic steps.

    Reply
  • Rare posting supplies the run information on the nike which just some visitors know of.

    Posted by icoppyapedcap on 04/25/2013 01:04pm

    GekKcpTixZzy [url=http://www.nikeyasuijp.com/]ナイキ[/url]XcuQlmHkmGun [url=http://www.nikeyasuijp.com/nike-air-force1エアフォース1-c-14.html]ナイキ エアフォース[/url]WidOfkXjdYzl [url=http://www.nikeyasuijp.com/nike-air-maxエアマックス-c-12.html]ナイキ エアマックス[/url]FuwIdrXelAtp [url=http://www.nikeyasuijp.com/nike-air-jordanエア-ジョーダン-c-13.html]ナイキランニング[/url]BnwOxqKacBlc

    Reply
Leave a Comment
  • Your email address will not be published. All fields are required.

Top White Papers and Webcasts

  • Live Event Date: December 11, 2014 @ 1:00 p.m. ET / 10:00 a.m. PT Market pressures to move more quickly and develop innovative applications are forcing organizations to rethink how they develop and release applications. The combination of public clouds and physical back-end infrastructures are a means to get applications out faster. However, these hybrid solutions complicate DevOps adoption, with application delivery pipelines that span across complex hybrid cloud and non-cloud environments. Check out this …

  • Due to internal controls and regulations, the amount of long term archival data is increasing every year. Since magnetic tape does not need to be periodically operated or connected to a power source, there will be no data loss because of performance degradation due to the drive actuator. Read this white paper to learn about a series of tests that determined magnetic tape is a reliable long-term storage solution for up to 30 years.

Most Popular Programming Stories

More for Developers

RSS Feeds