From 03b182422aec68f1567433aa3ae4eb9c0dd7e21a Mon Sep 17 00:00:00 2001 From: Fariborz Jahanian Date: Thu, 18 Sep 2014 17:58:27 +0000 Subject: [PATCH] Patch to check at compile time for overflow when __builtin___memcpy_chk and similar builtins are being used. Patch by Jacques Fortier (with added clang tests). rdar://11076881 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@218063 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticSemaKinds.td | 4 ++ include/clang/Sema/Sema.h | 3 +- lib/Sema/SemaChecking.cpp | 49 +++++++++++++++++++++- lib/Sema/SemaExpr.cpp | 4 +- test/Sema/builtin-object-size.c | 2 +- test/Sema/builtins.c | 25 ++++++++++- 6 files changed, 80 insertions(+), 7 deletions(-) diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index d955e4eaff..7e08fce794 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -454,6 +454,10 @@ def warn_assume_side_effects : Warning< "the argument to %0 has side effects that will be discarded">, InGroup>; +def warn_memcpy_chk_overflow : Warning< + "%0 will always overflow destination buffer">, + InGroup>; + /// main() // static main() is not an error in C, just in C++. def warn_static_main : Warning<"'main' should not be declared static">, diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 3755256da9..b262c45cd7 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -8355,7 +8355,8 @@ private: bool CheckObjCString(Expr *Arg); - ExprResult CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall); + ExprResult CheckBuiltinFunctionCall(FunctionDecl *FDecl, + unsigned BuiltinID, CallExpr *TheCall); bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall, unsigned MaxWidth); diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index 953b3f67d2..7462869306 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/lib/Sema/SemaChecking.cpp @@ -111,8 +111,37 @@ static bool SemaBuiltinAddressof(Sema &S, CallExpr *TheCall) { return false; } +static void SemaBuiltinMemChkCall(Sema &S, FunctionDecl *FDecl, + CallExpr *TheCall, unsigned SizeIdx, + unsigned DstSizeIdx) { + if (TheCall->getNumArgs() <= SizeIdx || + TheCall->getNumArgs() <= DstSizeIdx) + return; + + const Expr *SizeArg = TheCall->getArg(SizeIdx); + const Expr *DstSizeArg = TheCall->getArg(DstSizeIdx); + + llvm::APSInt Size, DstSize; + + // find out if both sizes are known at compile time + if (!SizeArg->EvaluateAsInt(Size, S.Context) || + !DstSizeArg->EvaluateAsInt(DstSize, S.Context)) + return; + + if (Size.ule(DstSize)) + return; + + // confirmed overflow so generate the diagnostic. + IdentifierInfo *FnName = FDecl->getIdentifier(); + SourceLocation SL = TheCall->getLocStart(); + SourceRange SR = TheCall->getSourceRange(); + + S.Diag(SL, diag::warn_memcpy_chk_overflow) << SR << FnName; +} + ExprResult -Sema::CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) { +Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID, + CallExpr *TheCall) { ExprResult TheCallResult(TheCall); // Find out if any arguments are required to be integer constant expressions. @@ -332,6 +361,24 @@ Sema::CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) { // so ensure that they are declared. DeclareGlobalNewDelete(); break; + + // check secure string manipulation functions where overflows + // are detectable at compile time + case Builtin::BI__builtin___memcpy_chk: + case Builtin::BI__builtin___memccpy_chk: + case Builtin::BI__builtin___memmove_chk: + case Builtin::BI__builtin___memset_chk: + case Builtin::BI__builtin___strlcat_chk: + case Builtin::BI__builtin___strlcpy_chk: + case Builtin::BI__builtin___strncat_chk: + case Builtin::BI__builtin___strncpy_chk: + case Builtin::BI__builtin___stpncpy_chk: + SemaBuiltinMemChkCall(*this, FDecl, TheCall, 2, 3); + break; + case Builtin::BI__builtin___snprintf_chk: + case Builtin::BI__builtin___vsnprintf_chk: + SemaBuiltinMemChkCall(*this, FDecl, TheCall, 1, 3); + break; } // Since the target specific builtins for each arch overlap, only check those diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 6b01b1f4b0..50bc4d2ac8 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -4665,7 +4665,7 @@ Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl, // Bail out early if calling a builtin with custom typechecking. if (BuiltinID && Context.BuiltinInfo.hasCustomTypechecking(BuiltinID)) - return CheckBuiltinFunctionCall(BuiltinID, TheCall); + return CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall); retry: const FunctionType *FuncT; @@ -4793,7 +4793,7 @@ Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl, return ExprError(); if (BuiltinID) - return CheckBuiltinFunctionCall(BuiltinID, TheCall); + return CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall); } else if (NDecl) { if (CheckPointerCall(NDecl, TheCall, Proto)) return ExprError(); diff --git a/test/Sema/builtin-object-size.c b/test/Sema/builtin-object-size.c index 0abc27ba18..db4832e347 100644 --- a/test/Sema/builtin-object-size.c +++ b/test/Sema/builtin-object-size.c @@ -23,6 +23,6 @@ int f3() { // rdar://6252231 - cannot call vsnprintf with va_list on x86_64 void f4(const char *fmt, ...) { __builtin_va_list args; - __builtin___vsnprintf_chk (0, 42, 0, 11, fmt, args); + __builtin___vsnprintf_chk (0, 42, 0, 11, fmt, args); // expected-warning {{'__builtin___vsnprintf_chk' will always overflow destination buffer}} } diff --git a/test/Sema/builtins.c b/test/Sema/builtins.c index 8e3a60ab06..f8df2f85cf 100644 --- a/test/Sema/builtins.c +++ b/test/Sema/builtins.c @@ -215,10 +215,31 @@ void Test19(void) 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}} \\ // 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-note {{change size argument to be the size of the destination}} \ + // expected-warning {{'__builtin___strlcpy_chk' will always overflow destination buffer}} 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-note {{change size argument to be the size of the destination}} \ + // expected-warning {{'__builtin___strlcat_chk' will always overflow destination buffer}} +} + +// rdar://11076881 +char * Test20(char *p, const char *in, unsigned n) +{ + static char buf[10]; + + __builtin___memcpy_chk (&buf[6], in, 5, __builtin_object_size (&buf[6], 0)); // expected-warning {{'__builtin___memcpy_chk' will always overflow destination buffer}} + + __builtin___memcpy_chk (p, "abcde", n, __builtin_object_size (p, 0)); + + __builtin___memcpy_chk (&buf[5], "abcde", 5, __builtin_object_size (&buf[5], 0)); + + __builtin___memcpy_chk (&buf[5], "abcde", n, __builtin_object_size (&buf[5], 0)); + + __builtin___memcpy_chk (&buf[6], "abcde", 5, __builtin_object_size (&buf[6], 0)); // expected-warning {{'__builtin___memcpy_chk' will always overflow destination buffer}} + + return buf; } -- 2.50.1