[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776
+def warn_alloca : Warning<
+  "use of builtin function %0">,
+  InGroup<DiagGroup<"alloca">>, DefaultIgnore;
----------------
george.burgess.iv wrote:
> aaron.ballman wrote:
> > george.burgess.iv wrote:
> > > aaron.ballman wrote:
> > > > george.burgess.iv wrote:
> > > > > nit: I'd just say "use of function '%0'" here; "builtin" doesn't really add much.
> > > > > 
> > > > > I also wonder if we should be saying anything more than "we found a use of this function." Looks like GCC doesn't (https://godbolt.org/z/sYs_8G), but since this warning is sort of opinionated in itself, might it be better to expand this to "use of '%0' is discouraged"?
> > > > > 
> > > > > WDYT, Aaron?
> > > > What is the purpose to this diagnostic, aside from GCC compatibility? What does it protect against?
> > > > 
> > > > If there's a reason users should not use alloc(), it would be better for the diagnostic to spell it out.
> > > > 
> > > > Btw, I'm okay with this being default-off because the GCC warning is as well. I'm mostly hoping we can do better with our diagnostic wording.
> > > > I'm mostly hoping we can do better with our diagnostic wording
> > > 
> > > +1
> > > 
> > > > What is the purpose to this diagnostic?
> > > 
> > > I think the intent boils down to that `alloca` is easily misused, and leads to e.g., https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt . Since its use often boils down to nothing but a tiny micro-optimization, some parties would like to discourage its use.
> > > 
> > > Both glibc and bionic recommend against the use of `alloca` in their documentation, though glibc's docs are less assertive than bionic's, and explicitly call out "[alloca's] use can improve efficiency compared to the use of malloc plus free."
> > > 
> > > Greping a codebase and investigating the first 15 results:
> > > - all of them look like microoptimizations; many of them also sit close to other `malloc`/`new` ops, so allocating on these paths presumably isn't prohibitively expensive
> > > - all but two of the uses of `alloca` have no logic to fall back to the heap `malloc` if the array they want to allocate passes a certain threshold. Some of the uses make it look *really* easy for the array to grow very large.
> > > - one of the uses compares the result of `alloca` to `NULL`. Since `alloca`'s behavior is undefined if it fails, ...
> > > 
> > > I'm having trouble putting this into a concise and actionable diagnostic message, though. The best I can come up with at the moment is something along the lines of "use of function %0 is subtle; consider using heap allocation instead."
> > Okay, that's along the lines of what I was thinking.
> > 
> > Part of me thinks that this should not diagnose calls to `alloca()` that are given a constant value that we can test for sanity at compile time. e.g., calling `alloca(10)` is highly unlikely to be a problem, but calling `alloca(1000000)` certainly could be, while `alloca(x)` is a different class of problem without good static analysis.
> > 
> > That said, perhaps we could get away with `use of function %0 is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability` or something along those lines?
> Yeah, GCC has a similar `-Walloca-larger-than=N` that does roughly what you described. The icky part is exactly what you said. GCC will warn about unknown values, but considers control flow when doing so: https://godbolt.org/z/J3pGiT
> 
> It looks like it's the same "we apply optimizations and then see what happens" behavior as similar diagnostics: https://godbolt.org/z/lLyteP
> 
> WRT the diag we emit here, maybe we could use notes to break it up and imply things? e.g.
> 
> warning: use of function %0 is discouraged, due to its security implications
> note: 'malloc' or 'new' are suggested alternatives, since they have well-defined behavior on failure
> 
> ...not sold on the idea, but it's a thought.
> 
> If we don't want to break it to pieces, I'm fine with your suggestion
I'm not certain the note adds value because it will always be printed on the same line as the warning. A note would make sense if we had a secondary location to annotate though.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64883/new/

https://reviews.llvm.org/D64883