Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -642,8 +642,8 @@ const CXXConstructExpr *findDirectConstructorForCurrentCFGElement(); /// For a CXXConstructExpr, walk forward in the current CFG block to find the - /// CFGElement for the DeclStmt or CXXInitCtorInitializer for which is - /// directly constructed by this constructor. Returns None if the current + /// CFGElement for the DeclStmt or CXXInitCtorInitializer or CXXNewExpr which + /// is directly constructed by this constructor. Returns None if the current /// constructor expression did not directly construct into an existing /// region. Optional findElementDirectlyInitializedByCurrentConstructor(); @@ -655,6 +655,30 @@ /// if not. const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE, ExplodedNode *Pred); + + /// Store the region returned by operator new() so that the constructor + /// that follows it knew what location to initialize. The value should be + /// cleared once the respective CXXNewExpr CFGStmt element is processed. + static ProgramStateRef + setCXXNewAllocatorValue(ProgramStateRef State, const CXXNewExpr *CNE, + const LocationContext *CallerLC, SVal V); + + /// Retrieve the location returned by the current operator new(). + static SVal + getCXXNewAllocatorValue(ProgramStateRef State, const CXXNewExpr *CNE, + const LocationContext *CallerLC); + + /// Clear the location returned by the respective operator new(). This needs + /// to be done as soon as CXXNewExpr CFG block is evaluated. + static ProgramStateRef + clearCXXNewAllocatorValue(ProgramStateRef State, const CXXNewExpr *CNE, + const LocationContext *CallerLC); + + /// Check if all allocator values are clear for the given context range + /// (including FromLC, not including ToLC). This is useful for assertions. + static bool areCXXNewAllocatorValuesClear(ProgramStateRef State, + const LocationContext *FromLC, + const LocationContext *ToLC); }; /// Traits for storing the call processing policy inside GDM. Index: lib/StaticAnalyzer/Core/ExprEngine.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngine.cpp +++ lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -63,6 +63,20 @@ REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporariesSet, llvm::ImmutableSet) +typedef llvm::ImmutableMap, SVal> + CXXNewAllocatorValuesTy; + +// Keeps track of return values of various operator new() calls between +// evaluation of the inlined operator new(), through the constructor call, +// to the actual evaluation of the CXXNewExpr. +// TODO: Refactor the key for this trait into a LocationContext sub-class, +// which would be put on the stack of location contexts before operator new() +// is evaluated, and removed from the stack when the whole CXXNewExpr +// is fully evaluated. +// Probably do something similar to the previous trait as well. +REGISTER_TRAIT_WITH_PROGRAMSTATE(CXXNewAllocatorValues, CXXNewAllocatorValuesTy) + //===----------------------------------------------------------------------===// // Engine construction and deletion. //===----------------------------------------------------------------------===// @@ -308,6 +322,43 @@ return State; } +ProgramStateRef +ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State, + const CXXNewExpr *CNE, + const LocationContext *CallerLC, SVal V) { + assert(!State->get(std::make_pair(CNE, CallerLC)) && + "Allocator value already set!"); + return State->set(std::make_pair(CNE, CallerLC), V); +} + +SVal ExprEngine::getCXXNewAllocatorValue(ProgramStateRef State, + const CXXNewExpr *CNE, + const LocationContext *CallerLC) { + return *State->get(std::make_pair(CNE, CallerLC)); +} + +ProgramStateRef +ExprEngine::clearCXXNewAllocatorValue(ProgramStateRef State, + const CXXNewExpr *CNE, + const LocationContext *CallerLC) { + return State->remove(std::make_pair(CNE, CallerLC)); +} + +bool ExprEngine::areCXXNewAllocatorValuesClear(ProgramStateRef State, + const LocationContext *FromLC, + const LocationContext *ToLC) { + const LocationContext *LC = FromLC; + do { + for (auto I : State->get()) + if (I.first.second == LC) + return false; + + LC = LC->getParent(); + assert(LC && "ToLC must be a parent of FromLC!"); + } while (LC != ToLC); + return true; +} + //===----------------------------------------------------------------------===// // Top-level transfer function logic (Dispatcher). //===----------------------------------------------------------------------===// @@ -429,6 +480,13 @@ const StackFrameContext *SFC = LC ? LC->getCurrentStackFrame() : nullptr; SymbolReaper SymReaper(SFC, ReferenceStmt, SymMgr, getStoreManager()); + for (auto I : CleanedState->get()) { + if (SymbolRef Sym = I.second.getAsSymbol()) + SymReaper.markLive(Sym); + if (const MemRegion *MR = I.second.getAsRegion()) + SymReaper.markElementIndicesLive(MR); + } + getCheckerManager().runCheckersForLiveSymbols(CleanedState, SymReaper); // Create a state in which dead bindings are removed from the environment Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -115,14 +115,25 @@ if (auto Elem = findElementDirectlyInitializedByCurrentConstructor()) { if (Optional StmtElem = Elem->getAs()) { - auto *DS = cast(StmtElem->getStmt()); - if (const auto *Var = dyn_cast(DS->getSingleDecl())) { - if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) { - SVal LValue = State->getLValue(Var, LCtx); - QualType Ty = Var->getType(); - LValue = makeZeroElementRegion(State, LValue, Ty); - return LValue.getAsRegion(); + if (const CXXNewExpr *CNE = dyn_cast(StmtElem->getStmt())) { + 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()) + return MR; } + } else if (auto *DS = dyn_cast(StmtElem->getStmt())) { + if (const auto *Var = dyn_cast(DS->getSingleDecl())) { + if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) { + SVal LValue = State->getLValue(Var, LCtx); + QualType Ty = Var->getType(); + LValue = makeZeroElementRegion(State, LValue, Ty); + return LValue.getAsRegion(); + } + } + } else { + llvm_unreachable("Unexpected directly initialized element!"); } } else if (Optional InitElem = Elem->getAs()) { const CXXCtorInitializer *Init = InitElem->getInitializer(); @@ -166,6 +177,9 @@ if (isa(StmtElem->getStmt())) { return true; } + if (isa(StmtElem->getStmt())) { + return true; + } } if (Elem.getKind() == CFGElement::Initializer) { @@ -455,12 +469,23 @@ getCheckerManager().runCheckersForPreCall(DstPreCall, Pred, *Call, *this); - ExplodedNodeSet DstInvalidated; - StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx); - for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end(); - I != E; ++I) - defaultEvalCall(Bldr, *I, *Call); - getCheckerManager().runCheckersForPostCall(Dst, DstInvalidated, + ExplodedNodeSet DstPostCall; + StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx); + for (auto I : DstPreCall) + defaultEvalCall(CallBldr, I, *Call); + + // 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) { + ProgramStateRef State = I->getState(); + ValueBldr.generateNode( + CNE, I, + setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx))); + } + + getCheckerManager().runCheckersForPostCall(Dst, DstPostValue, *Call, *this); } @@ -474,7 +499,7 @@ unsigned blockCount = currBldrCtx->blockCount(); const LocationContext *LCtx = Pred->getLocationContext(); - DefinedOrUnknownSVal symVal = UnknownVal(); + SVal symVal = UnknownVal(); FunctionDecl *FD = CNE->getOperatorNew(); bool IsStandardGlobalOpNewFunction = false; @@ -490,26 +515,37 @@ IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1); } + 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); + } + // We assume all standard global 'operator new' functions allocate memory in // heap. We realize this is an approximation that might not correctly model // a custom global allocator. - if (IsStandardGlobalOpNewFunction) - symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount); - else - symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(), - blockCount); + if (symVal.isUnknown()) { + if (IsStandardGlobalOpNewFunction) + symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount); + else + symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(), + blockCount); + } - ProgramStateRef State = Pred->getState(); CallEventManager &CEMgr = getStateManager().getCallEventManager(); CallEventRef Call = CEMgr.getCXXAllocatorCall(CNE, State, LCtx); - // 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 (!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 @@ -522,7 +558,8 @@ QualType Ty = FD->getType(); if (const FunctionProtoType *ProtoType = Ty->getAs()) if (!ProtoType->isNothrow(getContext())) - State = State->assume(symVal, true); + if (auto dSymVal = symVal.getAs()) + State = State->assume(*dSymVal, true); } StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx); Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp =================================================================== --- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -276,6 +276,14 @@ state = state->BindExpr(CCE, callerCtx, ThisV); } + + if (const CXXNewExpr *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)); + } } // Step 3: BindedRetNode -> CleanedNodes @@ -315,6 +323,10 @@ CallExitEnd Loc(calleeCtx, callerCtx); bool isNew; ProgramStateRef CEEState = (*I == CEBNode) ? state : (*I)->getState(); + + // See if we have any stale C++ allocator values. + assert(areCXXNewAllocatorValuesClear(CEEState, calleeCtx, callerCtx)); + ExplodedNode *CEENode = G.getNode(Loc, CEEState, false, &isNew); CEENode->addPredecessor(*I, G); if (!isNew) @@ -596,21 +608,34 @@ const CXXConstructorCall &Ctor = cast(Call); + const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr(); + + // FIXME: ParentMap is slow and ugly. The callee should provide the + // necessary context. Ideally as part of the call event, or maybe as part of + // location context. + const Stmt *ParentExpr = CurLC->getParentMap().getParent(CtorExpr); + + if (ParentExpr && isa(ParentExpr) && + !Opts.mayInlineCXXAllocator()) + return CIP_DisallowedOnce; + // FIXME: We don't handle constructors or destructors for arrays properly. // Even once we do, we still need to be careful about implicitly-generated // initializers for array fields in default move/copy constructors. + // We still allow construction into ElementRegion targets when they don't + // represent array elements. const MemRegion *Target = Ctor.getCXXThisVal().getAsRegion(); - if (Target && isa(Target)) - return CIP_DisallowedOnce; - - // FIXME: This is a hack. We don't use the correct region for a new - // expression, so if we inline the constructor its result will just be - // thrown away. This short-term hack is tracked in - // and the longer-term possible fix is discussed in PR12014. - const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr(); - if (const Stmt *Parent = CurLC->getParentMap().getParent(CtorExpr)) - if (isa(Parent)) - return CIP_DisallowedOnce; + if (Target && isa(Target)) { + if (ParentExpr) + if (const CXXNewExpr *NewExpr = dyn_cast(ParentExpr)) + if (NewExpr->isArray()) + return CIP_DisallowedOnce; + + if (const TypedValueRegion *TR = dyn_cast( + cast(Target)->getSuperRegion())) + if (TR->getValueType()->isArrayType()) + return CIP_DisallowedOnce; + } // Inlining constructors requires including initializers in the CFG. const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext(); @@ -629,7 +654,7 @@ // FIXME: This is a hack. We don't handle temporary destructors // right now, so we shouldn't inline their constructors. if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete) - if (!Target || !isa(Target)) + if (!Target || isa(Target)) return CIP_DisallowedOnce; break; Index: test/Analysis/inline.cpp =================================================================== --- test/Analysis/inline.cpp +++ test/Analysis/inline.cpp @@ -315,17 +315,13 @@ int value; IntWrapper(int input) : value(input) { - // We don't want this constructor to be inlined unless we can actually - // use the proper region for operator new. - // See PR12014 and . - clang_analyzer_checkInlined(false); // no-warning + clang_analyzer_checkInlined(true); // expected-warning{{TRUE}} } }; void test() { IntWrapper *obj = new IntWrapper(42); - // should be TRUE - clang_analyzer_eval(obj->value == 42); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(obj->value == 42); // expected-warning{{TRUE}} delete obj; } @@ -335,8 +331,9 @@ clang_analyzer_eval(alias == obj); // expected-warning{{TRUE}} - // should be TRUE - clang_analyzer_eval(obj->value == 42); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(obj->value == 42); // expected-warning{{TRUE}} + // Because malloc() was never free()d: + // expected-warning@-2{{Potential leak of memory pointed to by 'alias'}} } } Index: test/Analysis/new-ctor-conservative.cpp =================================================================== --- /dev/null +++ test/Analysis/new-ctor-conservative.cpp @@ -0,0 +1,14 @@ +// 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); + +struct S { + int x; + S() : x(1) {} + ~S() {} +}; + +void checkConstructorInlining() { + S *s = new S; + clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}} +} Index: test/Analysis/new-ctor-inlined.cpp =================================================================== --- /dev/null +++ test/Analysis/new-ctor-inlined.cpp @@ -0,0 +1,40 @@ +// 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 *conjure(); +void exit(int); + +void *operator new(size_t size) throw() { + void *x = conjure(); + if (x == 0) + exit(1); + return x; +} + +struct S { + int x; + S() : x(1) {} + ~S() {} +}; + +void checkNewAndConstructorInlining() { + S *s = new S; + // Check that the symbol for 's' is not dying. + clang_analyzer_eval(s != 0); // expected-warning{{TRUE}} + // Check that bindings are correct (and also not dying). + clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}} +} + +struct Sp { + Sp *p; + Sp(Sp *p): p(p) {} + ~Sp() {} +}; + +void checkNestedNew() { + Sp *p = new Sp(new Sp(0)); + clang_analyzer_eval(p->p->p == 0); // expected-warning{{TRUE}} +} Index: test/Analysis/new-ctor-recursive.cpp =================================================================== --- /dev/null +++ test/Analysis/new-ctor-recursive.cpp @@ -0,0 +1,118 @@ +// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s + +void clang_analyzer_eval(bool); +void clang_analyzer_dump(int); + +typedef __typeof__(sizeof(int)) size_t; + +void *conjure(); +void exit(int); + +struct S; + +S *global_s; + +// Recursive operator kinda placement new. +void *operator new(size_t size, S *place); + +enum class ConstructionKind : char { + Garbage, + Recursive +}; + +struct S { +public: + int x; + S(): x(1) {} + S(int y): x(y) {} + + S(ConstructionKind k) { + switch (k) { + case ConstructionKind::Recursive: { // Call one more operator new 'r'ecursively. + S *s = new (nullptr) S(5); + x = s->x + 1; + global_s = s; + return; + } + case ConstructionKind::Garbage: { + // Leaves garbage in 'x'. + } + } + } + ~S() {} +}; + +// Do not try this at home! +void *operator new(size_t size, S *place) { + if (!place) + return new S(); + return place; +} + +void testThatCharConstructorIndeedYieldsGarbage() { + S *s = new S(ConstructionKind::Garbage); + clang_analyzer_eval(s->x == 0); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(s->x == 1); // expected-warning{{UNKNOWN}} + // FIXME: This should warn, but MallocChecker doesn't default-bind regions + // returned by standard operator new to garbage. + s->x += 1; // no-warning + delete s; +} + + +void testChainedOperatorNew() { + S *s; + // * Evaluate standard new. + // * Evaluate constructor S(3). + // * Bind value for standard new. + // * Evaluate our custom new. + // * Evaluate constructor S(Garbage). + // * Bind value for our custom new. + s = new (new S(3)) S(ConstructionKind::Garbage); + clang_analyzer_eval(s->x == 3); // expected-warning{{TRUE}} + // expected-warning@+9{{Potential leak of memory pointed to by 's'}} + + // * Evaluate standard new. + // * Evaluate constructor S(Garbage). + // * Bind value for standard new. + // * Evaluate our custom new. + // * Evaluate constructor S(4). + // * Bind value for our custom new. + s = new (new S(ConstructionKind::Garbage)) S(4); + clang_analyzer_eval(s->x == 4); // expected-warning{{TRUE}} + delete s; + + // -> Enter our custom new (nullptr). + // * Evaluate standard new. + // * Inline constructor S(). + // * Bind value for standard new. + // <- Exit our custom new (nullptr). + // * Evaluate constructor S(Garbage). + // * Bind value for our custom new. + s = new (nullptr) S(ConstructionKind::Garbage); + clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}} + delete s; + + // -> Enter our custom new (nullptr). + // * Evaluate standard new. + // * Inline constructor S(). + // * Bind value for standard new. + // <- Exit our custom new (nullptr). + // -> Enter constructor S(Recursive). + // -> Enter our custom new (nullptr). + // * Evaluate standard new. + // * Inline constructor S(). + // * Bind value for standard new. + // <- Exit our custom new (nullptr). + // * Evaluate constructor S(5). + // * Bind value for our custom new (nullptr). + // * Assign that value to global_s. + // <- Exit constructor S(Recursive). + // * Bind value for our custom new (nullptr). + global_s = nullptr; + s = new (nullptr) S(ConstructionKind::Recursive); + clang_analyzer_eval(global_s); // expected-warning{{TRUE}} + clang_analyzer_eval(global_s->x == 5); // expected-warning{{TRUE}} + clang_analyzer_eval(s->x == 6); // expected-warning{{TRUE}} + delete s; +} Index: test/Analysis/new-ctor-symbolic.cpp =================================================================== --- /dev/null +++ test/Analysis/new-ctor-symbolic.cpp @@ -0,0 +1,33 @@ +// 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_warnOnDeadSymbol(int); + +typedef __typeof__(sizeof(int)) size_t; + +int conjure(); +void exit(int); + +char buffer[1000]; + +// This operator allocates stuff within the buffer. Additionally, it never +// places anything at the beginning of the buffer. +void *operator new(size_t size) { + int i = conjure(); + if (i == 0) + exit(1); + // Let's see if the symbol dies before new-expression is evaluated. + // It shouldn't. + clang_analyzer_warnOnDeadSymbol(i); + return buffer + i; +} + +struct S { +S() {} +~S() {} +}; + +void testIndexLiveness() { + S *s = new S(); + clang_analyzer_eval((void *)s == (void *)buffer); // expected-warning{{FALSE}} +} // expected-warning{{SYMBOL DEAD}}