<div dir="ltr"><div>Cool! Do these warnings fire on plain memcpy if the system headers don't arrange for memcpy to route to __builtin__memcpy_chk? If so, can you add tests for plain prototyped memcpy as you did for strlcpy in the previous test?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 18, 2014 at 10:58 AM, Fariborz Jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: fjahanian<br>
Date: Thu Sep 18 12:58:27 2014<br>
New Revision: 218063<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=218063&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=218063&view=rev</a><br>
Log:<br>
Patch to check at compile time for overflow when<br>
__builtin___memcpy_chk and similar builtins are<br>
being used. Patch by Jacques Fortier (with added<br>
clang tests). rdar://11076881<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/include/clang/Sema/Sema.h<br>
cfe/trunk/lib/Sema/SemaChecking.cpp<br>
cfe/trunk/lib/Sema/SemaExpr.cpp<br>
cfe/trunk/test/Sema/builtin-object-size.c<br>
cfe/trunk/test/Sema/builtins.c<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=218063&r1=218062&r2=218063&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=218063&r1=218062&r2=218063&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Sep 18 12:58:27 2014<br>
@@ -454,6 +454,10 @@ def warn_assume_side_effects : Warning<<br>
"the argument to %0 has side effects that will be discarded">,<br>
InGroup<DiagGroup<"assume">>;<br>
<br>
+def warn_memcpy_chk_overflow : Warning<<br>
+ "%0 will always overflow destination buffer">,<br>
+ InGroup<DiagGroup<"builtin-memcpy-chk-size">>;<br>
+<br>
/// main()<br>
// static main() is not an error in C, just in C++.<br>
def warn_static_main : Warning<"'main' should not be declared static">,<br>
<br>
Modified: cfe/trunk/include/clang/Sema/Sema.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=218063&r1=218062&r2=218063&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=218063&r1=218062&r2=218063&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Sema/Sema.h (original)<br>
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Sep 18 12:58:27 2014<br>
@@ -8355,7 +8355,8 @@ private:<br>
<br>
bool CheckObjCString(Expr *Arg);<br>
<br>
- ExprResult CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall);<br>
+ ExprResult CheckBuiltinFunctionCall(FunctionDecl *FDecl,<br>
+ unsigned BuiltinID, CallExpr *TheCall);<br>
<br>
bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall,<br>
unsigned MaxWidth);<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=218063&r1=218062&r2=218063&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=218063&r1=218062&r2=218063&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Sep 18 12:58:27 2014<br>
@@ -111,8 +111,37 @@ static bool SemaBuiltinAddressof(Sema &S<br>
return false;<br>
}<br>
<br>
+static void SemaBuiltinMemChkCall(Sema &S, FunctionDecl *FDecl,<br>
+ CallExpr *TheCall, unsigned SizeIdx,<br>
+ unsigned DstSizeIdx) {<br>
+ if (TheCall->getNumArgs() <= SizeIdx ||<br>
+ TheCall->getNumArgs() <= DstSizeIdx)<br>
+ return;<br>
+<br>
+ const Expr *SizeArg = TheCall->getArg(SizeIdx);<br>
+ const Expr *DstSizeArg = TheCall->getArg(DstSizeIdx);<br>
+<br>
+ llvm::APSInt Size, DstSize;<br>
+<br>
+ // find out if both sizes are known at compile time<br>
+ if (!SizeArg->EvaluateAsInt(Size, S.Context) ||<br>
+ !DstSizeArg->EvaluateAsInt(DstSize, S.Context))<br>
+ return;<br>
+<br>
+ if (Size.ule(DstSize))<br>
+ return;<br>
+<br>
+ // confirmed overflow so generate the diagnostic.<br>
+ IdentifierInfo *FnName = FDecl->getIdentifier();<br>
+ SourceLocation SL = TheCall->getLocStart();<br>
+ SourceRange SR = TheCall->getSourceRange();<br>
+<br>
+ S.Diag(SL, diag::warn_memcpy_chk_overflow) << SR << FnName;<br>
+}<br>
+<br>
ExprResult<br>
-Sema::CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {<br>
+Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,<br>
+ CallExpr *TheCall) {<br>
ExprResult TheCallResult(TheCall);<br>
<br>
// Find out if any arguments are required to be integer constant expressions.<br>
@@ -332,6 +361,24 @@ Sema::CheckBuiltinFunctionCall(unsigned<br>
// so ensure that they are declared.<br>
DeclareGlobalNewDelete();<br>
break;<br>
+<br>
+ // check secure string manipulation functions where overflows<br>
+ // are detectable at compile time<br>
+ case Builtin::BI__builtin___memcpy_chk:<br>
+ case Builtin::BI__builtin___memccpy_chk:<br>
+ case Builtin::BI__builtin___memmove_chk:<br>
+ case Builtin::BI__builtin___memset_chk:<br>
+ case Builtin::BI__builtin___strlcat_chk:<br>
+ case Builtin::BI__builtin___strlcpy_chk:<br>
+ case Builtin::BI__builtin___strncat_chk:<br>
+ case Builtin::BI__builtin___strncpy_chk:<br>
+ case Builtin::BI__builtin___stpncpy_chk:<br>
+ SemaBuiltinMemChkCall(*this, FDecl, TheCall, 2, 3);<br>
+ break;<br>
+ case Builtin::BI__builtin___snprintf_chk:<br>
+ case Builtin::BI__builtin___vsnprintf_chk:<br>
+ SemaBuiltinMemChkCall(*this, FDecl, TheCall, 1, 3);<br>
+ break;<br>
}<br>
<br>
// Since the target specific builtins for each arch overlap, only check those<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=218063&r1=218062&r2=218063&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=218063&r1=218062&r2=218063&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Sep 18 12:58:27 2014<br>
@@ -4665,7 +4665,7 @@ Sema::BuildResolvedCallExpr(Expr *Fn, Na<br>
<br>
// Bail out early if calling a builtin with custom typechecking.<br>
if (BuiltinID && Context.BuiltinInfo.hasCustomTypechecking(BuiltinID))<br>
- return CheckBuiltinFunctionCall(BuiltinID, TheCall);<br>
+ return CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall);<br>
<br>
retry:<br>
const FunctionType *FuncT;<br>
@@ -4793,7 +4793,7 @@ Sema::BuildResolvedCallExpr(Expr *Fn, Na<br>
return ExprError();<br>
<br>
if (BuiltinID)<br>
- return CheckBuiltinFunctionCall(BuiltinID, TheCall);<br>
+ return CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall);<br>
} else if (NDecl) {<br>
if (CheckPointerCall(NDecl, TheCall, Proto))<br>
return ExprError();<br>
<br>
Modified: cfe/trunk/test/Sema/builtin-object-size.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/builtin-object-size.c?rev=218063&r1=218062&r2=218063&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/builtin-object-size.c?rev=218063&r1=218062&r2=218063&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Sema/builtin-object-size.c (original)<br>
+++ cfe/trunk/test/Sema/builtin-object-size.c Thu Sep 18 12:58:27 2014<br>
@@ -23,6 +23,6 @@ int f3() {<br>
// rdar://6252231 - cannot call vsnprintf with va_list on x86_64<br>
void f4(const char *fmt, ...) {<br>
__builtin_va_list args;<br>
- __builtin___vsnprintf_chk (0, 42, 0, 11, fmt, args);<br>
+ __builtin___vsnprintf_chk (0, 42, 0, 11, fmt, args); // expected-warning {{'__builtin___vsnprintf_chk' will always overflow destination buffer}}<br>
}<br>
<br>
<br>
Modified: cfe/trunk/test/Sema/builtins.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/builtins.c?rev=218063&r1=218062&r2=218063&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/builtins.c?rev=218063&r1=218062&r2=218063&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Sema/builtins.c (original)<br>
+++ cfe/trunk/test/Sema/builtins.c Thu Sep 18 12:58:27 2014<br>
@@ -215,10 +215,31 @@ void Test19(void)<br>
strlcpy(buf, b, sizeof(b)); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}} \\<br>
// expected-note {{change size argument to be the size of the destination}}<br>
__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}} \<br>
- // expected-note {{change size argument to be the size of the destination}}<br>
+ // expected-note {{change size argument to be the size of the destination}} \<br>
+ // expected-warning {{'__builtin___strlcpy_chk' will always overflow destination buffer}}<br>
<br>
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}} \<br>
// expected-note {{change size argument to be the size of the destination}}<br>
+<br>
__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}} \<br>
- // expected-note {{change size argument to be the size of the destination}}<br>
+ // expected-note {{change size argument to be the size of the destination}} \<br>
+ // expected-warning {{'__builtin___strlcat_chk' will always overflow destination buffer}}<br>
+}<br>
+<br>
+// rdar://11076881<br>
+char * Test20(char *p, const char *in, unsigned n)<br>
+{<br>
+ static char buf[10];<br>
+<br>
+ __builtin___memcpy_chk (&buf[6], in, 5, __builtin_object_size (&buf[6], 0)); // expected-warning {{'__builtin___memcpy_chk' will always overflow destination buffer}}<br>
+<br>
+ __builtin___memcpy_chk (p, "abcde", n, __builtin_object_size (p, 0));<br>
+<br>
+ __builtin___memcpy_chk (&buf[5], "abcde", 5, __builtin_object_size (&buf[5], 0));<br>
+<br>
+ __builtin___memcpy_chk (&buf[5], "abcde", n, __builtin_object_size (&buf[5], 0));<br>
+<br>
+ __builtin___memcpy_chk (&buf[6], "abcde", 5, __builtin_object_size (&buf[6], 0)); // expected-warning {{'__builtin___memcpy_chk' will always overflow destination buffer}}<br>
+<br>
+ return buf;<br>
}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>