[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
This revision was automatically updated to reflect the committed changes.
Closed by commit rL367067: [Sema] add -Walloca to flag uses of `alloca`
(authored by gbiv, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D64883?vs=211816=211837#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64883/new/
https://reviews.llvm.org/D64883
Files:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/warn-alloca.c
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2779,6 +2779,11 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "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">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
Index: cfe/trunk/test/Sema/warn-alloca.c
===
--- cfe/trunk/test/Sema/warn-alloca.c
+++ cfe/trunk/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+ // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+ __builtin_alloca(a);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca' is discouraged;
there is no way to check for failure but failure may still occur, resulting in
a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca_with_align' is
discouraged; there is no way to check for failure but failure may still occur,
resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -1179,6 +1179,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< TheCall->getDirectCallee();
break;
case Builtin::BI__assume:
case Builtin::BI__builtin_assume:
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2779,6 +2779,11 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "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">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
Index: cfe/trunk/test/Sema/warn-alloca.c
===
--- cfe/trunk/test/Sema/warn-alloca.c
+++ cfe/trunk/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+ // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+ __builtin_alloca(a);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca_with_align' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -1179,6 +1179,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for the patch! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
ziyig updated this revision to Diff 211816.
ziyig marked an inline comment as done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64883/new/
https://reviews.llvm.org/D64883
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/warn-alloca.c
Index: clang/test/Sema/warn-alloca.c
===
--- /dev/null
+++ clang/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+ // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+ __builtin_alloca(a);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca' is discouraged;
there is no way to check for failure but failure may still occur, resulting in
a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca_with_align' is
discouraged; there is no way to check for failure but failure may still occur,
resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1169,6 +1169,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< TheCall->getDirectCallee();
break;
case Builtin::BI__assume:
case Builtin::BI__builtin_assume:
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2772,6 +2772,11 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "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">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
Index: clang/test/Sema/warn-alloca.c
===
--- /dev/null
+++ clang/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+ // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+ __builtin_alloca(a);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function '__builtin_alloca_with_align' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1169,6 +1169,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< TheCall->getDirectCallee();
break;
case Builtin::BI__assume:
case Builtin::BI__builtin_assume:
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2772,6 +2772,11 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "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">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1172 return ExprError(); +LLVM_FALLTHROUGH; + case Builtin::BI__builtin_alloca: ziyig wrote: > aaron.ballman wrote: > > Do we want to warn on all uses of alloca(), or just the ones that get past > > the call to `SemaBuiltinAllocaWithAlign()`? > I don't have strong opinion about this. Which one do you think is better? I think the code is fine as-is. `SemaBuiltinAllocaWithAlign()` returns true when there's an error with the call, and so users will not get the warning only if the call is erroneous, which seems fine given that the code didn't compile. It turns out this matches GCC's behavior as well. Comment at: clang/lib/Sema/SemaChecking.cpp:1175 +Diag(TheCall->getBeginLoc(), diag::warn_alloca) +<< TheCall->getDirectCallee()->getNameInfo().getAsString(); break; You should pass in `TheCall->getDirectCallee()` and not try to get the name directly; the diagnostics engine will do the right thing automatically. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
ziyig added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1172 return ExprError(); +LLVM_FALLTHROUGH; + case Builtin::BI__builtin_alloca: aaron.ballman wrote: > Do we want to warn on all uses of alloca(), or just the ones that get past > the call to `SemaBuiltinAllocaWithAlign()`? I don't have strong opinion about this. Which one do you think is better? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
ziyig updated this revision to Diff 211785.
ziyig marked 9 inline comments as done.
ziyig added a comment.
Updated the warning message and the test cases.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64883/new/
https://reviews.llvm.org/D64883
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/warn-alloca.c
Index: clang/test/Sema/warn-alloca.c
===
--- /dev/null
+++ clang/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+ // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+ __builtin_alloca(a);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function __builtin_alloca is discouraged;
there is no way to check for failure but failure may still occur, resulting in
a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function __builtin_alloca_with_align is
discouraged; there is no way to check for failure but failure may still occur,
resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1169,6 +1169,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< TheCall->getDirectCallee()->getNameInfo().getAsString();
break;
case Builtin::BI__assume:
case Builtin::BI__builtin_assume:
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2772,6 +2772,11 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "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">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
Index: clang/test/Sema/warn-alloca.c
===
--- /dev/null
+++ clang/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+ // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+ __builtin_alloca(a);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function __builtin_alloca is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+ // expected-warning@-2 {{use of function __builtin_alloca_with_align is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1169,6 +1169,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< TheCall->getDirectCallee()->getNameInfo().getAsString();
break;
case Builtin::BI__assume:
case Builtin::BI__builtin_assume:
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2772,6 +2772,11 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "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">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
george.burgess.iv added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; aaron.ballman wrote: > 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(100)` 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. SGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, 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(100)` 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
george.burgess.iv added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; 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(100)` 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 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, 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(100)` 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
george.burgess.iv added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; 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." Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; 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. Comment at: clang/lib/Sema/SemaChecking.cpp:1172 return ExprError(); +LLVM_FALLTHROUGH; + case Builtin::BI__builtin_alloca: Do we want to warn on all uses of alloca(), or just the ones that get past the call to `SemaBuiltinAllocaWithAlign()`? Comment at: clang/test/Sema/warn-alloca.c:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s + I'd appreciate a test demonstrating no warnings if `-Wall` is passed without an explicit `-Walloca`. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
george.burgess.iv added a comment. Thanks for this! Mostly just nitpicking the warning's wording. :) Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; 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? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function
ziyig created this revision.
ziyig added reviewers: gbiv, aaron.ballman.
Herald added a reviewer: george.burgess.iv.
Herald added subscribers: cfe-commits, kristina.
Herald added a project: clang.
Add new warning -Walloca for use of builtin alloca function.
Also warns the use of __builtin_alloca_with_align. GCC has this warning, and
we'd like to have this for compatibility.
Repository:
rC Clang
https://reviews.llvm.org/D64883
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/warn-alloca.c
Index: clang/test/Sema/warn-alloca.c
===
--- /dev/null
+++ clang/test/Sema/warn-alloca.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+void test1(int a) {
+ __builtin_alloca(a); // expected-warning {{use of builtin function
__builtin_alloca}}
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32); // expected-warning {{use of builtin
function __builtin_alloca_with_align}}
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1169,6 +1169,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< Context.BuiltinInfo.getName(BuiltinID);
break;
case Builtin::BI__assume:
case Builtin::BI__builtin_assume:
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2772,6 +2772,10 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "use of builtin function %0">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
Index: clang/test/Sema/warn-alloca.c
===
--- /dev/null
+++ clang/test/Sema/warn-alloca.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+void test1(int a) {
+ __builtin_alloca(a); // expected-warning {{use of builtin function __builtin_alloca}}
+}
+
+void test2(int a) {
+ __builtin_alloca_with_align(a, 32); // expected-warning {{use of builtin function __builtin_alloca_with_align}}
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1169,6 +1169,10 @@
case Builtin::BI__builtin_alloca_with_align:
if (SemaBuiltinAllocaWithAlign(TheCall))
return ExprError();
+LLVM_FALLTHROUGH;
+ case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< Context.BuiltinInfo.getName(BuiltinID);
break;
case Builtin::BI__assume:
case Builtin::BI__builtin_assume:
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2772,6 +2772,10 @@
def err_cannot_find_suitable_accessor : Error<
"cannot find suitable %select{getter|setter}0 for property %1">;
+def warn_alloca : Warning<
+ "use of builtin function %0">,
+ InGroup>, DefaultIgnore;
+
def warn_alloca_align_alignof : Warning<
"second argument to __builtin_alloca_with_align is supposed to be in bits">,
InGroup>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
