Why does this code emit buffer overrun warnings(C6385/C6386) in code analysis on Visual Studio 2012?

Question

I'm trying using the code analysis function of Visual Studio 2012. I just have run them over my existing project and found some buffer overrun warnings(C6385/C6386) on the part which contains my own implementation of Knuth's subtractive PRNG(aka RAN3). However, I can't understand why this happens because it looks fine(I can see no out-of-bound reads/writes). So I made a short equivalent(below) of that part but still got the same warnings and can't figure out the cause of them.

int main() {
  unsigned int k = 1U, seed = 12345U, randomNumbers[55];

  randomNumbers[54] = seed;
  for(unsigned int i = 1U; i <= 54U; ++i) {
    unsigned int ii = ((21U * i) % 55U) - 1U;
    randomNumbers[ii] = k;
    k = seed - k;
    seed = randomNumbers[ii];
  }

  return 0;
}

With the code above, I got a C6386 warning on line 7 and a C6385 on line 9. What wrong with this code? Am I missing something?

Answer 1

g++ 4.8 and clang++ 3.3 compile this without warning or error (using -Wall -Werror). In fact, we can use C++11's std::array and its at method to get bounds checking, and

#include <array>

int main() {
  unsigned int k = 1U, seed = 12345U;
  std::array<int,55> randomNumbers;

  randomNumbers.at(54) = seed;

  for(unsigned int i = 1U; i <= 54U; ++i) {
    unsigned int ii = ((21U * i) % 55U) - 1U;
    randomNumbers.at(ii) = k;
    k = seed - k;
    seed = randomNumbers.at(ii);
  }

  return 0;
}

yields no out-of-bounds accesses, as you claimed. I think your code is fine. VS is worried that the line ((21U * i) % 55U) - 1U) may result in 0 - 1, which will overflow because ii is an unsigned int. If you use ints instead of unsigned ints, does VS still complain?

(Using Python, your index mapping seems fine:

>>> sorted([21*n % 55 - 1 for n in range(1,55)])
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53]

There shouldn't be any out-of-bounds errors, especially since you won't "reach" -1 using unsigned ints.)

Answer 2

print the value of ii, see if its going beyond 54 Calculated by this line unsigned int ii = ((21U * i) % 55U) - 1U;

Answer 3

First off please understand the purpose of Static Analysis, it would perform the analysis of the code base to scrutinize and make sure that code complies with they industry standards. This is very first step in software quality control and always assisted with dynamic analysis to find subtle issues and vulnerabilities which static analysis is unable to find. Fuzzing is used to ensure software security and quality with dynamic analysis.

Static code analysis can result in false positive too which code auditor should ignore by. Your case reported here is false alarm.

#pragma warning(suppress:6385)

Now let's dig into your scenario, static analysis finds that your instruction ((21U * i) % 55U) COULD end up evaluating to zero which could result in ii = -1; this can further lead to potential crash or even security exploitation in your software (buffer overflow attack). Static analysis would not execute all of loop iterations (unlike dynamic analysis) to assert that every branch of code is executing well.

Now lets talk about your code, I have some recommendations and improvements for you. Please introduce some the constant or #define to hold the size of your array.

#define _SIZE 55U

Now instead of using hard coded numbers '55' and '54' start using _SIZE in your code. You can introduce the utility function to get safe bounds for an array.

int SafeBoundsInt32(int min, int max, int value)
{
    if (value < 0)
        return 0;
    if (value >= max)
        return max - 1;
    //Valid value
    return value;
}

I am going to change your code based upon the function introduced.

int main()
{
    unsigned int k = 1U, seed = 12345U, randomNumbers[_SIZE];

    randomNumbers[_SIZE - 1] = seed;
    for (unsigned int i = 1U; i <= _SIZE - 1; ++i)
    {
        unsigned int ii = ((21U * i) % _SIZE) - 1U;
        randomNumbers[SafeBoundsInt32(0, _SIZE, ii)] = k;
        k = seed - k;
        seed = randomNumbers[SafeBoundsInt32(0, _SIZE, ii)];
    }

    return 0;
}

Perform the code analysis now and it would report no warning.