[PATCH] D79330: [Analyzer][VLASizeChecker] Check for VLA size overflow.

Wed May 13 10:17:29 PDT 2020

martong accepted this revision.
martong added a comment.

In D79330#2033990 <https://reviews.llvm.org/D79330#2033990>, @Szelethus wrote:

> > Variable-length array (VLA) should have a size that fits into a size_t value. At least if the size is queried with sizeof, but it is better (and more simple) to check it always
>
> So creating VLA larger than `sizeof(size_t)` isn't a bug, bur rather a sign of code smell? Then we shouldn't create a fatal error node for it, **unless** we're trying to fit it in a variable that isn't sufficiently large. The fact that `sizeof`ing it is a bug wasn't immediately obvious to me either, so a quote from the standard as comments would be appreciated:
>
> §6.5.3.4.4 <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf>, about operator sizeof: The value of the result is implementation-defined, and its type (an unsigned integer type) is `size_t`, defined in `<stddef.h>` (and other headers).

I am not sure if I can follow your concern here.
`sizeof(size_t)` is typically 8, so that is not a bug, neither a code smell to have `char VLA[sizeof(size_t)];`. The problem is when the size is bigger than the maximum value of `size_t`, that ix 0xff...ff, as we can see that in the new tests.
Besides, not having the size printed out in the warning is not a blocker for me, this looks good enough.

================
Comment at: clang/test/Analysis/vla-overflow.c:10
+    // Size of this array should be the first to overflow.
+    size_t s = sizeof(char[x][x][x][x]); // expected-warning{{Declared variable-length array (VLA) has too large size}}
+    return s;
----------------
Szelethus wrote:
> Let's not trim be checker name here.
> 
> Also, we could mention what the specific size is.
Yes, that's a good idea to print the actual size of the VLA when we have that info. But I think we cannot just print that when it overflows! :D We can, however, print the maximum allowed value in case of the overflow.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79330/new/

https://reviews.llvm.org/D79330