[Bug c/98217] New: Prefer a warning for when VLAs declared on stack

[Bug c/98217] New: Prefer a warning for when VLAs declared on stack

[svoboda at cert dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

            Bug ID: 98217
           Summary: Prefer a warning for when VLAs declared on stack
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: svoboda at cert dot org
  Target Milestone: ---

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[svoboda at cert dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

David Svoboda <svoboda at cert dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |svoboda at cert dot org

--- Comment #1 from David Svoboda <svoboda at cert dot org> ---
The GCC docs (https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html) define
the "-Wvla" flag to do the following:

    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


(This bug report reports the same problem as
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85997 but provides a more
comprehensive solution.)

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[svoboda at cert dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

--- Comment #2 from David Svoboda <svoboda at cert dot org> ---
I have also submitted a similar bug report to Clang, it is here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           Severity|normal                      |enhancement
                 CC|                            |msebor at gcc dot gnu.org
           Keywords|                            |diagnostic
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2020-12-09

--- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
-Wvla-larger-than=<byte-size> seems close to the feature you're looking for:

$ cat pr98217.c && gcc -O2 -S -Wvla-larger-than=0 pr98217.c
void f (void*, ...);

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 */
  f (array, array2, array3);
}
pr98217.c: In function ‘func1’:
pr98217.c:5:7: warning: argument to variable-length array may be too large
[-Wvla-larger-than=]
    5 |   int array2[n];     /* bad, VLA on stack, warn! */
      |       ^~~~~~

The warning triggers whenever a VLA is either a) known to be larger than the
limit or b) not known to be less than or equal to it.  Please let us know if
this isn't sufficient.

For better portability, the solution to the VLA bound parameter problem I would
recommend for single-dimensional VLAs over the forward parameter declaration is
attribute access:

$ cat pr98217-2.c && gcc -S pr98217-2.c
typedef __SIZE_TYPE__ size_t;

__attribute__ ((access (read_write, 1, 2)))
void my_memset (char *p, size_t n, char v)
{
  __builtin_memset (p, v, n);
}

char a[3];

void f (void)
{
  my_memset (a, 3 * sizeof a, 0);
}
pr98217-2.c: In function ‘f’:
pr98217-2.c:13:3: warning: ‘my_memset’ accessing 9 bytes in a region of size 3
[-Wstringop-overflow=]
   13 |   my_memset (a, 3 * sizeof a, 0);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pr98217-2.c:4:6: note: in a call to function ‘my_memset’ declared with
attribute ‘access (read_write, 1, 2)’
    4 | void my_memset (char p[], size_t n, char v)
      |      ^~~~~~~~~

(Worth noting is that neither works reliably when the function is inlined and
the association between the array and its bound is lost.)

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[svoboda at cert dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

--- Comment #4 from David Svoboda <svoboda at cert dot org> ---
Martin:

Thanks. It looks like -Wvla-larger-than=0 is (theoretically) a good way to
catch VLA stack declarations.

There is still the issue that GCC's -Wvla did not flag use of array[*].  To me
that is lower-priority, but is still an issue.

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[svoboda at cert dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

--- Comment #5 from David Svoboda <svoboda at cert dot org> ---
Oops, the Clang bug entry is really here:
https://bugs.llvm.org/show_bug.cgi?id=48460

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[richard-gccbugzilla at metafoo dot co.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

Richard Smith <richard-gccbugzilla at metafoo dot co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |richard-gccbugzilla@metafoo
                   |                            |.co.uk

--- Comment #6 from Richard Smith <richard-gccbugzilla at metafoo dot co.uk> ---
Hi GCC folks! My preferred approach to take on the Clang side would be to
change -Wvla to not warn on function parameter types, if GCC is prepared to
make the same change. I don't think the current meaning of -Wvla is useful, and
warning only in cases where there is a variably-modified type on the stack
seems like the better meaning for -Wvla.

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

Martin Uecker <muecker at gwdg dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |muecker at gwdg dot de

--- Comment #7 from Martin Uecker <muecker at gwdg dot de> ---


int (*array3)[n];

on the stack should also be allowed as this enables bounds checking, i.e.

(*array3)[n] = 1;

is UB (and can be diagnosed at run-time by the UB sanitzer).

Non matching bounds at the function call boundary are also UB and could be
diagnosed.

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jsm28 at gcc dot gnu.org

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
For reference, -Wvla was introduced in this change:
  https://gcc.gnu.org/legacy-ml/gcc-patches/2007-02/msg00577.html
The description says "The main intention is to support checking of coding
styles
that don't allow variable-length arrays."
That's a little vague but since the common rationale for avoiding VLAs is that
they easily exhaust stack space (or otherwise manipulate the stack pointer in
an undefined way), relaxing -Wvla to be the equivalent of -Wvla-larger-than=0
would make sense to me.  To preserve the detection of all VLA uses (and to add
to it [*]) -Wvla could be extended to add a new level, say, -Wvla=2, at which
is would serve that purpose.  (Being a conditionally supported feature,
detecting all their uses might be helpful for portability.)

Joseph, do you have any concerns with any of this?

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[svoboda at cert dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

--- Comment #9 from David Svoboda <svoboda at cert dot org> ---
The more I think about it, the more I want to see two -W warnings...one to flag
VLA declarations on the stack, and another to see *any* use of VLAs, even if in
something as harmless as a conformant array. The latter flag is simply for
platforms (like C++) that does not support VLAs.

I am less concerned about codebases that have a "no VLAs" rule. They have the
same problem we do of understanding what chat constitutes a VLA, and they would
benefit the most from well-defined, precise -W warnings.

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

--- Comment #10 from Martin Uecker <muecker at gwdg dot de> ---
(for C++ compatibility, there is: -Wc++XX-compat )

So since Xmas is coming: 

- I would also like an option to allocate VLAs of unbounded size larger than X
on the heap. 
- Better code generation for VLAs. Maybe VLAs of small bounded size could be
automatically transformed into regular arrays for the purpose of code
generation.


IIRC, Linux kernel replaced some VLAs on the stack with fixed-sized buffers on
the stack. But this increases stack size and reduces type safety.

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

--- Comment #11 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
It would seem reasonable to have options both for the case of warning 
about all VLA declarations, and more specifically for the case of 
allocating a VLA on the stack.  The diagnostics for all VLA declarations, 
including [*], are needed in any case for -std=c90 -pedantic (as pedwarns, 
so they become errors with -pedantic-errors) and -Wc90-c99-compat / 
-Wc++-compat (as warnings).

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |msebor at gcc dot gnu.org
   Last reconfirmed|2020-12-09 00:00:00         |2020-12-18

--- Comment #12 from Martin Sebor <msebor at gcc dot gnu.org> ---
Okay then, let me try to handle this for GCC 12.

[Bug c/98217] Prefer a warning for when VLAs declared on stack

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98217

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
           Assignee|msebor at gcc dot gnu.org          |unassigned at gcc dot gnu.org

--- Comment #13 from Martin Sebor <msebor at gcc dot gnu.org> ---
I'm not working on this anymore.