<div dir="ltr"><div dir="ltr">On Mon, Nov 8, 2021 at 7:34 AM Aaron Ballman via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sun, Nov 7, 2021 at 10:27 AM Hu Jialun via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br>
><br>> It appears that -Wvla tends to be used for two different purposes,<br>> - Code compatibility with pre- and post-C99 code<br>
> - Good practices since VLA can have security implications<br>
><br>
> -Wvla seems to only serve the former one satisfactorily now. For the latter<br>
> purpose, conformant array parameters would still generate VLA diagnosis<br>
> although they degrade into pointers and have no runtime implications. There is<br>
> currently no way to inhibit such false positives.<br>
><br>
> $ clang -Wvla -x c -std=c99 - <<< "void f(unsigned n, int a[n]);"<br>
> <stdin>:1:26: warning: variable length array used [-Wvla]<br>
> void f(unsigned n, int a[n]);<br>
> ^<br></blockquote><div><br></div><div>FWIW, I personally <i>do</i> 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. <i>Particularly</i> with VLAs, but also with non-VLA arrays, this can be a big deal.</div><div><a href="https://quuxplusone.github.io/blog/2020/03/12/contra-array-parameters/">https://quuxplusone.github.io/blog/2020/03/12/contra-array-parameters/</a><br></div><div class="gmail_quote"><br></div> void helper(char mcs_mask[IEEE80211_HT_MCS_MASK_LEN]) {</div><div class="gmail_quote"> for (int i=0; i < sizeof(mcs_mask); ++i) { // totally wrong</div><div class="gmail_quote"> mcs_mask[i] &= SOME_MASK;</div><div class="gmail_quote"> }</div><div class="gmail_quote"> }<div> </div><div>So I disagree with your "only serve the former [code compatibility bullet point]"; I think the current situation is fine and dandy.</div><div>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</div><div>- Change -Wvla from a single warning to a warning group</div><div>- Add -Wvla-parameter as a new warning; include it in the -Wvla warning group</div><div><br></div><div>Alternatively, and closer to your original suggestion:</div><div>- Add -Warray-parameter as a new warning, for things like</div><div> void f(int n, int a[n]);</div><div> void f(int n, int a[2]);</div><div> 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.<br></div><div>- 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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I think this makes a fair amount of sense. In the standard, VLAs are<br>
split into two concepts: a variably-modified type and a variable<br>
length array.</blockquote><div><br></div><div>(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.</div><div>IIUC, Clang currently misdiagnoses</div><div><a href="https://godbolt.org/z/6x7399eqP">https://godbolt.org/z/6x7399eqP</a><br></div><div> return sizeof(int (*)(int[n])); // -Wvla warns here (wrongly)</div><div>because `int (*)(int[n])` is <i>not</i> a variably modified type — is that right? So it would be cool to clean up some of this code at the same time.</div><div><br></div><div>–Arthur</div></div></div>