[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D58797#1447148 , @phosek wrote:

> This is triggering a Clang assertion failure in Fuchsia build:
>
>   clang/lib/AST/ExprConstant.cpp:5032: bool (anonymous 
> namespace)::ExprEvaluatorBase<(anonymous 
> namespace)::PointerExprEvaluator>::VisitMemberExpr(const clang::MemberExpr *) 
> [Derived = (anonymous namespace)::PointerExprEvaluator]: Assertion 
> `!E->isArrow() && "missing call to bound member function?"' failed.
>
>
> I've filed PR41286 which also has a reproducer.


Fixed in r357304, thanks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Ah, looks like the problem is we're sending a dependent expression to the 
constant evaluator, we should just bail out in that case. I'll fix this 
tomorrow morning, thanks for tracking this down!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

This is triggering a Clang assertion failure in Fuchsia build:

  clang/lib/AST/ExprConstant.cpp:5032: bool (anonymous 
namespace)::ExprEvaluatorBase<(anonymous 
namespace)::PointerExprEvaluator>::VisitMemberExpr(const clang::MemberExpr *) 
[Derived = (anonymous namespace)::PointerExprEvaluator]: Assertion 
`!E->isArrow() && "missing call to bound member function?"' failed.

I've filed PR41286 which also has a reproducer.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D58797#1443856 , @erik.pilkington 
wrote:

> I added it in r357041.


LGTM, thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D58797#1443411 , @efriedma wrote:

> For that particular case, I think we could suppress the false positive using 
> DiagRuntimeBehavior.  How many total false positives are we talking about, 
> and how many can the compiler statically prove are unreachable?


Sure, that seems like a perfect fit, I added it in r357041. There are only two 
false-positives IIUC, and they're both immediately guarded by an `if (sizeof(x) 
== sizeof(y))`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

(Forgot to refresh before pressing 'Submit', so maybe efriedma's comment 
answers all of the questions in mine ;) )


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

We have warnings like -Wdivision-by-zero that take reachability into account: 
https://godbolt.org/z/3PD-eF. I don't immediately know how it's all done (e.g. 
in batch because CFG construction is expensive? ...), but looking there for 
inspiration may be useful.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

For that particular case, I think we could suppress the false positive using 
DiagRuntimeBehavior.  How many total false positives are we talking about, and 
how many can the compiler statically prove are unreachable?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D58797#1439888 , @erik.pilkington 
wrote:

> Ah, I didn't consider that case. Presumably `st` is configured to have 
> different sizes based on the target? I agree that this is a false-positive, 
> but it seems like a pretty narrow edge case, and there is a pretty obvious 
> source workaround that doesn't affect readability: `memcpy(&buf, st, 
> sizeof(buf))`. @aaron.ballman/@rsmith Any thoughts here? IMO keeping this 
> diagnostic is worth it.


Yes, I think we should keep this diagnostic. However, if we can find a way to 
silence it for this particular false-positive pattern, that would be great!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D58797#1439888 , @erik.pilkington 
wrote:

> Ah, I didn't consider that case. Presumably `st` is configured to have 
> different sizes based on the target?


Yes; sorry I was not clear about that in my example.

> I agree that this is a false-positive, but it seems like a pretty narrow edge 
> case, and there is a pretty obvious source workaround that doesn't affect 
> readability: `memcpy(&buf, st, sizeof(buf))`.

Oh, yeah, we could make that source level change.

> @aaron.ballman/@rsmith Any thoughts here? IMO keeping this diagnostic is 
> worth it.

I'm all for keeping it, I'm curious if it can be extended to NOT warn in the 
case provided?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington marked 4 inline comments as done.
erik.pilkington added a comment.

In D58797#1438975 , @nickdesaulniers 
wrote:

> This is causing false positive warnings for the Linux kernel:
>  https://github.com/ClangBuiltLinux/linux/issues/423
>  :(
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/statfs.c#n128
>  Consider this untested case (when the condition is false):
>
>   if (sizeof(buf) == sizeof(*st))
>   memcpy(&buf, st, sizeof(*st));
>
>
> fs/statfs.c:129:3: warning: 'memcpy' will always overflow; destination buffer 
> has size 64, but size argument is 88 [-Wfortify-source]
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/statfs.c#n169,
>  too.


Ah, I didn't consider that case. Presumably `st` is configured to have 
different sizes based on the target? I agree that this is a false-positive, but 
it seems like a pretty narrow edge case, and there is a pretty obvious source 
workaround that doesn't affect readability: `memcpy(&buf, st, sizeof(buf))`. 
@aaron.ballman/@rsmith Any thoughts here? IMO keeping this diagnostic is worth 
it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

This is causing false positive warnings for the Linux kernel:
https://github.com/ClangBuiltLinux/linux/issues/423
:(

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/statfs.c#n128
Consider this untested case:

if (sizeof(buf) == sizeof(*st))
memcpy(&buf, st, sizeof(*st));

fs/statfs.c:129:3: warning: 'memcpy' will always overflow; destination buffer 
has size 64, but size argument is 88 [-Wfortify-source]

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/statfs.c#n169,
 too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356397: [Sema] Add some compile time _FORTIFY_SOURCE 
diagnostics (authored by epilk, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58797?vs=190549&id=191142#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D58797

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  test/Analysis/bstring.c
  test/Analysis/null-deref-ps-region.c
  test/Analysis/pr22954.c
  test/Analysis/string.c
  test/Sema/builtin-object-size.c
  test/Sema/builtins.c
  test/Sema/transpose-memset.c
  test/Sema/warn-fortify-source.c
  test/Sema/warn-strncat-size.c

Index: include/clang/AST/Decl.h
===
--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -2255,7 +2255,7 @@
 return const_cast(this)->getCanonicalDecl();
   }
 
-  unsigned getBuiltinID() const;
+  unsigned getBuiltinID(bool ConsiderWrapperFunctions = false) const;
 
   // ArrayRef interface to parameters.
   ArrayRef parameters() const {
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -10677,6 +10677,7 @@
 
   ExprResult CheckBuiltinFunctionCall(FunctionDecl *FDecl,
   unsigned BuiltinID, CallExpr *TheCall);
+  void checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, CallExpr *TheCall);
 
   bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall,
 unsigned MaxWidth);
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -1054,3 +1054,5 @@
 def CrossTU : DiagGroup<"ctu">;
 
 def CTADMaybeUnsupported : DiagGroup<"ctad-maybe-unsupported">;
+
+def FortifySource : DiagGroup<"fortify-source">;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -672,11 +672,17 @@
   "the argument to %0 has side effects that will be discarded">,
   InGroup>;
 
-def warn_memcpy_chk_overflow : Warning<
+def warn_builtin_chk_overflow : Warning<
   "'%0' will always overflow; destination buffer has size %1,"
   " but size argument is %2">,
   InGroup>;
 
+def warn_fortify_source_overflow
+  : Warning, InGroup;
+def warn_fortify_source_size_mismatch : Warning<
+  "'%0' size argument is too large; destination buffer has size %1,"
+  " but size argument is %2">, InGroup;
+
 /// main()
 // static main() is not an error in C, just in C++.
 def warn_static_main : Warning<"'main' should not be declared static">,
Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -230,14 +230,14 @@
 // expected-note {{change size argument to be the size of the destination}}
 __builtin___strlcpy_chk(buf, b, sizeof(b), __builtin_object_size(buf, 0)); // expected-warning {{size argument in '__builtin___strlcpy_chk' call appears to be size of the source; expected the size of the destination}} \
 // expected-note {{change size argument to be the size of the destination}} \
-// expected-warning {{'__builtin___strlcpy_chk' will always overflow; destination buffer has size 20, but size argument is 40}}
+// expected-warning {{'strlcpy' will always overflow; destination buffer has size 20, but size argument is 40}}
 
 strlcat(buf, b, sizeof(b)); // expected-warning {{size argument in 'strlcat' call appears to be size of the source; expected the size of the destination}} \
 // expected-note {{change size argument to be the size of the destination}}
 
 __builtin___strlcat_chk(buf, b, sizeof(b), __builtin_object_size(buf, 0)); // expected-warning {{size argument in '__builtin___strlcat_chk' call appears to be size of the source; expected the size of the destination}} \
// expected-note {{change size argument to be the size of the destination}} \
-   // expected-warning {{'__builtin___strlcat_chk' will always overflow; destination buffer has size 20, but size argument is 40}}
+   // expected-warning {{'strlcat' will always overflow; destination buffer has size 20, but size a

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some minor nits.




Comment at: clang/lib/Sema/SemaChecking.cpp:338
+  case Builtin::BI__builtin___vsnprintf_chk: {
+DiagID = diag::warn_memcpy_chk_overflow;
+IsChkVariant = true;

I feel like this diagnostic name should be updated -- `snprintf()` and 
`memcpy()` are pretty distinct things. Maybe `warn_builtin_chk_overflow`?



Comment at: clang/lib/Sema/SemaChecking.cpp:400
+int BOSType = 0;
+if (auto *POS =
+FD->getParamDecl(ObjectIndex)->getAttr())

`const auto *`


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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

This LGTM; feel free to submit after Aaron stamps this.

Thanks again!




Comment at: clang/lib/Sema/SemaExpr.cpp:5929
 
+checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+

erik.pilkington wrote:
> george.burgess.iv wrote:
> > Why isn't this in CheckBuiltinFunctionCall?
> For the same reason I added the `bool` parameter to `getBuiltinCallee`, we 
> don't usually consider calls to `__attribute__((overloadable))` be builtins, 
> so we never reach `CheckBuiltinFunctionCall` for them. We're planning on 
> using that attribute though:
> 
> ```
> int sprintf(__attribute__((pass_object_size(_FORTIFY_LEVEL))) char *buffer, 
> const char *format, ...) 
>   __attribute__((overloadable)) 
> __asm__("___sprintf_pass_object_size_chk");
> ```
SGTM


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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 190549.
erik.pilkington marked 16 inline comments as done.
erik.pilkington added a comment.

Address review comments. Thanks!


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

https://reviews.llvm.org/D58797

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/bstring.c
  clang/test/Analysis/null-deref-ps-region.c
  clang/test/Analysis/pr22954.c
  clang/test/Analysis/string.c
  clang/test/Sema/builtin-object-size.c
  clang/test/Sema/builtins.c
  clang/test/Sema/transpose-memset.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/Sema/warn-strncat-size.c

Index: clang/test/Sema/warn-strncat-size.c
===
--- clang/test/Sema/warn-strncat-size.c
+++ clang/test/Sema/warn-strncat-size.c
@@ -39,7 +39,7 @@
   strncat(dest, "AAA", sizeof(dest) - strlen(dest)); // expected-warning{{the value of the size argument in 'strncat' is too large, might lead to a buffer overflow}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}}
 
   strncat((*s5)->f2[x], s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}}
-  strncat(s1+3, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}}
+  strncat(s1+3, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-warning {{strncat' size argument is too large; destination buffer has size 97, but size argument is 200}}
   strncat(s4.f1, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}}
 }
 
Index: clang/test/Sema/warn-fortify-source.c
===
--- /dev/null
+++ clang/test/Sema/warn-fortify-source.c
@@ -0,0 +1,83 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
+
+typedef unsigned long size_t;
+
+#if defined(USE_PASS_OBJECT_SIZE)
+void *memcpy(void *dst, const void *src, size_t c);
+static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
+static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
+  return 0;
+}
+#elif defined(USE_BUILTINS)
+#define memcpy(x,y,z) __builtin_memcpy(x,y,z)
+#else
+void *memcpy(void *dst, const void *src, size_t c);
+#endif
+
+void call_memcpy() {
+  char dst[10];
+  char src[20];
+  memcpy(dst, src, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 10, but size argument is 20}}
+}
+
+void call_memcpy_type() {
+  struct pair {
+int first;
+int second;
+  };
+  struct pair p;
+  char buf[20];
+  memcpy(&p.first, buf, 20);
+#ifdef USE_PASS_OBJECT_SIZE
+  // Use the more strict checking mode on the pass_object_size attribute:
+  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
+#else
+  // Or just fallback to type 0:
+  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
+#endif
+}
+
+void call_strncat() {
+  char s1[10], s2[20];
+  __builtin_strncat(s2, s1, 20);
+  __builtin_strncat(s1, s2, 20); // expected-warning {{'strncat' size argument is too large; destination buffer has size 10, but size argument is 20}}
+}
+
+void call_strncpy() {
+  char s1[10], s2[20];
+  __builtin_strncpy(s2, s1, 20);
+  __builtin_strncpy(s1, s2, 20); // expected-warning {{'strncpy' size argument is too large; destination buffer has size 10, but size argument is 20}}
+}
+
+void call_stpncpy() {
+  char s1[10], s2[20];
+  __builtin_stpncpy(s2, s1, 20);
+  __builtin_stpncpy(s1, s2, 20); // expected-warning {{'stpncpy' size argument is too large; destination buffer has size 10, but size argument is 20}}
+}
+
+void call_memmove() {
+  char s1[10], s2[20];
+  __builtin_memmove(s2, s1, 20);
+  __builtin_memmove(s1, s2, 20); // expected-warning {{'memmove' will always overflow; destination buffer has size 10, but size argument is 20}}
+}
+
+void call_memset() {
+  char buf[10];
+  __builtin_memset(buf, 0xff, 10);
+  __builtin_memset(buf,

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:319
+// If the parameter has a pass_object_size attribute, then we should use
+// it's (potentially) more strict checking mode. Otherwise, conservatively
+// assume type 0.

aaron.ballman wrote:
> it's -> its
The opposite mistake as above, lol.



Comment at: clang/lib/Sema/SemaChecking.cpp:321
+// assume type 0.
+int BOSType = 0;
+if (auto *POS = FD->getParamDecl(ObjIdx)->getAttr())

george.burgess.iv wrote:
> Should this also try to consider `fortify_stdlib`?
I reverted `fortify_stdlib` in r356103 (although you couldn't possible know 
this, since that was after you made this comment ;)). We're going to use your 
`pass_object_size` attribute instead.



Comment at: clang/lib/Sema/SemaChecking.cpp:367
+DiagID = diag::warn_memcpy_chk_overflow;
+if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+!evaluateSizeArg(TheCall->getNumArgs()-2))

aaron.ballman wrote:
> george.burgess.iv wrote:
> > nit: All of these cases (and the two lambdas above) look super similar. 
> > Might it be clearer to just set `SizeIndex` and `ObjectIndex` variables 
> > from here, and actually `evaluate` them before `UsedSize.ule(ComputedSize)`?
> > 
> > If not, I'm OK with this as-is.
> Formatting looks off here -- run the patch through clang-format?
Sure, I guess it's a bit cleaner to do that.



Comment at: clang/lib/Sema/SemaChecking.cpp:423
+  StringRef FunctionName = getASTContext().BuiltinInfo.getName(BuiltinID);
+  if (DiagID == diag::warn_memcpy_chk_overflow) {
+// __builtin___memcpy_chk -> memcpy

aaron.ballman wrote:
> Why don't we want to do this for the new fortify diagnostics?
No reason, just was trying to avoid changing the output of the `_chk` 
diagnostic. We might as as well though, it cleans up the diagnostic.



Comment at: clang/lib/Sema/SemaExpr.cpp:5929
 
+checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+

george.burgess.iv wrote:
> Why isn't this in CheckBuiltinFunctionCall?
For the same reason I added the `bool` parameter to `getBuiltinCallee`, we 
don't usually consider calls to `__attribute__((overloadable))` be builtins, so 
we never reach `CheckBuiltinFunctionCall` for them. We're planning on using 
that attribute though:

```
int sprintf(__attribute__((pass_object_size(_FORTIFY_LEVEL))) char *buffer, 
const char *format, ...) 
  __attribute__((overloadable)) 
__asm__("___sprintf_pass_object_size_chk");
```


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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/Decl.cpp:2994
+///
+/// \param ConsiderWrapperFunctions If we should consider wrapper functions as
+/// their wrapped builtins. This shouldn't be done in general, but its useful 
in

If we should -> If true, we should



Comment at: clang/lib/AST/Decl.cpp:2995
+/// \param ConsiderWrapperFunctions If we should consider wrapper functions as
+/// their wrapped builtins. This shouldn't be done in general, but its useful 
in
+/// Sema to diagnose calls to wrappers based on their semantics.

its -> it's



Comment at: clang/lib/Sema/SemaChecking.cpp:319
+// If the parameter has a pass_object_size attribute, then we should use
+// it's (potentially) more strict checking mode. Otherwise, conservatively
+// assume type 0.

it's -> its



Comment at: clang/lib/Sema/SemaChecking.cpp:367
+DiagID = diag::warn_memcpy_chk_overflow;
+if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+!evaluateSizeArg(TheCall->getNumArgs()-2))

george.burgess.iv wrote:
> nit: All of these cases (and the two lambdas above) look super similar. Might 
> it be clearer to just set `SizeIndex` and `ObjectIndex` variables from here, 
> and actually `evaluate` them before `UsedSize.ule(ComputedSize)`?
> 
> If not, I'm OK with this as-is.
Formatting looks off here -- run the patch through clang-format?



Comment at: clang/lib/Sema/SemaChecking.cpp:388
+  return;
+// Whether these functions overflow depend on the runtime strlen of the
+// string, not just the buffer size, so emitting the "always overflow"

depend -> depends



Comment at: clang/lib/Sema/SemaChecking.cpp:423
+  StringRef FunctionName = getASTContext().BuiltinInfo.getName(BuiltinID);
+  if (DiagID == diag::warn_memcpy_chk_overflow) {
+// __builtin___memcpy_chk -> memcpy

Why don't we want to do this for the new fortify diagnostics?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Looks solid to me overall; just a few nits.

I don't think I have actual ownership over any of this code, so I'll defer to 
either Aaron or Richard for the final LGTM

Thanks again!




Comment at: clang/lib/Sema/SemaChecking.cpp:317
+  // buffer size would be.
+  auto computeObjectSize = [&](unsigned ObjIdx) {
+// If the parameter has a pass_object_size attribute, then we should use

nit: I think the prevailing preference is to name lambdas as you'd name 
variables, rather than as you'd name methods/functions



Comment at: clang/lib/Sema/SemaChecking.cpp:321
+// assume type 0.
+int BOSType = 0;
+if (auto *POS = FD->getParamDecl(ObjIdx)->getAttr())

Should this also try to consider `fortify_stdlib`?



Comment at: clang/lib/Sema/SemaChecking.cpp:367
+DiagID = diag::warn_memcpy_chk_overflow;
+if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+!evaluateSizeArg(TheCall->getNumArgs()-2))

nit: All of these cases (and the two lambdas above) look super similar. Might 
it be clearer to just set `SizeIndex` and `ObjectIndex` variables from here, 
and actually `evaluate` them before `UsedSize.ule(ComputedSize)`?

If not, I'm OK with this as-is.



Comment at: clang/lib/Sema/SemaExpr.cpp:5929
 
+checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+

Why isn't this in CheckBuiltinFunctionCall?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for working on this!

I hope to take an in-depth look at this patch next week (if someone else 
doesn't beat me to it...), but wanted to note that I think enabling clang to 
emit these warnings on its own is a good thing. `diagnose_if` is great for 
potentially more targeted/implementation-defined things that standard libraries 
want to diagnose, but IMO clang should be able to catch trivially broken code 
like this regardless of the stdlib it's building against.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington created this revision.
erik.pilkington added reviewers: george.burgess.iv, rsmith, aaron.ballman.
Herald added subscribers: jdoerfert, dexonsmith, jkorous.
Herald added a project: clang.

These diagnose overflowing calls to subset of fortifiable functions. Some 
functions, like `sprintf` or `strcpy` aren't supported right not, but we should 
probably support these in the future. We previously supported this kind of 
functionality with `-Wbuiltin-memcpy-chk-size`, but that diagnose doesn't work 
with `_FORTIFY` implementations that use wrapper functions. Also unlike that 
diagnostic, we emit these warnings regardless of whether `_FORTIFY_SOURCE` is 
actually enabled, which is nice for programs that don't enable the runtime 
checks.

Why not just use diagnose_if, like Bionic does? We can get better diagnostics 
in the compiler (i.e. mention the sizes), and we have the potential to diagnose 
`sprintf` and `strcpy` which is impossible with diagnose_if (at least, in 
languages that don't support C++14 constexpr). This approach also saves 
standard libraries from having to add diagnose_if.

rdar://48006655

Thanks for taking a look!
Erik


Repository:
  rC Clang

https://reviews.llvm.org/D58797

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Analysis/bstring.c
  clang/test/Analysis/null-deref-ps-region.c
  clang/test/Analysis/pr22954.c
  clang/test/Analysis/string.c
  clang/test/Sema/builtin-object-size.c
  clang/test/Sema/builtins.c
  clang/test/Sema/transpose-memset.c
  clang/test/Sema/warn-fortify-source.c
  clang/test/Sema/warn-strncat-size.c

Index: clang/test/Sema/warn-strncat-size.c
===
--- clang/test/Sema/warn-strncat-size.c
+++ clang/test/Sema/warn-strncat-size.c
@@ -39,7 +39,7 @@
   strncat(dest, "AAA", sizeof(dest) - strlen(dest)); // expected-warning{{the value of the size argument in 'strncat' is too large, might lead to a buffer overflow}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}}
 
   strncat((*s5)->f2[x], s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}}
-  strncat(s1+3, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}}
+  strncat(s1+3, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-warning {{strncat' size argument is too large; destination buffer has size 97, but size argument is 200}}
   strncat(s4.f1, s2, sizeof(s2)); // expected-warning {{size argument in 'strncat' call appears to be size of the source}} expected-note {{change the argument to be the free space in the destination buffer minus the terminating null byte}}
 }
 
Index: clang/test/Sema/warn-fortify-source.c
===
--- /dev/null
+++ clang/test/Sema/warn-fortify-source.c
@@ -0,0 +1,83 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_PASS_OBJECT_SIZE
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
+
+typedef unsigned long size_t;
+
+#if defined(USE_PASS_OBJECT_SIZE)
+void *memcpy(void *dst, const void *src, size_t c);
+static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
+static void *memcpy(void *const dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) {
+  return 0;
+}
+#elif defined(USE_BUILTINS)
+#define memcpy(x,y,z) __builtin_memcpy(x,y,z)
+#else
+void *memcpy(void *dst, const void *src, size_t c);
+#endif
+
+void call_memcpy() {
+  char dst[10];
+  char src[20];
+  memcpy(dst, src, 20); // expected-warning {{memcpy' will always overflow; destination buffer has size 10, but size argument is 20}}
+}
+
+void call_memcpy_type() {
+  struct pair {
+int first;
+int second;
+  };
+  struct pair p;
+  char buf[20];
+  memcpy(&p.first, buf, 20);
+#ifdef USE_PASS_OBJECT_SIZE
+  // Use the more strict checking mode on the pass_object_size attribute:
+  // expected-warning@-3 {{memcpy' will always overflow; destination buffer has size 4, but size argument is 20}}
+#else
+  // Or just fallback to type 0:
+  // expected-warning@-6 {{memcpy' will always overflow; destination buffer has size 8, but size argument is 20}}
+#endif
+}
+
+void call_strncat() {
+  char s1[10], s2[20];
+  __builtin_strncat(s2, s1, 20);
+  __builtin