NoQ updated this revision to Diff 127420.
NoQ added a comment.
- Fix pop from empty stack.
- Add recursive operator new tests.
- Disable argument invalidation when the allocator was inlined (needed for
those tests to work)
In https://reviews.llvm.org/D40560#957653, @xazax.hun wrote:
> I think it would be great to have tests for such cases to make it apparent it
> is not trivial to do this only based on the cfg.
> Maybe something like:
>
> A *a = new A(new B, coin ? new C : new D, inlinedCallWithMoreAllocs());
>
>
> Or in case it turns out to be an easy problem to match these from CFG, I
> prefer avoiding the state.
>
> Also having a new expression within an overloaded operator new might be
> interesting.
Yeah, that's an excellent idea, thanks! I totally forgot about it.
Note, however, that neither chaining operator new as `new(new) C(...)` nor
calling new from within the allocator actually makes our stack grow. In the
former case, inner new-expression is fully evaluated before the outer
new-expression even starts. In the latter case, the value for the outer
allocator will only be put on stack after the outer allocator exits, after the
inner new-expression is fully evaluated.
So in order to actually make use of the stack, we need to call operator new
within a constructor that constructs into an outer operator new. In this case
we first evaluate the outer allocator and put its results on the stack, then we
evaluate the constructor and the operator new within it and put another value
on the stack, and only then we pop both values.
See newly added tests for more details.
> Are you sure we need to produce undef vals? Couldn't a checker subscribe to
> the post allocator call and check for the undefined value and generate a sink
> before the constructor call? I am not opposed to using SVals there, just
> curious.
Yep, you're totally right. Now that we understand how these work, we can
actually catch the undefined operator new return value in or just after the
allocator. Other reasons still stand though.
https://reviews.llvm.org/D40560
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/inline.cpp
test/Analysis/new-ctor-conservative.cpp
test/Analysis/new-ctor-inlined.cpp
test/Analysis/new-ctor-recursive.cpp
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-inlined.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-ctor-inlined.cpp
@@ -0,0 +1,29 @@
+// 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}}
+}
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/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 <rdar://problem/12180598>.
- 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: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -276,6 +276,13 @@
state = state->BindExpr(CCE, callerCtx, ThisV);
}
+
+ if (isa<CXXNewExpr>(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 = pushCXXNewAllocatorValue(state, state->getSVal(CE, callerCtx));
+ }
}
// Step 3: BindedRetNode -> CleanedNodes
@@ -596,21 +603,30 @@
const CXXConstructorCall &Ctor = cast<CXXConstructorCall>(Call);
+ const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr();
+ const Stmt *ParentExpr = CurLC->getParentMap().getParent(CtorExpr);
+
+ if (ParentExpr && isa<CXXNewExpr>(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<ElementRegion>(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 <rdar://problem/12180598>
- // 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<CXXNewExpr>(Parent))
- return CIP_DisallowedOnce;
+ if (Target && isa<ElementRegion>(Target)) {
+ if (ParentExpr)
+ if (const CXXNewExpr *NewExpr = dyn_cast<CXXNewExpr>(ParentExpr))
+ if (NewExpr->isArray())
+ return CIP_DisallowedOnce;
+
+ if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(
+ cast<SubRegion>(Target)->getSuperRegion()))
+ if (TR->getValueType()->isArrayType())
+ return CIP_DisallowedOnce;
+ }
// Inlining constructors requires including initializers in the CFG.
const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
@@ -629,7 +645,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<DeclRegion>(Target))
+ if (!Target || isa<CXXTempObjectRegion>(Target))
return CIP_DisallowedOnce;
break;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -106,22 +106,38 @@
const MemRegion *
ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
ExplodedNode *Pred) {
+ // TODO: We can only construct a CXXConstructorCall with a region.
+ // It means that we cannot handle construction into null or garbage pointers.
+ // Construction into null pointers should not occur (eg. new(std::nothrow)).
+ // Construction into undefined pointers needs to be handled by checkers
+ // to ensure that a warning is displayed to the user and that analysis
+ // doesn't explore such paths any further. However, in order to act on such
+ // events, checker still need to receive them, so we need to be able to return
+ // non-regions from here.
const LocationContext *LCtx = Pred->getLocationContext();
ProgramStateRef State = Pred->getState();
// See if we're constructing an existing region by looking at the next
// element in the CFG.
if (auto Elem = findElementDirectlyInitializedByCurrentConstructor()) {
if (Optional<CFGStmt> StmtElem = Elem->getAs<CFGStmt>()) {
- auto *DS = cast<DeclStmt>(StmtElem->getStmt());
- if (const auto *Var = dyn_cast<VarDecl>(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 (isa<CXXNewExpr>(StmtElem->getStmt())) {
+ if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator())
+ if (const MemRegion *MR =
+ peekCXXNewAllocatorValue(State).getAsRegion())
+ return MR;
+ } else if (auto *DS = dyn_cast<DeclStmt>(StmtElem->getStmt())) {
+ if (const auto *Var = dyn_cast<VarDecl>(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<CFGInitializer> InitElem = Elem->getAs<CFGInitializer>()) {
const CXXCtorInitializer *Init = InitElem->getInitializer();
@@ -165,6 +181,9 @@
if (isa<DeclStmt>(StmtElem->getStmt())) {
return true;
}
+ if (isa<CXXNewExpr>(StmtElem->getStmt())) {
+ return true;
+ }
}
if (Elem.getKind() == CFGElement::Initializer) {
@@ -437,12 +456,22 @@
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, pushCXXNewAllocatorValue(State, State->getSVal(CNE, LCtx)));
+ }
+
+ getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
*Call, *this);
}
@@ -456,7 +485,7 @@
unsigned blockCount = currBldrCtx->blockCount();
const LocationContext *LCtx = Pred->getLocationContext();
- DefinedOrUnknownSVal symVal = UnknownVal();
+ SVal symVal = UnknownVal();
FunctionDecl *FD = CNE->getOperatorNew();
bool IsStandardGlobalOpNewFunction = false;
@@ -472,26 +501,37 @@
IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
}
+ ProgramStateRef State = Pred->getState();
+
+ // Retrieve the stored operator new() return value.
+ if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
+ symVal = peekCXXNewAllocatorValue(State);
+ State = popCXXNewAllocatorValue(State);
+ }
+
// 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<CXXAllocatorCall> 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
@@ -504,7 +544,8 @@
QualType Ty = FD->getType();
if (const FunctionProtoType *ProtoType = Ty->getAs<FunctionProtoType>())
if (!ProtoType->isNothrow(getContext()))
- State = State->assume(symVal, true);
+ if (auto dSymVal = symVal.getAs<DefinedOrUnknownSVal>())
+ State = State->assume(*dSymVal, true);
}
StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -62,6 +62,11 @@
// functions do not interfere.
REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporariesSet,
llvm::ImmutableSet<CXXBindTemporaryContext>)
+// 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.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(CXXNewAllocatorValueStack,
+ llvm::ImmutableList<SVal>);
//===----------------------------------------------------------------------===//
// Engine construction and deletion.
@@ -308,6 +313,21 @@
return State;
}
+ProgramStateRef ExprEngine::pushCXXNewAllocatorValue(ProgramStateRef State,
+ SVal V) {
+ return State->add<CXXNewAllocatorValueStack>(V);
+}
+
+SVal ExprEngine::peekCXXNewAllocatorValue(ProgramStateRef State) {
+ return State->get<CXXNewAllocatorValueStack>().getHead();
+}
+
+ProgramStateRef ExprEngine::popCXXNewAllocatorValue(ProgramStateRef State) {
+ CXXNewAllocatorValueStackTy Stack = State->get<CXXNewAllocatorValueStack>();
+ assert(!Stack.isEmpty());
+ return State->set<CXXNewAllocatorValueStack>(Stack.getTail());
+}
+
//===----------------------------------------------------------------------===//
// Top-level transfer function logic (Dispatcher).
//===----------------------------------------------------------------------===//
@@ -429,6 +449,13 @@
const StackFrameContext *SFC = LC ? LC->getCurrentStackFrame() : nullptr;
SymbolReaper SymReaper(SFC, ReferenceStmt, SymMgr, getStoreManager());
+ for (auto I: CleanedState->get<CXXNewAllocatorValueStack>()) {
+ if (SymbolRef Sym = I.getAsSymbol())
+ SymReaper.markLive(Sym);
+ if (const MemRegion *MR = I.getAsRegion())
+ SymReaper.markElementIndicesLive(MR);
+ }
+
getCheckerManager().runCheckersForLiveSymbols(CleanedState, SymReaper);
// Create a state in which dead bindings are removed from the environment
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<CFGElement> findElementDirectlyInitializedByCurrentConstructor();
@@ -655,6 +655,19 @@
/// 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.
+ ProgramStateRef pushCXXNewAllocatorValue(ProgramStateRef State,
+ SVal V);
+
+ /// Retrieve the location returned by the current operator new().
+ SVal peekCXXNewAllocatorValue(ProgramStateRef State);
+
+ /// Clear the location returned by the respective operator new(). This needs
+ /// to be done as soon as CXXNewExpr CFG block is evaluated.
+ ProgramStateRef popCXXNewAllocatorValue(ProgramStateRef State);
};
/// Traits for storing the call processing policy inside GDM.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
