Add experimental -Wstrlcpy-size warning that looks to see if the size argument for...

Warning logic provided by Geoff Keating.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@137903 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/Builtins.def b/include/clang/Basic/Builtins.def
index a3cc6156238e7d88e1131cef6160ce9d019544b1..f45ca4458f62acd6a5b59729dbb599e27a37f17e 100644 (file)
--- a/include/clang/Basic/Builtins.def
+++ b/include/clang/Basic/Builtins.def
@@ -664,6 +664,9 @@ LIBBUILTIN(_exit, "vi",           "fr",    "unistd.h", ALL_LANGUAGES)
 // POSIX setjmp.h
 LIBBUILTIN(_longjmp, "vJi",       "fr",    "setjmp.h", ALL_LANGUAGES)
 LIBBUILTIN(siglongjmp, "vSJi",    "fr",    "setjmp.h", ALL_LANGUAGES)
+// non-standard but very common
+LIBBUILTIN(strlcpy, "zc*cC*z",    "f",     "string.h", ALL_LANGUAGES)
+LIBBUILTIN(strlcat, "zc*cC*z",    "f",     "string.h", ALL_LANGUAGES)
 //   id objc_msgSend(id, SEL, ...)
 LIBBUILTIN(objc_msgSend, "GGH.",   "f",     "objc/message.h", OBJC_LANG)
 

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 13f3dbc1d41e63ec1ec4227dd403dcdf8f874b68..809e1d1bec36ec62567a867a05c55abfad2aeba6 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -278,6 +278,13 @@ def warn_sizeof_pointer_type_memaccess : Warning<
   "argument to 'sizeof' in %0 call is the same pointer type %1 as the "
   "%select{destination|source}2; expected %3 or an explicit length">,
   InGroup<DiagGroup<"sizeof-pointer-memaccess">>;
+def warn_strlcpycat_wrong_size : Warning<
+  "size argument in %0 call appears to be size of the source; expected the size of "
+  "the destination">,
+  DefaultIgnore,
+  InGroup<DiagGroup<"strlcpy-size">>;
+def note_strlcpycat_wrong_size : Note<
+  "change size argument to be the size of the destination">;
 
 /// main()
 // static/inline main() are not errors in C, just in C++.

diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 3abdbd70ec7c571fd28e73e915b92d584df28d3d..520afbc5d243a5b3e7a93892b85af3503ddae1e9 100644 (file)
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -5973,6 +5973,9 @@ private:
   void CheckMemaccessArguments(const CallExpr *Call, CheckedMemoryFunction CMF,
                                IdentifierInfo *FnName);
 
+  void CheckStrlcpycatArguments(const CallExpr *Call,
+                                IdentifierInfo *FnName);
+
   void CheckReturnStackAddr(Expr *RetValExp, QualType lhsType,
                             SourceLocation ReturnLoc);
   void CheckFloatComparison(SourceLocation loc, Expr* lex, Expr* rex);

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 94e1c4f331e3649782338e66ad146b491c7b5a36..882668e515165784a9ba8b581356cee59f008b2e 100644 (file)
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -319,7 +319,7 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall) {
                           TheCall->getCallee()->getLocStart());
   }
 
-  // Memset/memcpy/memmove/memcmp handling
+  // Builtin handling
   int CMF = -1;
   switch (FDecl->getBuiltinID()) {
   case Builtin::BI__builtin_memset:
@@ -339,6 +339,11 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall) {
   case Builtin::BImemmove:
     CMF = CMF_Memmove;
     break;
+
+  case Builtin::BIstrlcpy:
+  case Builtin::BIstrlcat:
+    CheckStrlcpycatArguments(TheCall, FnInfo);
+    break;
     
   case Builtin::BI__builtin_memcmp:
     CMF = CMF_Memcmp;
@@ -359,6 +364,7 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall) {
     break;
   }
    
+  // Memset/memcpy/memmove handling
   if (CMF != -1)
     CheckMemaccessArguments(TheCall, CheckedMemoryFunction(CMF), FnInfo);
 
@@ -1990,6 +1996,95 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
   }
 }
 
+// A little helper routine: ignore addition and subtraction of integer literals.
+// This intentionally does not ignore all integer constant expressions because
+// we don't want to remove sizeof().
+static const Expr *ignoreLiteralAdditions(const Expr *Ex, ASTContext &Ctx) {
+  Ex = Ex->IgnoreParenCasts();
+
+  for (;;) {
+    const BinaryOperator * BO = dyn_cast<BinaryOperator>(Ex);
+    if (!BO || !BO->isAdditiveOp())
+      break;
+
+    const Expr *RHS = BO->getRHS()->IgnoreParenCasts();
+    const Expr *LHS = BO->getLHS()->IgnoreParenCasts();
+    
+    if (isa<IntegerLiteral>(RHS))
+      Ex = LHS;
+    else if (isa<IntegerLiteral>(LHS))
+      Ex = RHS;
+    else
+      break;
+  }
+
+  return Ex;
+}
+
+// Warn if the user has made the 'size' argument to strlcpy or strlcat
+// be the size of the source, instead of the destination.
+void Sema::CheckStrlcpycatArguments(const CallExpr *Call,
+                                    IdentifierInfo *FnName) {
+
+  // Don't crash if the user has the wrong number of arguments
+  if (Call->getNumArgs() != 3)
+    return;
+
+  const Expr *SrcArg = ignoreLiteralAdditions(Call->getArg(1), Context);
+  const Expr *SizeArg = ignoreLiteralAdditions(Call->getArg(2), Context);
+  const Expr *CompareWithSrc = NULL;
+  
+  // Look for 'strlcpy(dst, x, sizeof(x))'
+  if (const Expr *Ex = getSizeOfExprArg(SizeArg))
+    CompareWithSrc = Ex;
+  else {
+    // Look for 'strlcpy(dst, x, strlen(x))'
+    if (const CallExpr *SizeCall = dyn_cast<CallExpr>(SizeArg)) {
+      if (SizeCall->isBuiltinCall(Context) == Builtin::BIstrlen
+          && SizeCall->getNumArgs() == 1)
+        CompareWithSrc = ignoreLiteralAdditions(SizeCall->getArg(0), Context);
+    }
+  }
+
+  if (!CompareWithSrc)
+    return;
+
+  // Determine if the argument to sizeof/strlen is equal to the source
+  // argument.  In principle there's all kinds of things you could do
+  // here, for instance creating an == expression and evaluating it with
+  // EvaluateAsBooleanCondition, but this uses a more direct technique:
+  const DeclRefExpr *SrcArgDRE = dyn_cast<DeclRefExpr>(SrcArg);
+  if (!SrcArgDRE)
+    return;
+  
+  const DeclRefExpr *CompareWithSrcDRE = dyn_cast<DeclRefExpr>(CompareWithSrc);
+  if (!CompareWithSrcDRE || 
+      SrcArgDRE->getDecl() != CompareWithSrcDRE->getDecl())
+    return;
+  
+  const Expr *OriginalSizeArg = Call->getArg(2);
+  Diag(CompareWithSrcDRE->getLocStart(), diag::warn_strlcpycat_wrong_size)
+    << OriginalSizeArg->getSourceRange() << FnName;
+  
+  // Output a FIXIT hint if the destination is an array (rather than a
+  // pointer to an array).  This could be enhanced to handle some
+  // pointers if we know the actual size, like if DstArg is 'array+2'
+  // we could say 'sizeof(array)-2'.
+  const Expr *DstArg = Call->getArg(0)->IgnoreParenImpCasts();
+  
+  if (DstArg->getType()->isArrayType()) {
+    llvm::SmallString<128> sizeString;
+    llvm::raw_svector_ostream OS(sizeString);
+    OS << "sizeof(";
+    DstArg->printPretty(OS, Context, 0, Context.PrintingPolicy);
+    OS << ")";
+    
+    Diag(OriginalSizeArg->getLocStart(), diag::note_strlcpycat_wrong_size)
+      << FixItHint::CreateReplacement(OriginalSizeArg->getSourceRange(),
+                                      OS.str());
+  }
+}
+
 //===--- CHECK: Return Address of Stack Variable --------------------------===//
 
 static Expr *EvalVal(Expr *E, SmallVectorImpl<DeclRefExpr *> &refVars);

diff --git a/test/Sema/warn-strlcpycat-size.c b/test/Sema/warn-strlcpycat-size.c
new file mode 100644 (file)
index 0000000..58cb50f
--- /dev/null
+++ b/test/Sema/warn-strlcpycat-size.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -Wstrlcpy-size -verify -fsyntax-only %s
+
+typedef unsigned long size_t;
+size_t strlcpy (char * restrict dst, const char * restrict src, size_t size);
+size_t strlcat (char * restrict dst, const char * restrict src, size_t size);
+size_t strlen (const char *s);
+
+char s1[100];
+char s2[200];
+char * s3;
+
+struct {
+  char f1[100];
+  char f2[100][3];
+} s4, **s5;
+
+int x;
+
+void f(void)
+{
+  strlcpy(s1, s2, sizeof(s1)); // no warning
+  strlcpy(s1, s2, sizeof(s2)); // 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}}
+  strlcpy(s1, s3, strlen(s3)+1); // 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}}
+  strlcat(s2, s3, sizeof(s3)); // 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}}
+  strlcpy(s4.f1, s2, sizeof(s2)); // 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}}
+  strlcpy((*s5)->f2[x], s2, sizeof(s2)); // 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}}
+  strlcpy(s1+3, s2, sizeof(s2)); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}}
+}