/analyze for Visual Studio–the Ugly (part 1)

Introduction

/analyze is a powerful Visual C++ feature that can help to find bugs. However it uses some heuristics which are rather peculiar and which make it difficult to decide how seriously to take its warnings. Today we discuss one of those peculiar heuristics, and show a case where /analyze is flat out wrong.

Update: these peculiar warnings are mostly fixed in VS 2012. Most of the bad warnings in the repro project that I shared with Microsoft were addressed, so the false positive rate is lower.

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

The gory details

Take a look at this piece of code:

int g_values[10];

int GetValue1(int index)
{
    return g_values[index];
}

This piece of code is neither good nor bad, correct nor broken. It all depends on how it is used. It should probably have an assert to add a run-time check in debug builds for ‘index’ being in range, and index should probably be ‘unsigned’ or ‘size_t’, but it is not intrinsically broken. One could argue that since ‘index’ could potentially range from INT_MIN to INT_MAX that /analyze should warn about potential out-of-range accesses to g_values, but if /analyze did that then it would issue so many low-value warnings that it would be useless. So /analyze, having nothing useful to say about this function, wisely remains silent.

This piece of code is a different story. It is dangerously broken. It reads beyond the end of the array every single time which gives undefined behavior and should generally be avoided.

int GetValue2()
{
    return g_values[_countof(g_values)];
}

/analyze is smart enough to handle this. It gets a bit carried away and gives two warnings, but better safe than sorry, right?

warning C6200: Index ’10’ is out of valid index range ‘0’ to ‘9’ for non-stack buffer ‘int * g_values’
warning C6385: Invalid data: accessing ‘int * g_values’, the readable size is ’40’ bytes, but ’44’ bytes might be read

So far so good. Now things get weird. This piece of code is, from a correctness point of view, identical to GetValue1. But /analyze begs to differ:

int GetValue3(int index)
{
    if (index == 8 )
        return 0;

    return g_values[index];
}

To this benign piece of code /analyze says:

warning C6385: Invalid data: accessing ‘int * g_values’, the readable size is ’40’ bytes, but ’44’ bytes might be read

A bit of experimentation with the value that index is compared against reveals what /analyze appears to be thinking. If you compare ‘index’ to an integer then /analyze assumes that this means that you will be indexing into g_values with an index 2 greater, in this case an index of 10. An index of 10 means you read the 11th element which extends out to the 44th byte.

If you compare index against 100 as shown below:

int GetValue4(int index)
{
    if (index == 100)
        return 0;

        return g_values[index];
}

then naturally /analyze assumes that you will be indexing into g_values with an index of 102 so it gives this warning:

warning C6385: Invalid data: accessing ‘int * g_values’, the readable size is ’40’ bytes, but ‘412’ bytes might be read

Quite strange to say the least. When encountered in your code base with no explanation these warnings are practically impossible to understand. Only if you are the odd type who starts writing test functions for a /analyze blog post do you have any hope of figuring out the internal logic.

An equally annoying problem with /analyze’s ability to track indices is shown with the code below.

int GetValue6()
{
    int sum = 0;
    for (int index = 0; index < _countof(g_values) * 2; ++index)
        if (index < _countof(g_values))
            sum += g_values[index];
        else
            sum += g_values[index – _countof(g_values)];

    return sum;
}

The index variable loops over twice the extent of the array, but there are carefully written checks to ensure that index is always used correctly. Despite this code being provably correct and safe, /analyze spits out the following pair of warnings for the impossible scenarios that keep it up late at night.

warning C6200: Index ’19’ is out of valid index range ‘0’ to ‘9’ for non-stack buffer ‘int * g_values’
warning C6200: Index ‘-10’ is out of valid index range ‘0’ to ‘9’ for non-stack buffer ‘int * g_values’

Summary

Unfortunately these inappropriate or wrong warnings make /analyze much harder to use. Each warning must be carefully analyzed to decide whether it is a brilliant insight into a subtle bug, a bizarre revelation about /analyze’s internal workings, or just plain wrong. This requires expert knowledge of a type that makes the arcane rules of C++ seem simple by comparison.

These warnings then either have to be suppressed or somehow marked as uninteresting. Either way these bugs prevent /analyze from being used by all but the most dedicated developer.