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

[NoQ]

Summary

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.

[george.karpenkov]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

514 ↗

(On Diff #130224)

auto?

516 ↗

(On Diff #130224)

This is neither here nor there, but for this and many other cases I think we could be considerably more readable by defining helpers State->assumeIsTrue and State->assumeIsFalse (or assumeNonNull, or whatever is most descriptive)

[NoQ]

Forgot to mention that this patch follows the tradition of duplicating code from VisitCXXNewExpr() to VisitCXXNewAllocatorCall() - until we enable c++-allocator-inlining by default and clean away the old mode support from VisitCXXNewExpr() (which would become pretty trivial).

[NoQ]

Use auto.

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

516 ↗

(On Diff #130224)

Dunno, assume(x, true) sounds descriptive enough to me, and also feels more like a natural spoken language.

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

for (auto I : DstPostCall) {
     // is going to be of the same type (C++ object pointer type). Technically
     // this is not correct because the operator new's prototype always says that
     // it returns a 'void *'. So we should change the type of the symbol,
     // and then evaluate the cast over the symbolic pointer from 'void *' to
     // the object pointer type. But without changing the symbol's type it
     // 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;
   getCheckerManager().runCheckersForPostCall(DstPostPostCallCallback,
                                              DstPostValue, *Call, *this);
   for (auto I : DstPostPostCallCallback) {
     getCheckerManager().runCheckersForNewAllocator(
         CNE, getCXXNewAllocatorValue(I->getState(), CNE, LCtx), Dst, I, *this);
void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
 
   if (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
     // Invalidate placement args.
     // FIXME: Once we figure out how we want allocators to work,
     // we should be using the usual pre-/(default-)eval-/post-call checks here.
     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.
+    // /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 (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);
 
   SVal Result = symVal;
 
   if (CNE->isArray()) {
     // FIXME: allocating an array requires simulating the constructors.
     // For now, just return a symbolicated region.

cfe/trunk/test/Analysis/new-ctor-conservative.cpp

-// 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;
   S() : x(1) {}
   ~S() {}
 };
 
 void checkConstructorInlining() {
void checkNewPOD() {
   clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
 }
 
 void checkNewArray() {
   S *s = new S[10];
   // 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);
+}

cfe/trunk/test/Analysis/new-ctor-null-throw.cpp

+// 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}}
+}

cfe/trunk/test/Analysis/new-ctor-null.cpp

 // 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;
 
 void *operator new(size_t size) throw() {
   return nullptr;
 }
 void *operator new[](size_t size) throw() {
   return nullptr;
 }
 
 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() {}
 };
 
 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}}
+}