[Bug c++/110848] New: Consider enabling -Wvla by default in C++ modes

[Bug c++/110848] New: Consider enabling -Wvla by default in C++ modes

[aaron at aaronballman dot com]

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

            Bug ID: 110848
           Summary: Consider enabling -Wvla by default in C++ modes
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: aaron at aaronballman dot com
  Target Milestone: ---

VLAs as they're expressed in C have been considered by WG21 and rejected, are
easy to use accidentally to the surprise of users (e.g.,
https://ddanilov.me/default-non-standard-features/), and they have potential
security implications beyond constant-size arrays
(https://wiki.sei.cmu.edu/confluence/display/c/ARR32-C.+Ensure+size+arguments+for+variable+length+arrays+are+in+a+valid+range).

As a result, I've been exploring enabling this diagnostic by default in Clang
in both C++ and GNU++ modes. The in-progress patch discussion can be found at
https://reviews.llvm.org/D156565. However, we like to keep our diagnostic
behaviors in sync with GCC when possible, so I'm wondering if GCC would also
consider such a change.

[Bug c++/110848] Consider enabling -Wvla by default in C++ modes

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
           Keywords|                            |diagnostic

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Since VLA support has been a GNU C++ extension way before it was proposed to
WG21, I doubt we want to enable this by default.

[Bug c++/110848] Consider enabling -Wvla by default in C++ modes

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
GCC has documented VLA extensions for C++ support since
r0-35216-g4b404517536c85 (PR 930 which was done in 2001). So ....

[Bug c++/110848] Consider enabling -Wvla by default in C++ modes

[aaron at aaronballman dot com]

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

--- Comment #3 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Andrew Pinski from comment #1)
> Since VLA support has been a GNU C++ extension way before it was proposed to
> WG21, I doubt we want to enable this by default.

I think it boils down to whether you think users are using it on purpose or by
accident. My experience has been that more people use this by accident than not
in C++ and are unhappily surprised when they learn of it (sometimes by porting
to other compilers (like MSVC) that don't have the extension, sometimes through
other means like static analysis, etc). Given that there are security
implications with them, they're very easy to use accidentally, there are more
idiomatic approaches like std::vector, and that code generation can be quite a
bit slower for VLAs than other approaches, I think warning on them by default
is justifiable (the folks using them on purpose can add -Wno-vla to disable the
diagnostic, but I honestly expect them to be in the minority).

Do you have evidence there's a lot of intentional use of this feature in C++ in
the wild?

[Bug c++/110848] Consider enabling -Wvla by default in C++ modes

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Maybe my issue is this has been a documented extension for 20 years now.
-pedantic or -std=c++NN has always rejected it like it should. GCC has other
extensions which folks could use by accident too (like statement expressions).
Why is VLA special here?

[Bug c++/110848] Consider enabling -Wvla by default in C++ modes

[muecker at gwdg dot de]

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

Martin Uecker <muecker at gwdg dot de> changed:

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

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

I am not entirely convinced std::vector is actually superior in terms of
performance nor security. The code looks better to for VLAs even with bounds
checking. 

https://godbolt.org/z/xhvPePGbb

Although compiler support could be better and their usefulness in C++ without
other VM types is limited compared to C.

[Bug c++/110848] Consider enabling -Wvla by default in C++ modes

[aaron at aaronballman dot com]

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

--- Comment #6 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Andrew Pinski from comment #4)
> Maybe my issue is this has been a documented extension for 20 years now.

Which is totally fair -- we don't usually enable congratulatory diagnostics by
default.

> -pedantic or -std=c++NN has always rejected it like it should. GCC has other
> extensions which folks could use by accident too (like statement
> expressions). Why is VLA special here?

FWIW, I can't honestly say I've ever seen someone use a statement expression
accidentally, though I believe it's possible to do so if you work hard enough
at it. That said, I think misuse of accidental VLAs has more opportunity for
poor security behavior (specifically around attacker-controllable stack usage)
than for statement expressions. Given the security concerns coupled with the
ease of accidental usage, I think VLAs *are* different than statement
expressions. Some supporting evidence of the confusion in the wild:

https://stackoverflow.com/questions/70912167/how-do-i-tell-if-i-am-using-vla-variable-length-array
https://stackoverflow.com/questions/39334435/variable-length-array-vla-in-c-compilers
https://ddanilov.me/default-non-standard-features/
https://meta.stackoverflow.com/questions/376955/what-should-i-do-when-an-op-uses-variable-length-arrays-vlas-in-c
https://cplusplus.com/forum/beginner/284866/

(Granted, there's confusion about *everything* in C and C++.)

It's worth noting that -std=c++NN does *not* reject use of VLAs; you have to
pass -pedantic or -Wvla to get the diagnostic: https://godbolt.org/z/PGorTYG7r

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-07-31
            Summary|Consider enabling -Wvla by  |Consider enabling -Wvla by
                   |default in C++ modes        |default in non-GNU C++
                   |                            |modes

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think -std=c++XY should diagnose (at least with a warning) the use of GNU
extensions.  Let me alter the summary and confirm.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[aaron at aaronballman dot com]

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

--- Comment #8 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Richard Biener from comment #7)
> I think -std=c++XY should diagnose (at least with a warning) the use of GNU
> extensions.  Let me alter the summary and confirm.

Thanks! I still think this should be diagnosed in all language modes due to the
ease of accidental usage along with the feature's security concerns, but at
least getting it diagnosed by default in C++ language modes is a step in the
right direction. Some more evidence of the security concerns (VLAs in general,
not specific to C++):

https://nvd.nist.gov/vuln/detail/CVE-2015-5147
https://nvd.nist.gov/vuln/detail/CVE-2020-11203
https://nvd.nist.gov/vuln/detail/CVE-2021-3527

That said, it sounds like GCC maintainers feel (at least somewhat) strongly
that this extension should not be diagnosed by default in GNU mode. I think
Clang can follow suit so that there's less problems for folks porting between
the two compilers. But we've recently started being more aggressive about
diagnosing things that have security implications in C and C++ because of
warnings to not use these languages due to poor security practices and lack of
coverage with tooling:

https://advocacy.consumerreports.org/wp-content/uploads/2023/01/Memory-Safety-Convening-Report-1-1.pdf
https://media.defense.gov/2022/Nov/10/2003112742/-1/-1/0/CSI_SOFTWARE_MEMORY_SAFETY.PDF

I think VLA usage in C++ meets the bar as something to be more aggressive with
warning users about. It's not that the extension is broken, it's that it's very
often a surprise you're using the extension in the first place. It's
unfortunate to have to opt out of diagnostics about an extension you're
intentionally using; IMO, it's more unfortunate to have a CVE for your product
due to accidentally using an extension you weren't aware of.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Personally I'd like it diagnosed in GNU modes, but this is pretty much the
poster child for "conforming extension diagnosed with -pedantic" so I can see
the argument for not diagnosing it by default.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[aaron at aaronballman dot com]

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

--- Comment #10 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Jonathan Wakely from comment #9)
> Personally I'd like it diagnosed in GNU modes, but this is pretty much the
> poster child for "conforming extension diagnosed with -pedantic" so I can
> see the argument for not diagnosing it by default.

Agreed -- I also see the argument for not diagnosing it by default. Normally I
wouldn't suggest such a thing and I would not consider this to be a precedent
to be applied to other extensions if it does get warned on by default. (I think
the only other extension that might maaaayyyybbbbeeee be in the same category
would be nested functions in C, but I've not spent nearly enough time
researching that one to be sure.)

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[egallager at gcc dot gnu.org]

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

Eric Gallager <egallager at gcc dot gnu.org> changed:

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

--- Comment #11 from Eric Gallager <egallager at gcc dot gnu.org> ---
How about:

-std=c++XY: enabled by default (as per the proposal)
-std=gnu++XY: enabled by -Wall and/or -Wextra (in addition to being enabled by
-pedantic like it already is)

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[aaron at aaronballman dot com]

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

--- Comment #12 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Eric Gallager from comment #11)
> How about:
> 
> -std=c++XY: enabled by default (as per the proposal)
> -std=gnu++XY: enabled by -Wall and/or -Wextra (in addition to being enabled
> by -pedantic like it already is)

That's a good suggestion -- I'd be quite happy with adding it to -Wall (or
barring that, -Wextra) in GNU++ modes.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[aaron at aaronballman dot com]

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

--- Comment #13 from Aaron Ballman <aaron at aaronballman dot com> ---
Just to circle back around on this topic, these are changes recently landed in
Clang:
https://github.com/llvm/llvm-project/commit/84a3aadf0f2483dde0acfc4e79f2a075a5f35bd1

It enables the -Wvla-extension diagnostic group in C++ language modes by
default, adds the warning group to -Wall in GNU++ language modes, and the
warning is still opt-in in C language modes.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[egallager at gcc dot gnu.org]

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

--- Comment #14 from Eric Gallager <egallager at gcc dot gnu.org> ---
Oh right, if we're considering changing things for plain-C here, too, then
maybe have it enabled by -Wc++-compat there?

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[egallager at gcc dot gnu.org]

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

--- Comment #15 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Eric Gallager from comment #14)
> Oh right, if we're considering changing things for plain-C here, too, then
> maybe have it enabled by -Wc++-compat there?

Or rather, for plain-C modes, where "XY" is 99 or newer:

-std=cXY: enabled by -Wc++-compat
-std=gnuXY: still completely opt-in; -Wvla must be enabled explicitly

(and then for the pre-c99 modes, well, I guess they'd just keep their current
behavior)

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[muecker at gwdg dot de]

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

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

I do not think -Wall should warn about GNU extensions when used with
-std=gnu++XX in C++ and I think it is annoying that clang does it now. It only
drives people to use alloca or other alternatives with worse safety properties. 

And I think the security concerns for VLAs are largely based on a logical
fallacy: Because they appear in CVE is no reason to believe they caused it: It
is likely saying that people ICDs have more often cardiac arrests if because of
the ICDs.  Any kind of dynamically sized buffer will appear in CVEs because
buffers are used to process data from the network. If you discourage the one
with the best potential for  bounds checking people will turn to worse options.
This will not improve safety.

But stack clash protection should become the default.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[aaron at aaronballman dot com]

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

--- Comment #17 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Martin Uecker from comment #16)
> I do not think -Wall should warn about GNU extensions when used with
> -std=gnu++XX in C++ and I think it is annoying that clang does it now. It
> only drives people to use alloca or other alternatives with worse safety
> properties. 
> 
> And I think the security concerns for VLAs are largely based on a logical
> fallacy: Because they appear in CVE is no reason to believe they caused it:
> It is likely saying that people ICDs have more often cardiac arrests if
> because of the ICDs.  Any kind of dynamically sized buffer will appear in
> CVEs because buffers are used to process data from the network. If you
> discourage the one with the best potential for  bounds checking people will
> turn to worse options. This will not improve safety.
> 
> But stack clash protection should become the default.

In the time I opened this request, a new CVE related to VLAs came out:
https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2023-4039

Stack protection should become the default and it should certainly help
mitigate issues, but VLAs are still a valid security concern IMO. So yes, this
is intended to drive people to use alternatives (not necessarily `alloca`,
which would be a strange choice of replacement for VLAs in C++ in 2023).

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[pinskia at gcc dot gnu.org]

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

--- Comment #18 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Aaron Ballman from comment #17) 
> In the time I opened this request, a new CVE related to VLAs came out:
> https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2023-4039

Everything is a security risk. Seriously it is. Everything can and will be
abused; does not mean it is always right to warn about it.  Also
-fstack-protector should never be a CVE. CVEs will get to the point where they
will be ignored because how they are now pointing out non-security issues.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[aaron at aaronballman dot com]

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

--- Comment #19 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Andrew Pinski from comment #18)
> (In reply to Aaron Ballman from comment #17) 
> > In the time I opened this request, a new CVE related to VLAs came out:
> > https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2023-4039
> 
> Everything is a security risk. Seriously it is. Everything can and will be
> abused; does not mean it is always right to warn about it.  Also
> -fstack-protector should never be a CVE. CVEs will get to the point where
> they will be ignored because how they are now pointing out non-security
> issues.

My point is that this was a case where the developer used the language feature
and tried to do what they could to protect against security issues and still
ran into the security issue which resulted in a CVE. That's pretty different
from "everything can be abused". (I wasn't suggesting there's an issue with
using -fstack-protector or that it's a security issue itself)

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[muecker at gwdg dot de]

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

--- Comment #20 from Martin Uecker <muecker at gwdg dot de> ---
And what alternative do you think is fundamentally safer than VLAs?

VLAs know their bound. Thus, they integrate with _FORTIFY_SOURCE, and UBSan
bounds checking. Also UBSan address checking at run-time. At compile-time there
is -Wstringop-overflow -fanalyzer and -Walloc-larger-than. Then also stack
clash protection against stack overflow and stack protection against overflow
on the stack  (which is a second-line defense after bounds checking, which
failed here in a very specific case to protect alloca and VLAs. This is - of
course -  by itself not a vulnerability.) 

https://godbolt.org/z/PfcWP55or

alloca is clearly worse. Fixed size arrays on the stack which a worst-case
bounds are also worse in most cases.

std::vector<int> has some protection, e.g. ASAN finds the out of bounds access
(at a high run-time cost) and one could activate the GLIBC assertions someho:

https://godbolt.org/z/8zanMG5x4

But I would not call it safer and overhead is much worse.

Fundamentally, VLAs are the dynamic buffer which can be protected most easily
and should be *preferred*.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[redi at gcc dot gnu.org]

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

--- Comment #21 from Jonathan Wakely <redi at gcc dot gnu.org> ---
std::vector should be preferred in C++.

This warning will not drive responsible C++ developers to use alloca, it will
drive them to use std::vector. As Aaron has said, there are cases where people
use a VLA in C++ by mistake without even realising it, just because the code
compiles and they don't notice they're using a non-constant bound.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[muecker at gwdg dot de]

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

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

There may be many good reasons to prefer std::vector over VLAs in C++ but
security and safety is not one of them. There are plenty of CVEs caused by
std::vector out-of-bounds accesses. The question is whether in GNU mode one
should warn about a GNU extension. People who want to avoid VLAs for reasons of
standard compliance would also not use a GNU mode.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[redi at gcc dot gnu.org]

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

--- Comment #23 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Martin Uecker from comment #20)
> And what alternative do you think is fundamentally safer than VLAs?
> 
> VLAs know their bound. Thus, they integrate with _FORTIFY_SOURCE, and UBSan
> bounds checking. Also UBSan address checking at run-time. At compile-time
> there is -Ws

They don't integrate with idiomatic C++ such as ranges algorithms, and
std::end.

More generally, they simply don't integrate with the C++ type system, so are
unusable with most generic code using templates. Not only does std::is_array
not recognise them as arrays, but even attempting to ask the question with
std::is_array is ill-formed. Variably modified types are not part of the C++
type system, so can't be template arguments.

int n = 2;
int a[n]{};
static_assert(not std::is_array_v<decltype(a)>); // error

Clang doesn't even allow the {} initializer in the code above, so they're not
portable either, even among compilers that support -std=gnu++17 modes.


> std::vector<int> has some protection, e.g. ASAN finds the out of bounds
> access (at a high run-time cost) and one could activate the GLIBC assertions
> someho:
> 
> https://godbolt.org/z/8zanMG5x4

This will abort with the recommended hardening flags, specifically
-D_GLIBCXX_ASSERTIONS (which is nothing to do with Glibc, and is suitable for
production use, unlike ASan). Those assertions will be enabled by the proposed
-fhardening flag.


> 
> But I would not call it safer and overhead is much worse.
> 
> Fundamentally, VLAs are the dynamic buffer which can be protected most
> easily and should be *preferred*.

Maybe for C, but not for C++.

I know you are a big fan of VLAs, but please don't try to push them into a
language where they do not fit, and are not needed.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[redi at gcc dot gnu.org]

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

--- Comment #24 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Martin Uecker from comment #22)
> There may be many good reasons to prefer std::vector over VLAs in C++ but
> security and safety is not one of them. There are plenty of CVEs caused by
> std::vector out-of-bounds accesses.

There are plenty of CVEs caused by those for arrays too, static and variable
length ones.

The point is that vector carries its length with it properly, in a way that
actually works with the type system (e.g. works with std::span and std::end
etc.)

A VLA has a length that the compiler knows in a limited scope, but you can't
pass that to a function without passing the length explicitly as a separate
argument. The length information is easily lost.

> The question is whether in GNU mode one
> should warn about a GNU extension. People who want to avoid VLAs for reasons
> of standard compliance would also not use a GNU mode.

Yes, I know, and the lack of integration with the type system should show they
are simply inappropriate for general purpose use in idiomatic C++ code.

[Bug c++/110848] Consider enabling -Wvla by default in non-GNU C++ modes

[muecker at gwdg dot de]

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

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

I agree that they are not idiomatic C++ and that there exist good reasons why a
programmer may want to  avoid them (including standards compliance). But code
not being idiomatic is not a terrible good reason for having a warning. As a
matter of principle, we should not warn about our own extensions without a very
good reason with -std=gnu modes and neither should clang IMHO.

But the idea that VLAs are inherently very dangerous is incorrect, so let's not
perpetuate that myth.  There are many useful things a compiler could do to
improve security for VLAs and also for std::vector or elsewhere by having
better static analysis and more efficient options for bounds checking.  Neither
clang nor GCC will currently give any compile-time warning about a problem here
with -Wall -Wextra nor will there be a run-error with UBSan:

https://godbolt.org/z/7vhGMn3E5

And yes, -D_GLIBXX_DEBUG which will detect the out-of-bounds access but not the
memset. Maybe -D_FORTIFY_SOURCE=3 will do this (as it does for VLAs), but it
does not seem to work on godbolt for both cases, so I can't check.  Asan will
catch both.

For comparison, with VLAs we have this:

https://godbolt.org/z/hGxGrc569