Visual Studio 2015 Code Analysis C6386 warns of buffer overrun

Question

I've read a lot about the Visual Studio Code Analysis warning C6386, but can't figure out this particular issue with my code. I've reduced it to the following small program:

unsigned int nNumItems = 0;

int main()
{
    int *nWords=nullptr;
    unsigned int nTotal;

    nTotal = 3 + 2 * nNumItems;
    nWords = new int[nTotal];

    nWords[0] = 1;
    nWords[1] = 2; // this is line 18, warning C6386

    delete[] nWords;
    return 0;
}

Analyze->Run Code Analysis->On Solution will give the following warning:

file.cpp(18): warning C6386: Buffer overrun while writing to 'nWords': the writable size is 'nTotal*4' bytes, but '8' bytes might be written.

Is this legit? Now, if I move my global variable and make it local, the warning disappears!

int main()
{
    unsigned int nNumItems = 0;
...
}

But I can't do this as in the full code, this is a member variable.

Likewise, if I move the definition of nTotal into the 'new int', I can also remove the warning:

    nWords = new int[3 + 2 * nNumItems];

But I can't do this as nWords is referred to in other places in the full code.

Is this just an issue with the Visual Studio static code analyzer, or is it a legitimate issue with this code?

Bissextile

Won't "fix" the problem but use std::vector<int> instead. – Distinct

When it's global, perhaps the compiler thinks something else may change it. So if nNumItems = 0xffffffff, then it would be a buffer overflow. But it doesn't change that fact that the warning is ridiculous in this case. – Incardination

Answer 1

Static code analysis is hard, tracing possible values of an expression like 3 + 2 * nNumItems is hard, and tracing actual possible values is often near to impossible. This is why it is a warning, not error. So far for the obvious.

Now, looking at how you describe that this warning behaves, I would bet on a "bug", or rather, I should say it with less stress, deficiency, in the static analyzer.

I can see some imaginary possible causes behind this warning on original nWords[1] = 2 and global nNumItems. They are really weird and I don't think a reasonable analyst would add such rules to the analyzer. Also, I were right, then you should have these warnings on nWords[0] = 1 as well.

The fact that you don't see them on that proves my ideas wrong, so I stop here.

Instead, I would like to focus on that static code analysis is hard. It does not matter how well-written the analyzer and its rules are. For some cases, it will make errors, and for other cases it will simply fail and won't be even able to guess, and for other cases it will timeout and just let go. Until we have some breakthrough in AI or breakthrough in solving NP-hard problems, you will probably have to get used to the fact, that when you are using static code analyzers, then you have to write code in a way that they can comprehend, not count on that they can comprehend everything you can write.

Last thought, when I see this error:

file.cpp(18): warning C6386: Buffer overrun while writing to 'nWords': the writable size is 'nTotal*4' bytes, but '8' bytes might be written.

the first thing that I notice is nTotal*4 and 8. If you were using a hardcoded values, you would probably get an error like

file.cpp(18): warning C6386: Buffer overrun while writing to 'nWords': the writable size is '1024' bytes, but '8192' bytes might be written.

The fact that you see nTotal*4 seems to imply that the static code analyzer actually failed to guess the value under nTotal and it left it as symbolic name, which formed an expression incomparable to 8. Therefore, the analyzer did theonly thing it could - it reported a problem, and described it as well as it could. Still, it's just my guess.

// EDIT - note for Dan's answer about guessing: nNumItems <- SIZE_MAX

I actually think that he's may be quite about the SIZE_MAX. I played a bit with some SAT solvers from Microsoft, and one thing they did well was solving set of constraints in integer domain. Actually unsigned int x = SIZE_MAX; std::cout << ( (3+2*x)*sizeof(int) ); prints 4 (of course), and that is the only value of x for which the expression is less than 8.

I'm pretty sure that the constraint solver from microsoft I played with can detect this case when checking satisfiability of ((3+2*x)*4) < 8 in integer ring domain - hence the warning could be issued. However, I would expect the warning to include the result and print something like:

nTotal*4 < 8 when {nTotal=1,nNumItems=4294967295}`

since the analyzer had would have this information already. But then, that's.. probably expecting too much from it. Developers behind it probably wouldn't have thought of formatting such detailed warning message, or considered current format of the message to be more user-friendly.

Lolitaloll

Static analysis is at least as hard as the halting problem. That's why static-analyzers don't always do a good job. – Incardination

Answer 2

Since nNumItems is global, it would appear that code analyzer thinks that nNumItems might be set to SIZE_MAX elsewhere before your code executes. You can see this with a sample like:

size_t nNumItems = 0;

void foo()
{
    nNumItems = SIZE_MAX;
}
void bar()
{
    const size_t nTotal = 3 + 2 * nNumItems;
    auto nWords = new int[nTotal];

    nWords[0] = 1;
    nWords[1] = 2;
}

int main()
{
    foo();
    bar();

    return 0;
}

Perhaps the best fix is to side-step the entire problem by using std::vector<int>.

Seeing

Answer 3

Make the Global a const:

const unsigned int nNumItems = 0;

Rage

presumably other parts of the code want to change nNumItems – Granvillegranvillebarker

Answer 4

This may not be a common solution but it my case, VS was generating what I believe to be a false positive warning. Upon restarting VS, the warning went away.

Tyndareus

Warnings do not go away by restarting the IDE. Of course, you won't see them directly because the message window will be empty on start, but the warning will reappear every time you compile the code. Besides, the other answers already proved it's not a false positive. There are actual use cases where it could lead to a buffer overrun. – Benuecongo