[analyzer] CStringChecker: Modernize to use CallDescriptions.
authorArtem Dergachev <artem.dergachev@gmail.com>
Mon, 1 Jul 2019 23:02:10 +0000 (23:02 +0000)
committerArtem Dergachev <artem.dergachev@gmail.com>
Mon, 1 Jul 2019 23:02:10 +0000 (23:02 +0000)
This patch uses the new CDF_MaybeBuiltin flag to handle C library functions.
It's mostly an NFC/refactoring pass, but it does fix a bug in handling memset()
when it expands to __builtin___memset_chk() because the latter has
one more argument and memset() handling code was trying to match
the exact number of arguments. Now the code is deduplicated and there's
less room for mistakes.

Differential Revision: https://reviews.llvm.org/D62557

llvm-svn: 364868

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/test/Analysis/string.c

index 2aa1b22..44f4530 100644 (file)
@@ -73,7 +73,38 @@ public:
 
   typedef void (CStringChecker::*FnCheck)(CheckerContext &,
                                           const CallExpr *) const;
+  CallDescriptionMap<FnCheck> Callbacks = {
+      {{CDF_MaybeBuiltin, "memcpy", 3}, &CStringChecker::evalMemcpy},
+      {{CDF_MaybeBuiltin, "mempcpy", 3}, &CStringChecker::evalMempcpy},
+      {{CDF_MaybeBuiltin, "memcmp", 3}, &CStringChecker::evalMemcmp},
+      {{CDF_MaybeBuiltin, "memmove", 3}, &CStringChecker::evalMemmove},
+      {{CDF_MaybeBuiltin, "memset", 3}, &CStringChecker::evalMemset},
+      {{CDF_MaybeBuiltin, "explicit_memset", 3}, &CStringChecker::evalMemset},
+      {{CDF_MaybeBuiltin, "strcpy", 2}, &CStringChecker::evalStrcpy},
+      {{CDF_MaybeBuiltin, "strncpy", 3}, &CStringChecker::evalStrncpy},
+      {{CDF_MaybeBuiltin, "stpcpy", 2}, &CStringChecker::evalStpcpy},
+      {{CDF_MaybeBuiltin, "strlcpy", 3}, &CStringChecker::evalStrlcpy},
+      {{CDF_MaybeBuiltin, "strcat", 2}, &CStringChecker::evalStrcat},
+      {{CDF_MaybeBuiltin, "strncat", 3}, &CStringChecker::evalStrncat},
+      {{CDF_MaybeBuiltin, "strlcat", 3}, &CStringChecker::evalStrlcat},
+      {{CDF_MaybeBuiltin, "strlen", 1}, &CStringChecker::evalstrLength},
+      {{CDF_MaybeBuiltin, "strnlen", 2}, &CStringChecker::evalstrnLength},
+      {{CDF_MaybeBuiltin, "strcmp", 2}, &CStringChecker::evalStrcmp},
+      {{CDF_MaybeBuiltin, "strncmp", 3}, &CStringChecker::evalStrncmp},
+      {{CDF_MaybeBuiltin, "strcasecmp", 2}, &CStringChecker::evalStrcasecmp},
+      {{CDF_MaybeBuiltin, "strncasecmp", 3}, &CStringChecker::evalStrncasecmp},
+      {{CDF_MaybeBuiltin, "strsep", 2}, &CStringChecker::evalStrsep},
+      {{CDF_MaybeBuiltin, "bcopy", 3}, &CStringChecker::evalBcopy},
+      {{CDF_MaybeBuiltin, "bcmp", 3}, &CStringChecker::evalMemcmp},
+      {{CDF_MaybeBuiltin, "bzero", 2}, &CStringChecker::evalBzero},
+      {{CDF_MaybeBuiltin, "explicit_bzero", 2}, &CStringChecker::evalBzero},
+  };
+
+  // These require a bit of special handling.
+  CallDescription StdCopy{{"std", "copy"}, 3},
+      StdCopyBackward{{"std", "copy_backward"}, 3};
 
+  FnCheck identifyCall(const CallEvent &Call, CheckerContext &C) const;
   void evalMemcpy(CheckerContext &C, const CallExpr *CE) const;
   void evalMempcpy(CheckerContext &C, const CallExpr *CE) const;
   void evalMemmove(CheckerContext &C, const CallExpr *CE) const;
@@ -1201,9 +1232,6 @@ void CStringChecker::evalCopyCommon(CheckerContext &C,
 
 
 void CStringChecker::evalMemcpy(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // void *memcpy(void *restrict dst, const void *restrict src, size_t n);
   // The return value is the address of the destination buffer.
   const Expr *Dest = CE->getArg(0);
@@ -1213,9 +1241,6 @@ void CStringChecker::evalMemcpy(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalMempcpy(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // void *mempcpy(void *restrict dst, const void *restrict src, size_t n);
   // The return value is a pointer to the byte following the last written byte.
   const Expr *Dest = CE->getArg(0);
@@ -1225,9 +1250,6 @@ void CStringChecker::evalMempcpy(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalMemmove(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // void *memmove(void *dst, const void *src, size_t n);
   // The return value is the address of the destination buffer.
   const Expr *Dest = CE->getArg(0);
@@ -1237,18 +1259,12 @@ void CStringChecker::evalMemmove(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalBcopy(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // void bcopy(const void *src, void *dst, size_t n);
   evalCopyCommon(C, CE, C.getState(),
                  CE->getArg(2), CE->getArg(1), CE->getArg(0));
 }
 
 void CStringChecker::evalMemcmp(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // int memcmp(const void *s1, const void *s2, size_t n);
   CurrentFunctionDescription = "memory comparison function";
 
@@ -1323,18 +1339,12 @@ void CStringChecker::evalMemcmp(CheckerContext &C, const CallExpr *CE) const {
 
 void CStringChecker::evalstrLength(CheckerContext &C,
                                    const CallExpr *CE) const {
-  if (CE->getNumArgs() < 1)
-    return;
-
   // size_t strlen(const char *s);
   evalstrLengthCommon(C, CE, /* IsStrnlen = */ false);
 }
 
 void CStringChecker::evalstrnLength(CheckerContext &C,
                                     const CallExpr *CE) const {
-  if (CE->getNumArgs() < 2)
-    return;
-
   // size_t strnlen(const char *s, size_t maxlen);
   evalstrLengthCommon(C, CE, /* IsStrnlen = */ true);
 }
@@ -1459,9 +1469,6 @@ void CStringChecker::evalstrLengthCommon(CheckerContext &C, const CallExpr *CE,
 }
 
 void CStringChecker::evalStrcpy(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 2)
-    return;
-
   // char *strcpy(char *restrict dst, const char *restrict src);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ false,
@@ -1470,9 +1477,6 @@ void CStringChecker::evalStrcpy(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalStrncpy(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // char *strncpy(char *restrict dst, const char *restrict src, size_t n);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ false,
@@ -1481,9 +1485,6 @@ void CStringChecker::evalStrncpy(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalStpcpy(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 2)
-    return;
-
   // char *stpcpy(char *restrict dst, const char *restrict src);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ true,
@@ -1492,9 +1493,6 @@ void CStringChecker::evalStpcpy(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalStrlcpy(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // char *strlcpy(char *dst, const char *src, size_t n);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ true,
@@ -1504,9 +1502,6 @@ void CStringChecker::evalStrlcpy(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalStrcat(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 2)
-    return;
-
   //char *strcat(char *restrict s1, const char *restrict s2);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ false,
@@ -1515,9 +1510,6 @@ void CStringChecker::evalStrcat(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   //char *strncat(char *restrict s1, const char *restrict s2, size_t n);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ false,
@@ -1526,9 +1518,6 @@ void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalStrlcat(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   // FIXME: strlcat() uses a different rule for bound checking, i.e. 'n' means
   // a different thing as compared to strncat(). This currently causes
   // false positives in the alpha string bound checker.
@@ -1885,35 +1874,23 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
 }
 
 void CStringChecker::evalStrcmp(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 2)
-    return;
-
   //int strcmp(const char *s1, const char *s2);
   evalStrcmpCommon(C, CE, /* isBounded = */ false, /* ignoreCase = */ false);
 }
 
 void CStringChecker::evalStrncmp(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   //int strncmp(const char *s1, const char *s2, size_t n);
   evalStrcmpCommon(C, CE, /* isBounded = */ true, /* ignoreCase = */ false);
 }
 
 void CStringChecker::evalStrcasecmp(CheckerContext &C,
     const CallExpr *CE) const {
-  if (CE->getNumArgs() < 2)
-    return;
-
   //int strcasecmp(const char *s1, const char *s2);
   evalStrcmpCommon(C, CE, /* isBounded = */ false, /* ignoreCase = */ true);
 }
 
 void CStringChecker::evalStrncasecmp(CheckerContext &C,
     const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
-    return;
-
   //int strncasecmp(const char *s1, const char *s2, size_t n);
   evalStrcmpCommon(C, CE, /* isBounded = */ true, /* ignoreCase = */ true);
 }
@@ -2047,9 +2024,6 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
 
 void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const {
   //char *strsep(char **stringp, const char *delim);
-  if (CE->getNumArgs() < 2)
-    return;
-
   // Sanity: does the search string parameter match the return type?
   const Expr *SearchStrPtr = CE->getArg(0);
   QualType CharPtrTy = SearchStrPtr->getType()->getPointeeType();
@@ -2118,7 +2092,7 @@ void CStringChecker::evalStdCopyBackward(CheckerContext &C,
 
 void CStringChecker::evalStdCopyCommon(CheckerContext &C,
     const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
+  if (!CE->getArg(2)->getType()->isPointerType())
     return;
 
   ProgramStateRef State = C.getState();
@@ -2145,9 +2119,6 @@ void CStringChecker::evalStdCopyCommon(CheckerContext &C,
 }
 
 void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() != 3)
-    return;
-
   CurrentFunctionDescription = "memory set function";
 
   const Expr *Mem = CE->getArg(0);
@@ -2196,9 +2167,6 @@ void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalBzero(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() != 2)
-    return;
-
   CurrentFunctionDescription = "memory clearance function";
 
   const Expr *Mem = CE->getArg(0);
@@ -2241,110 +2209,53 @@ void CStringChecker::evalBzero(CheckerContext &C, const CallExpr *CE) const {
   C.addTransition(State);
 }
 
-static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) {
-  IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-    return false;
-
-  if (!AnalysisDeclContext::isInStdNamespace(FD))
-    return false;
-
-  if (II->getName().equals(Name))
-    return true;
-
-  return false;
-}
 //===----------------------------------------------------------------------===//
 // The driver method, and other Checker callbacks.
 //===----------------------------------------------------------------------===//
 
-static CStringChecker::FnCheck identifyCall(const CallExpr *CE,
-                                            CheckerContext &C) {
-  const FunctionDecl *FDecl = C.getCalleeDecl(CE);
-  if (!FDecl)
+CStringChecker::FnCheck CStringChecker::identifyCall(const CallEvent &Call,
+                                                     CheckerContext &C) const {
+  const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return nullptr;
+
+  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+  if (!FD)
     return nullptr;
 
+  if (Call.isCalled(StdCopy)) {
+    return &CStringChecker::evalStdCopy;
+  } else if (Call.isCalled(StdCopyBackward)) {
+    return &CStringChecker::evalStdCopyBackward;
+  }
+
   // Pro-actively check that argument types are safe to do arithmetic upon.
   // We do not want to crash if someone accidentally passes a structure
-  // into, say, a C++ overload of any of these functions.
-  if (isCPPStdLibraryFunction(FDecl, "copy")) {
-    if (CE->getNumArgs() < 3 || !CE->getArg(2)->getType()->isPointerType())
-      return nullptr;
-    return &CStringChecker::evalStdCopy;
-  } else if (isCPPStdLibraryFunction(FDecl, "copy_backward")) {
-    if (CE->getNumArgs() < 3 || !CE->getArg(2)->getType()->isPointerType())
+  // into, say, a C++ overload of any of these functions. We could not check
+  // that for std::copy because they may have arguments of other types.
+  for (auto I : CE->arguments()) {
+    QualType T = I->getType();
+    if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
       return nullptr;
-    return &CStringChecker::evalStdCopyBackward;
-  } else {
-    // An umbrella check for all C library functions.
-    for (auto I: CE->arguments()) {
-      QualType T = I->getType();
-      if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
-        return nullptr;
-    }
   }
 
-  // FIXME: Poorly-factored string switches are slow.
-  if (C.isCLibraryFunction(FDecl, "memcpy"))
-    return &CStringChecker::evalMemcpy;
-  else if (C.isCLibraryFunction(FDecl, "mempcpy"))
-    return &CStringChecker::evalMempcpy;
-  else if (C.isCLibraryFunction(FDecl, "memcmp"))
-    return &CStringChecker::evalMemcmp;
-  else if (C.isCLibraryFunction(FDecl, "memmove"))
-    return &CStringChecker::evalMemmove;
-  else if (C.isCLibraryFunction(FDecl, "memset") ||
-           C.isCLibraryFunction(FDecl, "explicit_memset"))
-    return &CStringChecker::evalMemset;
-  else if (C.isCLibraryFunction(FDecl, "strcpy"))
-    return &CStringChecker::evalStrcpy;
-  else if (C.isCLibraryFunction(FDecl, "strncpy"))
-    return &CStringChecker::evalStrncpy;
-  else if (C.isCLibraryFunction(FDecl, "stpcpy"))
-    return &CStringChecker::evalStpcpy;
-  else if (C.isCLibraryFunction(FDecl, "strlcpy"))
-    return &CStringChecker::evalStrlcpy;
-  else if (C.isCLibraryFunction(FDecl, "strcat"))
-    return &CStringChecker::evalStrcat;
-  else if (C.isCLibraryFunction(FDecl, "strncat"))
-    return &CStringChecker::evalStrncat;
-  else if (C.isCLibraryFunction(FDecl, "strlcat"))
-    return &CStringChecker::evalStrlcat;
-  else if (C.isCLibraryFunction(FDecl, "strlen"))
-    return &CStringChecker::evalstrLength;
-  else if (C.isCLibraryFunction(FDecl, "strnlen"))
-    return &CStringChecker::evalstrnLength;
-  else if (C.isCLibraryFunction(FDecl, "strcmp"))
-    return &CStringChecker::evalStrcmp;
-  else if (C.isCLibraryFunction(FDecl, "strncmp"))
-    return &CStringChecker::evalStrncmp;
-  else if (C.isCLibraryFunction(FDecl, "strcasecmp"))
-    return &CStringChecker::evalStrcasecmp;
-  else if (C.isCLibraryFunction(FDecl, "strncasecmp"))
-    return &CStringChecker::evalStrncasecmp;
-  else if (C.isCLibraryFunction(FDecl, "strsep"))
-    return &CStringChecker::evalStrsep;
-  else if (C.isCLibraryFunction(FDecl, "bcopy"))
-    return &CStringChecker::evalBcopy;
-  else if (C.isCLibraryFunction(FDecl, "bcmp"))
-    return &CStringChecker::evalMemcmp;
-  else if (C.isCLibraryFunction(FDecl, "bzero") ||
-           C.isCLibraryFunction(FDecl, "explicit_bzero"))
-    return &CStringChecker::evalBzero;
+  const FnCheck *Callback = Callbacks.lookup(Call);
+  if (Callback)
+    return *Callback;
 
   return nullptr;
 }
 
 bool CStringChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
-  const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  FnCheck evalFunction = identifyCall(CE, C);
+  FnCheck Callback = identifyCall(Call, C);
 
   // If the callee isn't a string function, let another checker handle it.
-  if (!evalFunction)
+  if (!Callback)
     return false;
 
   // Check and evaluate the call.
-  (this->*evalFunction)(C, CE);
+  const auto *CE = cast<CallExpr>(Call.getOriginExpr());
+  (this->*Callback)(C, CE);
 
   // If the evaluate call resulted in no change, chain to the next eval call
   // handler.
index d3b131e..841bc15 100644 (file)
@@ -1598,3 +1598,9 @@ void memset29_plain_int_zero() {
   memset(&x, 0, sizeof(short));
   clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 }
+
+void test_memset_chk() {
+  int x;
+  __builtin___memset_chk(&x, 0, sizeof(x), __builtin_object_size(&x, 0));
+  clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+}