[analyzer] Invalidate placement args; return the pointer given to placement new

The default global placement new just returns the pointer it is given.
Note that other custom 'new' implementations with placement args are not
guaranteed to do this.

In addition, we need to invalidate placement args, since they may be updated by
the allocator function. (Also, right now we don't properly handle the
constructor inside a CXXNewExpr, so we need to invalidate the placement args
just so that callers know something changed!)

This invalidation is not perfect because CallOrObjCMessage doesn't support
CXXNewExpr, and all of our invalidation callbacks expect that if there's no
CallOrObjCMessage, the invalidation is happening manually (e.g. by a direct
assignment) and shouldn't affect checker-specific metadata (like malloc state);
hence the malloc test case in new-fail.cpp. But region values are now
properly invalidated, at least.

The long-term solution to this problem is to rework CallOrObjCMessage into
something more general, rather than the morass of branches it is today.

<rdar://problem/11679031>

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

diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index a14a491333f20d9c9f16451be9ae9e225dc1c1d5..8996669dce027b200f36ec5b734a8ae78cf947ca 100644 (file)
--- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -170,6 +170,16 @@ void ExprEngine::VisitCXXDestructor(const CXXDestructorDecl *DD,
   Bldr.generateNode(PP, Pred, state);
 }
 
+static bool isPointerToConst(const ParmVarDecl *ParamDecl) {
+  // FIXME: Copied from ExprEngineCallAndReturn.cpp
+  QualType PointeeTy = ParamDecl->getOriginalType()->getPointeeType();
+  if (PointeeTy != QualType() && PointeeTy.isConstQualified() &&
+      !PointeeTy->isAnyPointerType() && !PointeeTy->isReferenceType()) {
+    return true;
+  }
+  return false;
+}
+
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
   StmtNodeBuilder Bldr(Pred, Dst, *currentBuilderContext);
@@ -182,18 +192,108 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
   QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType();
   const ElementRegion *EleReg = 
     getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
+  ProgramStateRef State = Pred->getState();
 
   if (CNE->isArray()) {
     // FIXME: allocating an array requires simulating the constructors.
     // For now, just return a symbolicated region.
-    ProgramStateRef state = Pred->getState();
-    state = state->BindExpr(CNE, Pred->getLocationContext(),
+    State = State->BindExpr(CNE, Pred->getLocationContext(),
                             loc::MemRegionVal(EleReg));
-    Bldr.generateNode(CNE, Pred, state);
+    Bldr.generateNode(CNE, Pred, State);
     return;
   }
 
-  // FIXME: Update for AST changes.
+  FunctionDecl *FD = CNE->getOperatorNew();
+  if (FD && FD->isReservedGlobalPlacementOperator()) {
+    // Non-array placement new should always return the placement location.
+    SVal PlacementLoc = State->getSVal(CNE->getPlacementArg(0), LCtx);
+    State = State->BindExpr(CNE, LCtx, PlacementLoc);
+    // FIXME: Once we have proper support for CXXConstructExprs inside
+    // CXXNewExpr, we need to make sure that the constructed object is not
+    // immediately invalidated here. (The placement call should happen before
+    // the constructor call anyway.)
+  }
+
+  // Invalidate placement args.
+
+  // FIXME: This is largely copied from invalidateArguments, because
+  // CallOrObjCMessage is not general enough to handle new-expressions yet.
+  SmallVector<const MemRegion *, 4> RegionsToInvalidate;
+
+  unsigned Index = 0;
+  for (CXXNewExpr::const_arg_iterator I = CNE->placement_arg_begin(),
+                                      E = CNE->placement_arg_end();
+       I != E; ++I) {
+    // Pre-increment the argument index to skip over the implicit size arg.
+    ++Index;
+    if (FD && Index < FD->getNumParams())
+      if (isPointerToConst(FD->getParamDecl(Index)))
+        continue;
+    
+    SVal V = State->getSVal(*I, LCtx);
+    
+    // If we are passing a location wrapped as an integer, unwrap it and
+    // invalidate the values referred by the location.
+    if (nonloc::LocAsInteger *Wrapped = dyn_cast<nonloc::LocAsInteger>(&V))
+      V = Wrapped->getLoc();
+    else if (!isa<Loc>(V))
+      continue;
+    
+    if (const MemRegion *R = V.getAsRegion()) {
+      // Invalidate the value of the variable passed by reference.
+      
+      // Are we dealing with an ElementRegion?  If the element type is
+      // a basic integer type (e.g., char, int) and the underlying region
+      // is a variable region then strip off the ElementRegion.
+      // FIXME: We really need to think about this for the general case
+      //   as sometimes we are reasoning about arrays and other times
+      //   about (char*), etc., is just a form of passing raw bytes.
+      //   e.g., void *p = alloca(); foo((char*)p);
+      if (const ElementRegion *ER = dyn_cast<ElementRegion>(R)) {
+        // Checking for 'integral type' is probably too promiscuous, but
+        // we'll leave it in for now until we have a systematic way of
+        // handling all of these cases.  Eventually we need to come up
+        // with an interface to StoreManager so that this logic can be
+        // appropriately delegated to the respective StoreManagers while
+        // still allowing us to do checker-specific logic (e.g.,
+        // invalidating reference counts), probably via callbacks.
+        if (ER->getElementType()->isIntegralOrEnumerationType()) {
+          const MemRegion *superReg = ER->getSuperRegion();
+          if (isa<VarRegion>(superReg) || isa<FieldRegion>(superReg) ||
+              isa<ObjCIvarRegion>(superReg))
+            R = cast<TypedRegion>(superReg);
+        }
+        // FIXME: What about layers of ElementRegions?
+      }
+      
+      // Mark this region for invalidation.  We batch invalidate regions
+      // below for efficiency.
+      RegionsToInvalidate.push_back(R);
+    } else {
+      // Nuke all other arguments passed by reference.
+      // FIXME: is this necessary or correct? This handles the non-Region
+      //  cases.  Is it ever valid to store to these?
+      State = State->unbindLoc(cast<Loc>(V));
+    }
+  }
+  
+  // Invalidate designated regions using the batch invalidation API.
+  
+  // FIXME: We can have collisions on the conjured symbol if the
+  //  expression *I also creates conjured symbols.  We probably want
+  //  to identify conjured symbols by an expression pair: the enclosing
+  //  expression (the context) and the expression itself.  This should
+  //  disambiguate conjured symbols.
+  unsigned Count = currentBuilderContext->getCurrentBlockCount();
+  
+  // NOTE: Even if RegionsToInvalidate is empty, we may still invalidate
+  //  global variables.
+  State = State->invalidateRegions(RegionsToInvalidate, CNE, Count, LCtx);
+  Bldr.generateNode(CNE, Pred, State);
+  return;
+
+  // FIXME: The below code is long-since dead. However, constructor handling
+  // in new-expressions is far from complete. See PR12014 for more details.
 #if 0
   // Evaluate constructor arguments.
   const FunctionProtoType *FnType = NULL;

diff --git a/test/Analysis/new-fail.cpp b/test/Analysis/new-fail.cpp
new file mode 100644 (file)
index 0000000..8b1903f
--- /dev/null
+++ b/test/Analysis/new-fail.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store region -verify %s
+// XFAIL: *
+
+void f1() {
+  int *n = new int;
+  if (*n) { // expected-warning {{Branch condition evaluates to a garbage value}}
+  }
+}
+
+void f2() {
+  int *n = new int(3);
+  if (*n) { // no-warning
+  }
+}
+
+void *operator new(size_t, void *, void *);
+void *testCustomNew() {
+  int *x = (int *)malloc(sizeof(int));  
+  void *y = new (0, x) int;  
+  return y; // no-warning (placement new could have freed x)
+}

diff --git a/test/Analysis/new.cpp b/test/Analysis/new.cpp
index 5ca8c462bdf53ec3f1d1b739b777e8d08c0aa84c..3cf5b0f8cc9fcd88bd28fac445f9564335df583a 100644 (file)
--- a/test/Analysis/new.cpp
+++ b/test/Analysis/new.cpp
@@ -1,15 +1,36 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-store region -verify %s
-// XFAIL: *
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-store region -verify %s
 
-void f1() {
-  int *n = new int;
-  if (*n) { // expected-warning {{Branch condition evaluates to a garbage value}}
-  }
+void clang_analyzer_eval(bool);
+
+typedef typeof(sizeof(int)) size_t;
+extern "C" void *malloc(size_t);
+
+// This is the standard placement new.
+inline void* operator new(size_t, void* __p) throw()
+{
+  return __p;
+}
+
+void *testPlacementNew() {
+  int *x = (int *)malloc(sizeof(int));
+  *x = 1;
+  clang_analyzer_eval(*x == 1); // expected-warning{{TRUE}};
+
+  void *y = new (x) int;
+  clang_analyzer_eval(x == y); // expected-warning{{TRUE}};
+  clang_analyzer_eval(*x == 1); // expected-warning{{UNKNOWN}};
+
+  return y;
 }
 
-void f2() {
-  int *n = new int(3);
-  if (*n) { // no-warning
-  }
+void *operator new(size_t, size_t, int *);
+void *testCustomNew() {
+  int x[1] = {1};
+  clang_analyzer_eval(*x == 1); // expected-warning{{TRUE}};
+
+  void *y = new (0, x) int;
+  clang_analyzer_eval(*x == 1); // expected-warning{{UNKNOWN}};
+
+  return y; // no-warning
 }