[analyzer] CStringChecker: Fix a crash when an argument of a weird type is encountered.

[NoQ]

Summary

It turns out that it's not all that uncommon to have a C++ override of, say, memcpy that receives a structure (or two) by reference (or by value, if it's being copied from) and copies memory from it (or into it, if it's passed by reference). In this case the argument will be of structure type (recall that expressions of reference type do not exist: instead, C++ classifies expressions into prvalues and lvalues and xvalues).

In this scenario we crash because we are trying to assume that, say, a memory region is equal to an empty CompoundValue (the non-lazy one; this is what makeZeroVal() return for compound types and it represents prvalue of an object that is initialized with an empty initializer list).

Add defensive checks. We should probably extend CallDescription so that it encapsulated these checks and we were always sure that this is the function we're looking for.

[dcoughlin]

LGTM.

test/Analysis/string.cpp

24

I'm pretty sure you mean 'overload' instead of 'override' here and elsewhere.

[Szelethus]

Cheers!

[NoQ]

Indeed :)

[NoQ]

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2317–2320

Yes, indeed, this patch does break modeling of std::copy, because, well, we *did* have explicit modeling of std::copy.

[NoQ]

Fxd.

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

ProgramStateRef CheckBufferAccess(CheckerContext &C,
                                         bool WarnAboutSize = false) const;
 
   ProgramStateRef CheckBufferAccess(CheckerContext &C,
                                         ProgramStateRef state,
                                         const Expr *Size,
                                         const Expr *Buf,
                                         const char *message = nullptr,
                                         bool WarnAboutSize = false) const {
-    // This is a convenience override.
+    // This is a convenience overload.
     return CheckBufferAccess(C, state, Size, Buf, nullptr, message, nullptr,
                              WarnAboutSize);
   }
   ProgramStateRef CheckOverlap(CheckerContext &C,
                                    ProgramStateRef state,
                                    const Expr *Size,
                                    const Expr *First,
                                    const Expr *Second) const;
static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) {
 
   return false;
 }
 //===----------------------------------------------------------------------===//
 // The driver method, and other Checker callbacks.
 //===----------------------------------------------------------------------===//
 
 bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
-  const FunctionDecl *FDecl = C.getCalleeDecl(CE);
 
+  const FunctionDecl *FDecl = C.getCalleeDecl(CE);
   if (!FDecl)
     return false;
 
+  // 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.
+  for (auto I: CE->arguments()) {
+    QualType T = I->getType();
+    if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
+      return false;
+  }
+
   // FIXME: Poorly-factored string switches are slow.
   FnCheck evalFunction = nullptr;
   if (C.isCLibraryFunction(FDecl, "memcpy"))
     evalFunction =  &CStringChecker::evalMemcpy;
   else if (C.isCLibraryFunction(FDecl, "mempcpy"))
     evalFunction =  &CStringChecker::evalMempcpy;
   else if (C.isCLibraryFunction(FDecl, "memcmp"))
     evalFunction =  &CStringChecker::evalMemcmp;
bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
   else if (C.isCLibraryFunction(FDecl, "strncasecmp"))
     evalFunction =  &CStringChecker::evalStrncasecmp;
   else if (C.isCLibraryFunction(FDecl, "strsep"))
     evalFunction =  &CStringChecker::evalStrsep;
   else if (C.isCLibraryFunction(FDecl, "bcopy"))
     evalFunction =  &CStringChecker::evalBcopy;
   else if (C.isCLibraryFunction(FDecl, "bcmp"))
     evalFunction =  &CStringChecker::evalMemcmp;
   else if (isCPPStdLibraryFunction(FDecl, "copy"))
     evalFunction =  &CStringChecker::evalStdCopy;
   else if (isCPPStdLibraryFunction(FDecl, "copy_backward"))
     evalFunction =  &CStringChecker::evalStdCopyBackward;
   else if (C.isCLibraryFunction(FDecl, "bzero") ||
     C.isCLibraryFunction(FDecl, "explicit_bzero"))
     evalFunction =  &CStringChecker::evalBzero;
 
   // If the callee isn't a string function, let another checker handle it.
   if (!evalFunction)
     return false;
 

test/Analysis/string.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s
+
+// expected-no-diagnostics
+
+// Test functions that are called "memcpy" but aren't the memcpy
+// we're looking for. Unfortunately, this test cannot be put into
+// a namespace. The out-of-class weird memcpy needs to be recognized
+// as a normal C function for the test to make sense.
+typedef __typeof(sizeof(int)) size_t;
+void *memcpy(void *, const void *, size_t);
+
+struct S {
+  static S s1, s2;
+
+  // A weird overload within the class that accepts a structure reference
+  // instead of a pointer.
+  void memcpy(void *, const S &, size_t);
+  void test_in_class_weird_memcpy() {
+    memcpy(this, s2, 1); // no-crash
+  }
+};
+
+// A similarly weird overload outside of the class.
+void *memcpy(void *, const S &, size_t);
+
+void test_out_of_class_weird_memcpy() {
+  memcpy(&S::s1, S::s2, 1); // no-crash
+}