From e38c1c2c449529e60f48e740cb8662e68e5a5330 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Wed, 20 Jun 2012 01:32:01 +0000 Subject: [PATCH] [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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@158784 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 108 +++++++++++++++++++++- test/Analysis/new-fail.cpp | 21 +++++ test/Analysis/new.cpp | 41 ++++++-- 3 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 test/Analysis/new-fail.cpp diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index a14a491333..8996669dce 100644 --- 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()->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 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(&V)) + V = Wrapped->getLoc(); + else if (!isa(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(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(superReg) || isa(superReg) || + isa(superReg)) + R = cast(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(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 index 0000000000..8b1903fbea --- /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 5ca8c462bd..3cf5b0f8cc 100644 --- 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 } -- 2.40.0