[analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

[NoQ]

Summary

Default global operator new(), like malloc(), should return heap pointers, which in the analyzer are represented by SymbolicRegions with HeapSpaceRegion as their parent.

In the -analyzer-config c++-allocator-inlining mode, this was broken, and regular SymbolicRegions were returned instead, which have UnknownSpaceRegion as their parent.

This patch fixes this straightforwardly on ExprEngine side. We may want to delegate this job to the checkers though evalCall, but for now this mode doesn't support evalCall for operator new(), and i'm not sure if it'd be used much.

With this patch going on top of previous patches, enabling c++-allocator-inlining by default causes no regressions on tests (causes some improvements though). It doesn't mean it works (we still have callbacks broken, path diagnostic pieces unsupported, and i've just noticed one more void element region crash), just a psychological checkpoint.

[xazax.hun]

In the future, we might want to model the standard placement new and return a symbol with the correct SpaceRegion (i.e.: the space region of the argument).

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

523

The parens are redundant here.

[NoQ]

Rebase.

Remove the redundant cast that is done in c++-allocator-inlining mode when modeling array new. After rebase it started causing two identical element regions top appear on top of each other.

[dcoughlin]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

513

I realize this isn't your code, but could we use FunctionDecl::isReplaceableGlobalAllocationFunction() here?

[NoQ]

Rebase. Address the comment.

In D41266#959763, @NoQ wrote:

Remove the redundant cast that is done in c++-allocator-inlining mode when modeling array new. After rebase it started causing two identical element regions top appear on top of each other.

Remove this workaround because i reverted the change for which it is a workaround in D41250#971888: we no longer add the redundant ElementRegion, so we don't need to defend from double ElementRegions here. I guess it all finally starts to make sense.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

513

Hmm. Totally. That's much better.

[NoQ]

Rebase (fix minor conflict).

[dcoughlin]

LGTM as well.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

CallEventRef<CXXAllocatorCall> Call =
     CEMgr.getCXXAllocatorCall(CNE, State, LCtx);
 
   ExplodedNodeSet DstPreCall;
   getCheckerManager().runCheckersForPreCall(DstPreCall, Pred,
                                             *Call, *this);
 
   ExplodedNodeSet DstPostCall;
   StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
-  for (auto I : DstPreCall)
+  for (auto I : DstPreCall) {
+    // FIXME: Provide evalCall for checkers?
     defaultEvalCall(CallBldr, I, *Call);
+  }
   // If the call is inlined, DstPostCall will be empty and we bail out now.
 
   // Store return value of operator new() for future use, until the actual
   // CXXNewExpr gets processed.
   ExplodedNodeSet DstPostValue;
   StmtNodeBuilder ValueBldr(DstPostCall, DstPostValue, *currBldrCtx);
   for (auto I : DstPostCall) {
     // FIXME: Because CNE serves as the "call site" for the allocator (due to
ValueBldr.generateNode(
         CNE, I,
         setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
   }
 
   getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
                                              *Call, *this);
 }
 
-
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
   // FIXME: Much of this should eventually migrate to CXXAllocatorCall.
   // Also, we need to decide how allocators actually work -- they're not
   // really part of the CXXNewExpr because they happen BEFORE the
   // CXXConstructExpr subexpression. See PR12014 for some discussion.
 
   unsigned blockCount = currBldrCtx->blockCount();
   const LocationContext *LCtx = Pred->getLocationContext();
   SVal symVal = UnknownVal();
   FunctionDecl *FD = CNE->getOperatorNew();
 
-  bool IsStandardGlobalOpNewFunction = false;
-  if (FD && !isa<CXXMethodDecl>(FD) && !FD->isVariadic()) {
-    if (FD->getNumParams() == 2) {
-      QualType T = FD->getParamDecl(1)->getType();
-      if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
-        // NoThrow placement new behaves as a standard new.
-        IsStandardGlobalOpNewFunction = II->getName().equals("nothrow_t");
-    }
-    else
-      // Placement forms are considered non-standard.
-      IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
-  }
+  bool IsStandardGlobalOpNewFunction =
+      FD->isReplaceableGlobalAllocationFunction();
 
   ProgramStateRef State = Pred->getState();
 
   // Retrieve the stored operator new() return value.
   if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
     symVal = getCXXNewAllocatorValue(State, CNE, LCtx);
     State = clearCXXNewAllocatorValue(State, CNE, LCtx);
   }
void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
 
   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.
-    if (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
-      const SubRegion *NewReg =
-          symVal.castAs<loc::MemRegionVal>().getRegionAs<SubRegion>();
+    if (const SubRegion *NewReg =
+            dyn_cast_or_null<SubRegion>(symVal.getAsRegion())) {
       QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType();
       const ElementRegion *EleReg =
           getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
       Result = loc::MemRegionVal(EleReg);
     }
     State = State->BindExpr(CNE, Pred->getLocationContext(), Result);
     Bldr.generateNode(CNE, Pred, State);
     return;

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(&Call)) {
 
     return State->BindExpr(E, LCtx, ThisV);
   }
 
   // Conjure a symbol if the return value is unknown.
   QualType ResultTy = Call.getResultType();
   SValBuilder &SVB = getSValBuilder();
   unsigned Count = currBldrCtx->blockCount();
-  SVal R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+
+  // See if we need to conjure a heap pointer instead of
+  // a regular unknown pointer.
+  bool IsHeapPointer = false;
+  if (const auto *CNE = dyn_cast<CXXNewExpr>(E))
+    if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+      // FIXME: Delegate this to evalCall in MallocChecker?
+      IsHeapPointer = true;
+    }
+
+  SVal R = IsHeapPointer
+               ? SVB.getConjuredHeapSymbolVal(E, LCtx, Count)
+               : SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
   return State->BindExpr(E, LCtx, R);
 }
 
 // Conservatively evaluate call by invalidating regions and binding
 // a conjured return value.
 void ExprEngine::conservativeEvalCall(const CallEvent &Call, NodeBuilder &Bldr,
                                       ExplodedNode *Pred,
                                       ProgramStateRef State) {

test/Analysis/NewDelete-checker-test.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -fblocks -analyzer-config c++-allocator-inlining=true -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef __typeof__(sizeof(int)) size_t;
 extern "C" void *malloc(size_t);
 extern "C" void free (void* ptr);
 int *global;
 
 //------------------

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);
+
+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() {}
+};
+
+void testArrays() {
+  S *s = new S[10]; // no-crash
+  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}