3

Visual Studio 2019 started showing Code Analysis warnings as in-editor green squiggles by default. These may be extremely useful for students learning C programming, because they catch classical mistakes, such as off by one array accesses.

Unfortunately false positives may completely ruin the learning experience and I fear that I will have to ask the students to disable the feature in order to avoid having them worry on non existing problems.

This short snippet doesn't cause any warning:

#include <stdlib.h>

int main(void)
{
    size_t n = 6;
    int *v = malloc(n * sizeof(int));
    if (v == NULL) {
        return 1;
    }
    for (size_t i = 0; i < n; ++i) {
        v[i] = i;
    }
    free(v);
    return 0;
}

Unfortunately, if you move the allocation in a function, like this:

#include <stdlib.h>

int *test(size_t n)
{
    int *v = malloc(n * sizeof(int));
    if (v == NULL) { 
        return NULL;
    }
    for (size_t i = 0; i < n; ++i) {
        v[i] = i;
    }
    return v;
}

int main(void)
{
    size_t n = 6;
    int *v = test(n);   
    free(v);
    return 0;
}

you get a warning C6386: Buffer overrun while writing to 'v': the writable size is 'n*sizeof(int)' bytes, but '8' bytes might be written.

Even reading on Stack Overflow, I don't get where the '8' comes from, but, more importantly, why it fails to recognize that i will never be out of range.

So the question is: is there a way to write this type of code in a way that will not generate the warning?

I know that I can go to Tools > Options > Text Editor > C/C++ > Experimental > Code Analysis and set Disable Code Analysis Squiggles to True, or use a #pragma warning(disable:6386), but I'd rather avoid it, and certainly avoid suggesting my students the latter.

Costantino Grana
  • 3,132
  • 1
  • 15
  • 35
  • 1
    Anyway, this looks like a typical "false positive", it's simply a bug in the analysis tool. The standard procedure when working with static analysers is to disable all diagnostics that are broken and buggy. Btw you shouldn't be using Visual Studio for teaching C in the first place, it is non-compliant and terribly outdated. – Lundin Nov 06 '20 at 11:44
  • @Lundin: on the Code Analysis warning, the point is that it works well for many real cases and can be a real help for students, so disabling it is a pity. – Costantino Grana Nov 06 '20 at 11:50
  • 1
    @Lundin: I have to disagree with the fact that Visual Studio is "non-compliant and terribly outdated", but I won't go into a religious war for that. I think that it is much worse to teach using the command line with `printf()` as the debugger tool of choice... – Costantino Grana Nov 06 '20 at 11:52
  • 2
    @CostantinoGrana: MS announced this past September that they are *just now* adding support for C11 and C17 to the VS compiler. Yeah, it’s been horribly out of date for a while. – John Bode Nov 06 '20 at 12:10
  • The thing is, `n` might not necessarily wrap-around at `SIZE_MAX`, see https://stackoverflow.com/questions/42574890/why-is-the-maximum-size-of-an-array-too-large. – Lundin Nov 06 '20 at 12:30
  • @CostantinoGrana VS has full support for an obsolete version of C from 1990 and partial support for an obsolete version from 1999. If I wrote a Windows program with full support for Windows 3.1 from 1991 and partial support for Windows 98 and released it in 2019, then what would you call that program? – Lundin Nov 06 '20 at 12:37
  • This is unfortunately one of the consequences of using a language that makes it difficult to write verifiably safe code: any IDE that deals with that language has to assume that everything is going to blow up everywhere. Which is why I wouldn't ever teach students C, or C++; Java or C# are far better choices. – Ian Kemp Nov 06 '20 at 12:39
  • @Lundin "The thing is, `n` might not necessarily wrap-around at `SIZE_MAX`," In this case, I believe that is only something for the implementation of `malloc` (+ `realloc`, `calloc`) to worry about. – Ian Abbott Nov 06 '20 at 12:58
  • @Lundin: "The thing is, `n` might not necessarily wrap-around at `SIZE_MAX`," Are you sure? `size_t` is unsigned and required to wrap at the 0xfff... value that matches its type. Shouldn't it be SIZE_MAX? Maybe a question would be more appropriate... – Costantino Grana Nov 07 '20 at 04:00
  • @CostantinoGrana See the posted link. – Lundin Nov 08 '20 at 15:31

2 Answers2

3

I really want to thank everybody for their contributions and I agree that it is a bug in the Code Analyzer (by looking on Microsoft web sites it has been "Closed - Lower Priority" two years ago...).

Adrian Mole max(n, 0) trick points to a way for coping with the warning in code, that is checking that n is greater than zero. The funny thing is that you can still use that zero for what n was supposed to be used. While the idea could be used for experienced programmers (that would probably disable the warning), as John Bollinger points out, it's not for students.

So, after telling the students that it's a bug and how to turn off the Code Analysis squiggles or disable the warning, I'd go with

int *test(size_t n)
{
    if (n == 0) {
        return NULL;
    }
    int *v = malloc(n * sizeof(int));
    if (v == NULL) {
        return NULL;
    }
    for (size_t i = 0; i < n; ++i) {
        v[i] = i;
    }
    return v;
}

Which may also be interpreted as: don't allow 0 elements allocation.

Costantino Grana
  • 3,132
  • 1
  • 15
  • 35
1

You can supress this warning (which could be considered a bug), simply by ensuring the value of n given to the malloc call has not "wrapped around" following overflow (as hinted at in the comment from Eric Postpischill).

To do this, you can replace the n argument by the seemingly bizarre max(n,0):

int* test(size_t n)
{
//  int* v = malloc(n * sizeof(int));         // Warning C6386 on v[i] = i
    int* v = malloc(max(n, 0) * sizeof(int)); // No warning
    if (v == NULL) {
        return NULL;
    }
    for (size_t i = 0; i < n; ++i) {
        v[i] = i;
    }
    return v;
}
Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
  • 1
    I guess it is a fluke that this makes any difference to the warning. For example, the warning returns when reversing the arguments of `max`. – Ian Abbott Nov 06 '20 at 12:44
  • @IanAbbott Fluke, for sure. I guess it's the way the `max` macro is defined (using a ternary operator). – Adrian Mole Nov 06 '20 at 12:49
  • 1
    It's interesting that that persuades the static analyzer that the code is ok, but from a point of view of instructing students, I think that demonstrating such a workaround, much less teaching students to use it, would be worse than living with the warning. At least with the warning, you are reminded to pay careful attention to the allocation involved to judge whether the warning is well founded. – John Bollinger Nov 06 '20 at 12:56
  • @JohnBollinger I agree. However, if the students are forced to use the MSVC code analyser, one has limited options: teach them to live with the warning, show them how to disable it using `#pragma`, or tell them it's a bug in MSVC and show them how to fool that tool. – Adrian Mole Nov 06 '20 at 13:03
  • But the 'workaround' I've suggested here may be useful for other developers, with large code bases, who would like to supress that warning in places where such false positives are otherwise shown. – Adrian Mole Nov 06 '20 at 13:05
  • @IanAbbott: reversing the arguments on VS2017 Code Analyzer still suppresses the warning, as well as using any other number in the max. Which of course has not the correct behavior. – Costantino Grana Nov 07 '20 at 04:40