NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov,
rnkovacs, Szelethus, mikhail.ramalho, baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.
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.
Repository:
rC Clang
https://reviews.llvm.org/D55873
Files:
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
test/Analysis/string.cpp
Index: test/Analysis/string.cpp
===================================================================
--- /dev/null
+++ test/Analysis/string.cpp
@@ -0,0 +1,28 @@
+// 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 override 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 override 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
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -2255,11 +2255,20 @@
//===----------------------------------------------------------------------===//
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++ override 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"))
Index: test/Analysis/string.cpp
===================================================================
--- /dev/null
+++ test/Analysis/string.cpp
@@ -0,0 +1,28 @@
+// 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 override 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 override 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
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -2255,11 +2255,20 @@
//===----------------------------------------------------------------------===//
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++ override 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"))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
