Prefer a warning for when VLAs declared on stack

[Quuxplusone]

Bugzilla Link	PR48460
Status	NEW
Importance	P enhancement
Reported by	David Svoboda (svoboda@cert.org)
Reported on	2020-12-09 09:36:27 -0800
Last modified on	2020-12-10 11:13:02 -0800
Version	trunk
Hardware	Macintosh MacOS X
CC	blitzrakete@gmail.com, dgregor@apple.com, erik.pilkington@gmail.com, josephcsible@gmail.com, llvm-bugs@lists.llvm.org, richard-llvm@metafoo.co.uk, svoboda@cert.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

The Clang docs (https://clang.llvm.org/docs/DiagnosticsReference.html#wvla)
define the "-Wvla" flag to do the following:

    Also controls -Wvla-extension.
    warning: variable length array used

    Warn if a variable-length array is used in the code. -Wno-vla prevents the -Wpedantic warning of the variable-length array.

The -Wvla flag causes both GCC 10 and Clang 12 to holler about this declaration
having a VLA:

  void func1(int n, int array[n]);

This happens even if func1 is called with a regular array or an int pointer.
Whether you consider 'array' to be a VLA or not depends on how you interpret
ISO C17 6.7.6.3p4.  A colleague called the declaration of array a "heisen-VLA"
on the grounds that array may be cast to a VLA before being immediately cast to
an int pointer.

Clang 12 also hollers about this line, but GCC 10 doesn't:

  void func2(int array[*]);

ISO C17 6.7.6.3p4 is pretty clear that an array declared this way is indeed a
VLA.

But both code examples use VLAs only as an actual parameter argument type. The
main hazard of VLAs is being declared as a stack variable with an unsecured
dimension, where they could potentially exhaust your stack.

Most experts on VLAs would suggest that casting something to a VLA is not a
problem per se, and the real danger of VLAs is declaring a VLA on the stack
(because of the potential for stack exhaustion).  However, neither GCC nor
Clang seem to have a warning to detect VLA stack declarations. This would be a
useful feature, as either a replacement for -Wvla's current behavior, or for a
new warning flag.

    void func1(int n, int array[n]) { /* ok, no warning */
      int array2[n];     /* bad, VLA on stack, warn! */
      int (*array3)[n];  /* ok, no VLA on stack, so no warning */
    }

Finally, declaring a function argument type as a VLA with an explicit (non-
compile-time) array bounds can improve software security, as a VLA bounds
conveys useful semantic information to programmers. Also a VLA bounds can be
checked by the compiler or a static-analysis tool. At CERT, we call such an
array a "conformant array". For more background, see CERT guideline:

  API05-C. Use conformant array parameters
  https://wiki.sei.cmu.edu/confluence/x/n9UxBQ

I have also submitted a similar bug report to GCC, it is here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

[Quuxplusone]

I've just replied to the GCC bug thread: my preferred approach here would be to suppress -Wvla in function parameter types. But that's only really useful if GCC is prepared to make the same change.

[Quuxplusone]

In the other thread, you said "warning only in cases where there is a variably-modified type on the stack seems like the better meaning for -Wvla", which I agree with. That sounds like we should only warn on array2, but the approach of suppressing it for only function parameter types would mean we'd actually warn on both array2 and array3. What if we instead suppressed it for types that are pointers after possible decay? Then we would really only warn on array2.

[Quuxplusone]

Yes, you're right; only warning on variably-modified types on the stack is the better rule and the one I meant :-)

(There's also the issue that bound of array3 is not checked in any meaningful way by the C specification, but at least static analysis tools can verify it. I wonder if it'd make sense to permit 'static' there?)