[analyzer] Modernize CStringChecker to use CallDescriptions.

[NoQ]

Summary

This uses the new CDF_MaybeBuiltin flag to handle C library functions. It's mostly a 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.

[a_sidorin]

LGTM!

[Szelethus]

That's beautiful *-*

[xazax.hun]

Nice one :)

[NoQ]

Remove more dead code!~

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

checkRegionChanges(ProgramStateRef state,
                        const InvalidatedSymbols *,
                        ArrayRef<const MemRegion *> ExplicitRegions,
                        ArrayRef<const MemRegion *> Regions,
                        const LocationContext *LCtx,
                        const CallEvent *Call) const;
 
   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;
   void evalBcopy(CheckerContext &C, const CallExpr *CE) const;
   void evalCopyCommon(CheckerContext &C, const CallExpr *CE,
                       ProgramStateRef state,
                       const Expr *Size,
                       const Expr *Source,
state = InvalidateBuffer(C, state, Source, C.getSVal(Source),
                              /*IsSourceBuffer*/true, nullptr);
 
     C.addTransition(state);
   }
 }
 
 
 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);
   ProgramStateRef state = C.getState();
 
   evalCopyCommon(C, CE, state, CE->getArg(2), Dest, CE->getArg(1), true);
 }
 
 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);
   ProgramStateRef state = C.getState();
 
   evalCopyCommon(C, CE, state, CE->getArg(2), Dest, CE->getArg(1), true, true);
 }
 
 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);
   ProgramStateRef state = C.getState();
 
   evalCopyCommon(C, CE, state, CE->getArg(2), Dest, CE->getArg(1));
 }
 
 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";
 
   const Expr *Left = CE->getArg(0);
   const Expr *Right = CE->getArg(1);
   const Expr *Size = CE->getArg(2);
 
   ProgramStateRef state = C.getState();
if (StNotSameBuf) {
         C.addTransition(state);
       }
     }
   }
 }
 
 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);
 }
 
 void CStringChecker::evalstrLengthCommon(CheckerContext &C, const CallExpr *CE,
                                          bool IsStrnlen) const {
   CurrentFunctionDescription = "string length function";
   ProgramStateRef state = C.getState();
void CStringChecker::evalstrLengthCommon(CheckerContext &C, const CallExpr *CE,
 
   // Bind the return value.
   assert(!result.isUnknown() && "Should have conjured a value by now");
   state = state->BindExpr(CE, LCtx, result);
   C.addTransition(state);
 }
 
 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,
                    /* isBounded = */ false,
                    /* isAppending = */ false);
 }
 
 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,
                    /* isBounded = */ true,
                    /* isAppending = */ false);
 }
 
 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,
                    /* isBounded = */ false,
                    /* isAppending = */ false);
 }
 
 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,
                    /* isBounded = */ true,
                    /* isAppending = */ false,
                    /* returnPtr = */ false);
 }
 
 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,
                    /* isBounded = */ false,
                    /* isAppending = */ true);
 }
 
 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,
                    /* isBounded = */ true,
                    /* isAppending = */ true);
 }
 
 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.
 
   //char *strlcat(char *s1, const char *s2, size_t n);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ false,
                    /* isBounded = */ true,
if (returnPtr) {
     }
   }
   // Set the return value.
   state = state->BindExpr(CE, LCtx, Result);
   C.addTransition(state);
 }
 
 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);
 }
 
 void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
     bool isBounded, bool ignoreCase) const {
   CurrentFunctionDescription = "string comparison function";
   ProgramStateRef state = C.getState();
void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
   state = state->BindExpr(CE, LCtx, resultVal);
 
   // Record this as a possible path.
   C.addTransition(state);
 }
 
 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();
   if (CharPtrTy.isNull() ||
       CE->getType().getUnqualifiedType() != CharPtrTy.getUnqualifiedType())
     return;
 
   CurrentFunctionDescription = "strsep()";

 
 void CStringChecker::evalStdCopyBackward(CheckerContext &C,
     const CallExpr *CE) const {
   evalStdCopyCommon(C, CE);
 }
 
 void CStringChecker::evalStdCopyCommon(CheckerContext &C,
     const CallExpr *CE) const {
-  if (CE->getNumArgs() < 3)
+  if (!CE->getArg(2)->getType()->isPointerType())
     return;
 
   ProgramStateRef State = C.getState();
 
   const LocationContext *LCtx = C.getLocationContext();
 
   // template <class _InputIterator, class _OutputIterator>
   // _OutputIterator
void CStringChecker::evalStdCopyCommon(CheckerContext &C,
 
   SVal ResultVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
   State = State->BindExpr(CE, LCtx, ResultVal);
 
   C.addTransition(State);
 }
 
 void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() != 3)
-    return;
-
   CurrentFunctionDescription = "memory set function";
 
   const Expr *Mem = CE->getArg(0);
   const Expr *CharE = CE->getArg(1);
   const Expr *Size = CE->getArg(2);
   ProgramStateRef State = C.getState();
 
   // See if the size argument is zero.
void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const {
   if (!memsetAux(Mem, C.getSVal(CharE), Size, C, State))
     return;
 
   State = State->BindExpr(CE, LCtx, MemVal);
   C.addTransition(State);
 }
 
 void CStringChecker::evalBzero(CheckerContext &C, const CallExpr *CE) const {
-  if (CE->getNumArgs() != 2)
-    return;
-
   CurrentFunctionDescription = "memory clearance function";
 
   const Expr *Mem = CE->getArg(0);
   const Expr *Size = CE->getArg(1);
   SVal Zero = C.getSValBuilder().makeZeroVal(C.getASTContext().IntTy);
 
   ProgramStateRef State = C.getState();
   
if (!State)
     return;
 
   if (!memsetAux(Mem, Zero, Size, C, State))
     return;
 
   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;
 
-  // 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;
+  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+  if (!FD)
+
+  if (Call.isCalled(StdCopy)) {
     return &CStringChecker::evalStdCopy;
-  } else if (isCPPStdLibraryFunction(FDecl, "copy_backward")) {
-    if (CE->getNumArgs() < 3 || !CE->getArg(2)->getType()->isPointerType())
-      return nullptr;
+  } else if (Call.isCalled(StdCopyBackward)) {
     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;
-    }
-  }
+  }
+
+  // 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. We could not check
+  // that for std::copy because they may have arguments of other types.
+  for (auto I : CE->arguments()) {
 
-  // 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.
   // Note, the custom CString evaluation calls assume that basic safety
   // properties are held. However, if the user chooses to turn off some of these
   // checks, we ignore the issues and leave the call evaluation to a generic
   // handler.
   return C.isDifferent();

clang/test/Analysis/string.c

void memset28() {
   clang_analyzer_eval(x == 0x101); // expected-warning{{UNKNOWN}}
 }
 
 void memset29_plain_int_zero() {
   short x;
   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}}
+}