/analyze for Visual Studio-the Ugly (part 5)

/Analyze is a powerful Visual C++ feature that can help bugs. But oh my it does seem to have a lot of bugs in itself. In this post I describe some new ones that I’ve found, suggest some workarounds for some of these bugs, and then discuss bugs with some of the workarounds!

This post is a bit long because I’m trying to clean out the backlog of issues that I’ve found, but I’ve tried to keep it as short as possible and put the more important issues near the top.

Update: Luckily VC++ 2013 has solved many of these issues, but the problems with __analysis_assume remain.

These tests are all done with Visual Studio 2010 SP1, C/C++ Optimizing Compiler Version 16.00.40219.01 for 80×86.

Is it worth it?

If you’ve been following along as I find myriad annoying flaws in /analyze then you might well be wondering if it’s worth it. My summary would be that while it’s not as good as it should be (it seems like many of these /analyze bugs should have been found during testing), it is still worthwhile. In particular:

• By using /analyze I have found literally hundreds of bugs, and now that it is part of the nightly build it finds a half dozen new bugs each week soon after they are checked in
• John Carmack, from id Software, is a huge fan. He recently said: “I’ll say this quite clearly with no hesitation. Any 360 developers that are listening to this. If you’re not using analyze you are making a mistake”

On to the bugs…

Array index heuristics gone mad

If you do any tests of a variable before using it as an array index then /analyze gets some peculiar ideas. Here are some examples:

static char g_array[15];

char GetValue1(int index) {
    // No warning
    return g_array[index];
}

char GetValue2(int index) {
    if (index == 15) return 0;
    // warning C6385: Invalid data: ’18’ bytes might be read
    return g_array[index];
}

char GetValue3(int index) {
    index &= 15;
    if (index == 15) return 0;
    // warning C6385: Invalid data: ’18’ bytes might be read
    return g_array[index];
}

char GetValue4(int index, int number) {
    if (index == number) return 0;
    // warning C6385: Invalid data: ‘1048561’ bytes might be read
    return g_array[index];
}

char GetValue5(int index) {
    if (!index) return 0;
    // warning C6385: Invalid data: ‘1001’ bytes might be read
    return g_array[index];
}

GetValue2() demonstrates that if you compare index against a constant ‘n’ then /analyze assumes that index might have the value ‘n+2’ (an index of 17 means the 18th byte is read). GetValue3() demonstrates that /analyze maintains that belief even though the ‘and’ operation in the first line of the function makes that belief impossible. GetValue4() demonstrates that if you compare the index against some other variable then /analyze thinks that index could be equal to 1048560 (in hex that is 0xFFFF0). Finally, if you check index with the ‘!’ operator then /analyze thinks that index could be equal to 1000.

I don’t know if this behavior is from a heuristic gone wrong or if it indicates an uninitialized variable in the /analyze code, but this behavior is the biggest source of frustrating false positives that I have to deal with. If I compare index against a constant then that is an indication that index could conceivably have that value, but it is not an indication that index could conceivably be two larger than that value, or 1,000, or 0xFFFF0.

Eventually I started to recognize the magic numbers and that helped me to triage the warnings faster, but it is still a time consuming process and I’d rather not see these unjustified and confusing warnings at all.

Out of range bool

In this function we are using a bool to index a two element array. Since a bool can only be true or false, one or zero, we are guaranteed to be safe. However if we test the value of the bool then /analyze decides that it might have the value -24.

void OccludeTest(bool var1, char var2) {
    char arr2[2] = {};
    if (var1)
        ++var2;

    // Buffer overrun: accessing ‘arr2’, the writable size
    // is ‘2’bytes, but ‘-24’ bytes might be written
    arr2[var1] = var2;
}

At first I assume that -24 was yet another magic number, but it turns out it is just a new variation on an old friend. A bool is an 8-bit type in VC++ and -24 in hex is 0xE8. It turns out that 1,000 (one of the magic numbers from the previous item) is 0x3E8, so -24 is just that magic number masked down to an 8-bit value.

In range, then out

I find this block of code amusing – in a dark humor sort of way. The code reads a char from g_Bar[blah] with no warning. However it them compares blah to the result, and once /analyze sees an index being compared to anything it gets weird ideas about what values that index might have. So, in the body of the ‘if’ statement when we index into the array with exactly the same index it gives us a warning. Madness.

char g_Bar[2];
const char GetChar(int blah) {
    if (g_Bar[blah] == blah) {
        // warning C6385: Invalid data: accessing ‘char * g_Bar’, the
        // readable size is ‘2’ bytes, but ’56’ bytes might be read
        return g_Bar[blah] + 1;
    }

    return 0;
}

Reading 56 bytes implies an index of 55 and I have no idea where the magic number 55 comes from.

Suppressing array index warnings

A useful technique for suppressing these index related /analyze warnings is a specialized Assert macro, like this:

#ifdef    _PREFAST_
    // Warning – this doesn’t always compile!
    #define Assert(x) __analysis_assume(x)
#else
    #define Assert(x) assert(x)
#endif

This technique was highly recommended by John Carmack and, while I find it frustrating to have to use it to work around /analyze bugs, it is a handy way of suppressing a warning so that I don’t have to see it again, and I find it preferable to using “#pragma warning(suppress : warningNumber)”. An example of using this Assert macro is:

char GetValue6(int index) {
    if (index == 15) return 0;
    // This suppresses the warning
    Assert(index < ARRAYSIZE(g_array));
    return g_array[index];
}

Voila! No more warning. And, as a bonus, if my assumption is incorrect then in debug builds the assert will trigger. That’s overkill for those times when /analyze is warning about something impossible, but oh well. I recommend this technique.

Suppressing errors from suppressing array index warnings

Unfortunately, on a sufficiently large code base you may find some constructs that Assert can handle but __analysis_assume can’t. An example of this type of problem is shown below:

void PointerTest() {
    std::shared_ptr<char> p;
    assert(p);
    //__analysis_assume(p); // error C2088: ‘__assume’ : illegal for class
    Assert(p);
}

assert(p) checks to see whether p points to an object or not and is a reasonable thing to assert. But __analysis_assume can’t handle it. Luckily there is a workaround for this problem with our first workaround.

#ifdef    _PREFAST_
    #define Assert(x) __analysis_assume(!!(x))
#else
    #define Assert(x) assert(x)
#endif

Adding a pair of ‘!’ characters to our expression (don’t forget the extra parentheses) does the trick, and now Assert is understood by all. Ugly hacks for the win!

Bad Bounds Checking

/analyze does some pretty sophisticated analysis, which makes its failures on this simple function very peculiar. The first writes to ‘arr’ in the ‘if’ and ‘else’ clauses are obviously and entirely valid, but /analyze warns about them. Using Assert/__analysis_assume fails to suppress the warnings.

At first it seemed encouraging that creating temporary variables (tempIndex1 in both clauses) avoided the warning. However I then found that this technique avoided the warnings by disabling almost all checking! In both clauses I create and use tempIndex2, and use that for invalid accesses. The accesses are obviously and entirely invalid, but there is no warning.

void FillArray() {
    char arr[10];
    for (int i = 0; i < 15; ++i) {
        if (i < ARRAYSIZE(arr)) {
            // This doesn’t prevent a warning.
            Assert(i < ARRAYSIZE(arr));
            // Bogus warning on valid code.
            // warning C6201: Index ’14’ is out of valid index range
            arr[i] = 0;

            // Use a temp int to suppress the warning
            const int tempIndex1 = i;
            arr[tempIndex1] = 0;

            // No warning on bogus code.
            const int tempIndex2 = i + 1;
            arr[tempIndex2] = 0;
        }
        else {
            // This doesn’t prevent a warning.
            Assert(i – 5 >= 0);
            // Bogus warning on valid code.
            // warning C6201: Index ‘-5’ is out of valid index range
            arr[i – 5] = 1;

            // Use a temp int to suppress the warning
            const int tempIndex1 = i – 5;
            arr[tempIndex1] = 0;

            // No warning on bogus code.
            const int tempIndex = i – 100;
            arr[tempIndex] = 1;
        }
    }
}

I don’t know what to say here. The false positives give an impression of vigilance, but once you scratch the surface you realize that /analyze is unable to determine anything about this function. I consider this to be one of the most serious /analyze bugs I have found, indicating as it does that /analyze cannot analyze a simple loop whose execution is entirely statically determined.

This bug was what made me realize that getting our project to zero warnings was not practical, and in fact wasn’t even desirable.

Bad Annotations

Before I dive into this bug I need to make two observations:

1) Annotations of functions that deal with buffers are crucial for helping /analyze find bugs. If /analyze doesn’t know what the contract is (buffer size, input or output buffer, null-terminated or not) then it can’t verify that the contract is being respected.

2) The template overload versions of the safer CRT string functions should always be used when the destination is an array. The let the compiler infer the array size through template magic which is far superior (less error prone) than having programmers specify it.

With that said, it makes me sad that this code fails with /analyze:

void wcstombsTest(const wchar_t* pSrc) {
char dest[MAX_PATH];
size_t count;
// Use the template override version
wcstombs_s( &count, dest, pSrc, _TRUNCATE );

// Until this bug is fixed use wcstombs_s this way:
//wcstombs_s( &count, dest, ARRAYSIZE(dest), pSrc, 10 );
}

The reason it fails is because the template version of wcstombs_s is annotated to say how much memory its destination pointer is pointing to. The problem is, it’s not a pointer, it’s a reference to an array. So, /analyze quite sensibly spits out four warnings complaining of invalid annotations in stdlib.h. Oops.

The one true ARRAYSIZE macro

A common thing to assert is that array indices are in range, and for that purpose an ARRAYSIZE macro is very handy. A good ARRAYSIZE macro should be entirely evaluated at compile time and it should fail to compile if it is accidentally passed a pointer instead of an array. Microsoft puts a version of ARRAYSIZE in winnt.h which uses template magic to ensure that it will only compile for arrays, except that when you do /analyze builds it switches implementations to one that will also compile for pointers!

If you change this implementation to use template magic in all cases then the following reasonable code will trigger a warning:

// warning C6260: sizeof * sizeof is usually wrong.
// Did you intend to use a character count or a byte count?
const size_t size = MYARRAYSIZE(arr1) * sizeof(arr1[0]);

The correct solution, as found in Microsoft’s _countof macro, is to add zero in the intermediate macro in order to avoid multiplying sizeof by sizeof. So here is the one-true macro to get the size of an array:

template <typename T, size_t N>
char (*MyNumberOf( T (&)[N] ))[N];
// Add zero to avoid warnings when multiplying
// ARRAYSIZE and sizeof(arr[0])
#define ARRSIZE(A) (sizeof(*MyNumberOf(A))+0)
#define MYARRAYSIZE(A) ARRSIZE(A)

Not equal to NULL

/analyze warns about the code below. If you delete the “!= NULL” then it realizes that all is well. Huh.

const char* const NullTerm[] = {
    “string1”,
    “string2”,
    NULL
};

void PrintStrings() {
    // warning C6385: Invalid data: accessing ‘NullTerm’, the
    // readable size is ’12’ bytes, but ’16’ bytes might be read
    for (int i = 0; NullTerm[i] != NULL; ++i)
        printf(“%s”, NullTerm[i]);
}

Qsort

All the cool kids use std::sort, but qsort still shows up in legacy code and it still works. But /analyze can’t handle it.

int intCompare(const void* p1, const void* p2) {
    return memcmp(p1, p2, sizeof(int));
}

void TestQsort(int count) {
    int* data = new int[count];

    for (int i = 0; i < count; ++i)
        data[i] = rand();

    for (int i = 0; i < count; ++i)
        printf(“data[%d] = %d\n”, i, data[i]);

    // This makes /analyze think that all of the data is no longer valid.
    qsort(data, count, sizeof(data[0]), intCompare);

    for (int i = 0; i < count; ++i)
    {
        // warning C6385: Invalid data: accessing ‘data’,
        // the readable size is ‘1*0’ bytes, but ‘4’ bytes might be read
        printf(“data[%d] = %d\n”, i, data[i]);
    }

    delete [] data;
}

As soon as TestQSort calls qsort /analyze assumes that all the memory pointed to by data has been uninitialized, or freed, or something. I don’t know of an acceptable way to suppress this warning.

Zero valid bytes redux

I don’t understand this warning. Just like with qsort /analyze is convinced that ptr points to zero bytes but I see no justification for its belief. Various ways of rearranging this code make the warning go away, but that’s not a technique I’m willing to employ.

void SetClipboardText(const wchar_t *text, int textLen) {
    BOOL cb = OpenClipboard(GetDesktopWindow() );
    if (!cb)
        return;

    EmptyClipboard();

    size_t length = (textLen + 1) * sizeof(wchar_t);
    HANDLE hmem = GlobalAlloc(GMEM_MOVEABLE, length);
    if (hmem) {
        void *ptr = GlobalLock(hmem);
        if (ptr != NULL) {
            memset(ptr, 0, (textLen + 1) * sizeof(wchar_t));
            // warning C6386: Buffer overrun: accessing ‘argument 1’, the
            // writable size is ‘1*0’ bytes, but ‘2’ bytes might be written:
            memcpy(ptr, text, textLen * sizeof(wchar_t));
            GlobalUnlock(hmem);

            SetClipboardData( CF_UNICODETEXT, hmem );
        }
    }
      
    CloseClipboard();
}

Bit counting

A popular Microsoft interview question is how to count how many bits are set in an integer. It is amusing that /analyze fails this test. It thinks that the loop in this function will never terminate, but I’ve tested it and verified that it does – after counting all of the set bits.

int BitSetCount64(unsigned __int64 num) {
    int bits = 0;

    // warning C6295: Ill-defined for-loop: Loop executes infinitely
    for (unsigned __int64 x = num; x; x = x & (x-1))
        ++bits;
    return bits;
}

/analyze has no problem with the 32-bit version of this loop.

More problems with annotations

The following code is a bit odd – a template function to call strncpy with any type for the size, called with an enum for the size – but it is legal. However /analyze doesn’t like it:

template< typename T > inline int V_strncpy( _Out_z_bytecap_(destSize) char* pDest, const char* pSrc, T destSize )
{
    strncpy_s( pDest, destSize, pSrc, _TRUNCATE );
    return 0;
}

enum { RENDER_ADAPTER_NAME_LENGTH = 512 };

void TestCopy()
{
    char buffer[RENDER_ADAPTER_NAME_LENGTH];

    V_strncpy(buffer, “test”, RENDER_ADAPTER_NAME_LENGTH);
}

/analyze says:

Invalid size specification: expression must be of integral type

It is my understanding that an enum is an integral type, and certainly the regular compiler has no trouble with this. It is important that the /analyze compiler be able to consume all of the code that the regular compiler can handle. I’ve changed a few enums to “const int” to work around this.

Summary

As I said at the top, /analyze is still valuable – there’s no doubt about that – but at times it feels a bit unpolished. I’m writing up descriptions of these warnings both to help educate others who might try to use this feature, and as a bug list for the VC++ compiler team. I’m hopeful that the next version of VC++ will correct most or all of these oversights so that I can spend more of my time investigating real bugs, and less time chasing synthetic insects.