How to properly rewrite ASSERT code to pass /analyze in msvc?

Question

Visual Studio added code analysis (/analyze) for C/C++ in order to help identify bad code. This is quite a nice feature but when you deal with and old project you may be overwhelmed by the number of warnings.

Most of the problems are generating because the old code is doing some ASSERT at the beginning of the method or function.

I think this is the ASSERT definition used in the code (from afx.h)

#define ASSERT(f)          DEBUG_ONLY((void) ((f) || !::AfxAssertFailedLine(THIS_FILE, __LINE__) || (AfxDebugBreak(), 0)))

Example code:

ASSERT(pBytes != NULL);
*pBytes = 0; // <- warning C6011: Dereferencing NULL pointer 'pBytes'

I'm looking for an easy, clean and safe solution to solve these warnings that does not imply disabling these warnings. Did I mention that there are lots of occurrences in current codebase?

Answer 1

You seem to assume that ASSERT(ptr) somehow means that ptr is not NULL afterwards. That's not true, and the code analyzer doesn't make that assumption.

Answer 2

PREFast is telling you that you have a defect in your code; don't ignore it. You do in fact have one, but you have only skittered around acknowleging it. The problem is this: just because pBytes has never been NULL in development & testing doesn't mean it won't be in production. You don't handle that eventuality. PREfast knows this, and is trying to warn you that production environments are hostile, and will leave your code a smoking, mutilated mass of worthless bytes.

/rant

There are two ways to fix this: the Right Way, and a hack.

The right way is to handle NULL pointers at runtime:

void DoIt(char* pBytes)
{
    assert(pBytes != NULL);
    if( !pBytes )
        return;
    *pBytes = 0;
}

This will silence PREfast.

The hack is to use an annotation. For example:

void DoIt(char* pBytes)
{
    assert(pBytes != NULL);
    __analysis_assume( pBytes );
    *pBytes = 0;
}

EDIT: Here's a link describing PREfast annotations. A starting point, anyway.

Answer 3

/analyze is not guaranteed to yield relevant and correct warnings. It can and will miss a lot of issues, and it also gives a number of false positives (things it identifies as warnings, but which are perfectly safe and will never actually occur)

It is unrealistic to expect to have zero warnings with /analyze.

It has pointed out a situation where you dereference a pointer which it can not verify is always valid. As far as PREfast can tell, there's no guarantee that it will never be NULL.

But that doesn't mean it can be NULL. Just that the analysis required to prove that it's safe is too complex for PREfast.

You may be able to use the Microsoft-specific extension __assume to tell the compiler that it shouldn't produce this warning, but a better solution is to leave the warning. Every time you compile with /analyze (which need not be every time you compile), you should verify that the warnings it does come up with are still false positives.

If you use your asserts correctly (to catch logic error during programming, guarding against situations that cannot happen, then I see no problem with your code, or with leaving the warning. Adding code to handle a problem that can never occur is just pointless. You're adding more code and more complexity for no reason (if it can never occur, then you have no way of recovering from it, because you have absolutely no clue what state the program will be in. All you know is that it has entered a code path you thought impossible.

However, if you use your assert as actual error handling (the value can be NULL in exceptional cases, you just expect that it won't happen), then it is a defect in your code. Then proper error handling (exceptions, typically) is needed.

Never ever use asserts for problems that are possible. Use them to verify that the impossible isn't happening. And when /analyze gives you warnings, look at them. If it is a false positive, ignore it (don't suppress it, because while it's a false positive today, the code you check in tomorrow may turn it into a real issue).

Answer 4

remember, ASSERT() goes away in a retail build so the C6011 warning is absolutely correct in your code above: you must check that pBytes is non-null as well as doing the ASSERT(). the ASSERT() simply throws your app into a debugger if that condition is met in a debug bug.

I work a great deal on /analyze and PREfast, so if you have other questions, please feel to let me know.

Answer 5

My co-author David LeBlanc would tell me this code is broken anyway, assuming you're using C++, you should use a reference rather than a pointer, and a ref can't be NULL :)