[analyzer] Assume that the allocated value is non-null before construction.

I.e. not after. In the c++-allocator-inlining=true mode, we need to make the
assumption that the conservatively evaluated operator new() has returned a
non-null value. Previously we did this on CXXNewExpr, but now we have to do that
before calling the constructor, because some clever constructors are sometimes
assuming that their "this" is null and doing weird stuff. We would also crash
upon evaluating CXXNewExpr when the allocator was inlined and returned null and
had a throw specification; this is UB even for custom allocators, but we still
need not to crash.

Added more FIXME tests to ensure that eventually we fix calling the constructor
for null return values.

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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@323370 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 9d6b056a2eeb3861cee14a6f7d20992dfa699e82..9dc50df93f28db036b1f0a6ee64fb26592daf477 100644 (file)
--- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -500,9 +500,24 @@ void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
     // is breaking too much to evaluate the no-op symbolic cast over it, so we
     // skip it for now.
     ProgramStateRef State = I->getState();
-    ValueBldr.generateNode(
-        CNE, I,
-        setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
+    SVal RetVal = State->getSVal(CNE, LCtx);
+
+    // If this allocation function is not declared as non-throwing, failures
+    // /must/ be signalled by exceptions, and thus the return value will never
+    // be NULL. -fno-exceptions does not influence this semantics.
+    // FIXME: GCC has a -fcheck-new option, which forces it to consider the case
+    // where new can return NULL. If we end up supporting that option, we can
+    // consider adding a check for it here.
+    // C++11 [basic.stc.dynamic.allocation]p3.
+    if (const FunctionDecl *FD = CNE->getOperatorNew()) {
+      QualType Ty = FD->getType();
+      if (const auto *ProtoType = Ty->getAs<FunctionProtoType>())
+        if (!ProtoType->isNothrow(getContext()))
+          State = State->assume(RetVal.castAs<DefinedOrUnknownSVal>(), true);
+    }
+
+    ValueBldr.generateNode(CNE, I,
+                           setCXXNewAllocatorValue(State, CNE, LCtx, RetVal));
   }
 
   ExplodedNodeSet DstPostPostCallCallback;
@@ -559,21 +574,21 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
     State = Call->invalidateRegions(blockCount);
     if (!State)
       return;
-  }
 
-  // If this allocation function is not declared as non-throwing, failures
-  // /must/ be signalled by exceptions, and thus the return value will never be
-  // NULL. -fno-exceptions does not influence this semantics.
-  // FIXME: GCC has a -fcheck-new option, which forces it to consider the case
-  // where new can return NULL. If we end up supporting that option, we can
-  // consider adding a check for it here.
-  // C++11 [basic.stc.dynamic.allocation]p3.
-  if (FD) {
-    QualType Ty = FD->getType();
-    if (const FunctionProtoType *ProtoType = Ty->getAs<FunctionProtoType>())
-      if (!ProtoType->isNothrow(getContext()))
-        if (auto dSymVal = symVal.getAs<DefinedOrUnknownSVal>())
-          State = State->assume(*dSymVal, true);
+    // If this allocation function is not declared as non-throwing, failures
+    // /must/ be signalled by exceptions, and thus the return value will never
+    // be NULL. -fno-exceptions does not influence this semantics.
+    // FIXME: GCC has a -fcheck-new option, which forces it to consider the case
+    // where new can return NULL. If we end up supporting that option, we can
+    // consider adding a check for it here.
+    // C++11 [basic.stc.dynamic.allocation]p3.
+    if (FD) {
+      QualType Ty = FD->getType();
+      if (const auto *ProtoType = Ty->getAs<FunctionProtoType>())
+        if (!ProtoType->isNothrow(getContext()))
+          if (auto dSymVal = symVal.getAs<DefinedOrUnknownSVal>())
+            State = State->assume(*dSymVal, true);
+    }
   }
 
   StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);

diff --git a/test/Analysis/new-ctor-conservative.cpp b/test/Analysis/new-ctor-conservative.cpp
index 4500e3a253d12020a60f173cab6e82efef26dab7..b82df9abf1e2102a1f8462a107e08e57514f116b 100644 (file)
--- a/test/Analysis/new-ctor-conservative.cpp
+++ b/test/Analysis/new-ctor-conservative.cpp
@@ -1,6 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
 
 struct S {
   int x;
@@ -27,3 +28,19 @@ void checkNewArray() {
   // FIXME: Should be true once we inline array constructors.
   clang_analyzer_eval(s[0].x == 1); // expected-warning{{UNKNOWN}}
 }
+
+struct NullS {
+  NullS() {
+    if (this) {}
+  }
+  NullS(int x) {
+    if (!this) {
+      clang_analyzer_warnIfReached(); // no-warning
+    }
+  }
+};
+
+void checkNullThis() {
+  NullS *nulls = new NullS(); // no-crash
+  NullS *nulls2 = new NullS(0);
+}

diff --git a/test/Analysis/new-ctor-null-throw.cpp b/test/Analysis/new-ctor-null-throw.cpp
new file mode 100644 (file)
index 0000000..cdaf01d
--- /dev/null
+++ b/test/Analysis/new-ctor-null-throw.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+
+// These are ill-formed. One cannot return nullptr from a throwing version of an
+// operator new.
+void *operator new(size_t size) {
+  return nullptr;
+}
+void *operator new[](size_t size) {
+  return nullptr;
+}
+
+struct S {
+  int x;
+  S() : x(1) {}
+  ~S() {}
+};
+
+void testArrays() {
+  S *s = new S[10]; // no-crash
+  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}

diff --git a/test/Analysis/new-ctor-null.cpp b/test/Analysis/new-ctor-null.cpp
index 301c72a6c18bcfeac1d932cbb99df625d7f2ad4c..ac2a39a028406477f33fd8e3c339a95d033b2b7b 100644 (file)
--- a/test/Analysis/new-ctor-null.cpp
+++ b/test/Analysis/new-ctor-null.cpp
@@ -1,6 +1,7 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
 
 typedef __typeof__(sizeof(int)) size_t;
 
@@ -13,7 +14,11 @@ void *operator new[](size_t size) throw() {
 
 struct S {
   int x;
-  S() : x(1) {}
+  S() : x(1) {
+    // FIXME: Constructor should not be called with null this, even if it was
+    // returned by operator new().
+    clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  }
   ~S() {}
 };
 
@@ -21,3 +26,11 @@ void testArrays() {
   S *s = new S[10]; // no-crash
   s[0].x = 2; // expected-warning{{Dereference of null pointer}}
 }
+
+int global;
+void testInvalidationOnConstructionIntoNull() {
+  global = 0;
+  S *s = new S();
+  // FIXME: Should be FALSE - we should not invalidate globals.
+  clang_analyzer_eval(global); // expected-warning{{UNKNOWN}}
+}