r364868 - [analyzer] CStringChecker: Modernize to use CallDescriptions.

Artem Dergachev via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 1 16:02:11 PDT 2019


Author: dergachev
Date: Mon Jul  1 16:02:10 2019
New Revision: 364868

URL: http://llvm.org/viewvc/llvm-project?rev=364868&view=rev
Log:
[analyzer] CStringChecker: Modernize to use CallDescriptions.

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

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
    cfe/trunk/test/Analysis/string.c

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp?rev=364868&r1=364867&r2=364868&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp Mon Jul  1 16:02:10 2019
@@ -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(Chec
 
 
 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(CheckerC
 }
 
 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(Checker
 }
 
 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(Checker
 }
 
 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(CheckerC
 
 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
 }
 
 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(CheckerC
 }
 
 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(Checker
 }
 
 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(CheckerC
 }
 
 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(Checker
 }
 
 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(CheckerC
 }
 
 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(Checker
 }
 
 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(Ch
 }
 
 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(Ch
 
 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
 
 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(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(CheckerC
 }
 
 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(CheckerCo
   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.

Modified: cfe/trunk/test/Analysis/string.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/string.c?rev=364868&r1=364867&r2=364868&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/string.c (original)
+++ cfe/trunk/test/Analysis/string.c Mon Jul  1 16:02:10 2019
@@ -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}}
+}




More information about the cfe-commits mailing list