[cfe-dev] [RFC] Control -Wvla for conformant parameters

[Arthur O'Dwyer via cfe-dev]

On Mon, Nov 8, 2021 at 7:34 AM Aaron Ballman via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> On Sun, Nov 7, 2021 at 10:27 AM Hu Jialun via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
> >
> > It appears that -Wvla tends to be used for two different purposes,
> > - Code compatibility with pre- and post-C99 code
> > - Good practices since VLA can have security implications
> >
> > -Wvla seems to only serve the former one satisfactorily now. For the
> latter
> > purpose, conformant array parameters would still generate VLA diagnosis
> > although they degrade into pointers and have no runtime implications.
> There is
> > currently no way to inhibit such false positives.
> >
> >         $ clang -Wvla -x c -std=c99 - <<< "void f(unsigned n, int a[n]);"
> >         <stdin>:1:26: warning: variable length array used [-Wvla]
> >         void f(unsigned n, int a[n]);
> >                                 ^
>

FWIW, I personally *do* consider this code to have a "security
implication," in the moral-hazard sense. People frequently pass "array
parameters" thinking that that's what's happening, when in fact they're
silently decaying to pointers. *Particularly* with VLAs, but also with
non-VLA arrays, this can be a big deal.
https://quuxplusone.github.io/blog/2020/03/12/contra-array-parameters/

    void helper(char mcs_mask[IEEE80211_HT_MCS_MASK_LEN]) {
        for (int i=0; i < sizeof(mcs_mask); ++i) { // totally wrong
            mcs_mask[i] &= SOME_MASK;
        }
    }

So I disagree with your "only serve the former [code compatibility bullet
point]"; I think the current situation is fine and dandy.
But I see your point about there being two technically-different features
at the machine level, and maybe it would be nice to put them under
different warnings. A backward-compatible way to do that would be
- Change -Wvla from a single warning to a warning group
- Add -Wvla-parameter as a new warning; include it in the -Wvla warning
group

Alternatively, and closer to your original suggestion:
- Add -Warray-parameter as a new warning, for things like
    void f(int n, int a[n]);
    void f(int n, int a[2]);
  which are equally error-prone. In fact, the Linux kernel issue mentioned
above uses the non-VLA syntax, and currently gets no warning from Clang at
all, even in -Weverything mode.
- People who enable -Wvla should probably also enable -Warray-parameter.
Clang might enable it under -Wextra (with a special carveout for `main(int,
char *[])`), or just leave it off-by-default.

I think this makes a fair amount of sense. In the standard, VLAs are
> split into two concepts: a variably-modified type and a variable
> length array.


(I'm coming from the C++ side with no particular C-standardese expertise.)
FWIW, I don't see a benefit to introducing the term "variably modified
type" all the way up in the command-line warning-option interface. I think
it may make sense to mention the term in the wording of the diagnostic
message, just not in its name.
IIUC, Clang currently misdiagnoses
https://godbolt.org/z/6x7399eqP
    return sizeof(int (*)(int[n]));  // -Wvla warns here (wrongly)
because `int (*)(int[n])` is *not* a variably modified type — is that
right? So it would be cool to clean up some of this code at the same time.

–Arthur
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211108/4e68eaf7/attachment.html>