[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:
> > > 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?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1175
+    Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+        << Context.BuiltinInfo.getName(BuiltinID);
     break;
----------------
I think this should see how the user spelled the builtin call. It would be a bit strange for a user who wrote `alloca()` in their code to get a diagnostic about not calling `__builtin_alloca()`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64883