From: Artem Dergachev Date: Wed, 17 Jan 2018 22:51:19 +0000 (+0000) Subject: [analyzer] operator new: Model the cast of returned pointer into object type. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9178f1690a6c8714c8fed0544c1d34ac7e216724;p=clang [analyzer] operator new: Model the cast of returned pointer into object type. According to [basic.stc.dynamic.allocation], the return type of any C++ overloaded operator new() is "void *". However, type of the new-expression "new T()" and the type of "this" during construction of "T" are both "T *". Hence an implicit cast, which is not present in the AST, needs to be performed before the construction. This patch adds such cast in the case when the allocator was indeed inlined. For now, in the case where the allocator was *not* inlined we still use the same symbolic value (which is a pure SymbolicRegion of type "T *") because it is consistent with how we represent the casts and causes less surprise in the checkers after switching to the new behavior. The better approach would be to represent that value as a cast over a SymbolicRegion of type "void *", however we have technical difficulties conjuring such region without any actual expression of type "void *" present in the AST. Differential Revision: https://reviews.llvm.org/D41250 rdar://problem/12180598 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@322777 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index ec2e2e42c6..2c15607c79 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -119,9 +119,17 @@ ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE, if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) { // TODO: Detect when the allocator returns a null pointer. // Constructor shall not be called in this case. - if (const MemRegion *MR = - getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion()) + if (const SubRegion *MR = dyn_cast_or_null( + getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())) { + if (CNE->isArray()) { + // TODO: This code exists only to trigger the suppression for + // array constructors. In fact, we need to call the constructor + // for every allocated element, not just the first one! + return getStoreManager().GetElementZeroRegion( + MR, CNE->getType()->getPointeeType()); + } return MR; + } } } else if (auto *DS = dyn_cast(StmtElem->getStmt())) { if (const auto *Var = dyn_cast(DS->getSingleDecl())) { @@ -473,12 +481,22 @@ void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE, StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx); for (auto I : DstPreCall) 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 + // lack of a better expression in the AST), the conjured return value symbol + // 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, @@ -564,16 +582,20 @@ 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. - const SubRegion *NewReg = - symVal.castAs().getRegionAs(); - QualType ObjTy = CNE->getType()->getAs()->getPointeeType(); - const ElementRegion *EleReg = - getStoreManager().GetElementZeroRegion(NewReg, ObjTy); - State = State->BindExpr(CNE, Pred->getLocationContext(), - loc::MemRegionVal(EleReg)); + if (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) { + const SubRegion *NewReg = + symVal.castAs().getRegionAs(); + QualType ObjTy = CNE->getType()->getAs()->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; } @@ -582,7 +604,6 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred, // 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.) - SVal Result = symVal; if (FD && FD->isReservedGlobalPlacementOperator()) { // Non-array placement new should always return the placement location. SVal PlacementLoc = State->getSVal(CNE->getPlacementArg(0), LCtx); diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 2cd169ee6d..bb05acd1f4 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -277,12 +277,19 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) { state = state->BindExpr(CCE, callerCtx, ThisV); } - if (const CXXNewExpr *CNE = dyn_cast(CE)) { + if (const auto *CNE = dyn_cast(CE)) { // We are currently evaluating a CXXNewAllocator CFGElement. It takes a // while to reach the actual CXXNewExpr element from here, so keep the // region for later use. - state = setCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(), - state->getSVal(CE, callerCtx)); + // Additionally cast the return value of the inlined operator new + // (which is of type 'void *') to the correct object type. + SVal AllocV = state->getSVal(CNE, callerCtx); + AllocV = svalBuilder.evalCast( + AllocV, CNE->getType(), + getContext().getPointerType(getContext().VoidTy)); + + state = + setCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(), AllocV); } } diff --git a/test/Analysis/new-ctor-conservative.cpp b/test/Analysis/new-ctor-conservative.cpp index ce0ad46465..4500e3a253 100644 --- a/test/Analysis/new-ctor-conservative.cpp +++ b/test/Analysis/new-ctor-conservative.cpp @@ -12,3 +12,18 @@ void checkConstructorInlining() { S *s = new S; clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}} } + +void checkNewPOD() { + int *i = new int; + clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}} + int *j = new int(); + clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}} + int *k = new int(5); + 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}} +} diff --git a/test/Analysis/new-ctor-inlined.cpp b/test/Analysis/new-ctor-inlined.cpp index d16b9b1d28..1506bf27f3 100644 --- a/test/Analysis/new-ctor-inlined.cpp +++ b/test/Analysis/new-ctor-inlined.cpp @@ -38,3 +38,18 @@ void checkNestedNew() { Sp *p = new Sp(new Sp(0)); clang_analyzer_eval(p->p->p == 0); // expected-warning{{TRUE}} } + +void checkNewPOD() { + int *i = new int; + clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}} + int *j = new int(); + clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}} + int *k = new int(5); + clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}} +} + +void checkTrivialCopy() { + S s; + S *t = new S(s); // no-crash + clang_analyzer_eval(t->x == 1); // expected-warning{{TRUE}} +}