Hi Jordan, Anna,
Implemented comments from Jordan.
I think the FIXME comments were a bit misleading. We still do report use
after free in this case but at the 1st visible Stmt. We are just skipping the
implicit call to destructor in case of delete as there was no Stmt
corresponding to it.
To make it more clear i have added 2 TC's.
For e.g in testDoubleDeleteClassInstance we still report use of memory after
free but it is inside the destructor.
I do agree here that the destructor should not be modeled when we call delete
the second time but as Jordan highlighted currently we do not have a Stmt
corresponding to call to destructor in case of delete.
I'm a bit reserved about calling ReportUseAfterFree with default Range in
this case as it would report the warning at incorrect location making it
difficult to track the exact point of problem.
I will try to investigate as Anna suggested if it is possible to add stmt
info to destructor in these cases. I think it would simply be same as the
deleteExpr which triggered this call. But i would like to address that in a
separate revision.
For now do you think it is ok if we go ahead with commit? I will try to
address the issues raised by Anna in next revision ( added a fixme in test case
for the same).
Thanks
Karthik Bhat
Hi jordan_rose, zaks.anna,
http://llvm-reviews.chandlerc.com/D1594
CHANGE SINCE LAST DIFF
http://llvm-reviews.chandlerc.com/D1594?vs=4261&id=4320#toc
Files:
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Core/CallEvent.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/NewDelete-checker-test.cpp
test/Analysis/new.cpp
include/clang/Analysis/CFG.h
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1790,7 +1790,8 @@
bool MallocChecker::checkUseAfterFree(SymbolRef Sym, CheckerContext &C,
const Stmt *S) const {
- if (isReleased(Sym, C)) {
+ // FIXME: Handle destructor called from delete more precisely.
+ if (isReleased(Sym, C) && S) {
ReportUseAfterFree(C, S->getSourceRange(), Sym);
return true;
}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -957,6 +957,8 @@
const Stmt *Trigger;
if (Optional<CFGAutomaticObjDtor> AutoDtor = E.getAs<CFGAutomaticObjDtor>())
Trigger = AutoDtor->getTriggerStmt();
+ else if (Optional<CFGDeleteDtor> DeleteDtor = E.getAs<CFGDeleteDtor>())
+ Trigger = cast<Stmt>(DeleteDtor->getDeleteExpr());
else
Trigger = Dtor->getBody();
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -296,7 +296,9 @@
// FIXME: We need to run the same destructor on every element of the array.
// This workaround will just run the first destructor (which will still
// invalidate the entire array).
- SVal DestVal = loc::MemRegionVal(Dest);
+ SVal DestVal = UnknownVal();
+ if (Dest)
+ DestVal = loc::MemRegionVal(Dest);
DestVal = makeZeroElementRegion(State, DestVal, ObjectType);
Dest = DestVal.getAsRegion();
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -569,7 +569,22 @@
void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
ExplodedNode *Pred,
ExplodedNodeSet &Dst) {
- //TODO: Handle DeleteDtor
+ ProgramStateRef State = Pred->getState();
+ const LocationContext *LCtx = Pred->getLocationContext();
+ const CXXDeleteExpr *DE = Dtor.getDeleteExpr();
+ const Stmt *Arg = cast<Stmt>(DE->getArgument());
+ SVal ArgVal = Pred->getState()->getSVal(Arg, LCtx);
+ // If it is known to be bound to a null value
+ // dont run destructor.
+ if (State->isNull(ArgVal).isConstrainedTrue()) {
+ StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+ Bldr.generateNode(DE, Pred, State);
+ return;
+ }
+ VisitCXXDestructor(DE->getDestroyedType(),
+ ArgVal.getAsRegion(),
+ DE, /*IsBase=*/ false,
+ Pred, Dst);
}
void ExprEngine::ProcessBaseDtor(const CFGBaseDtor D,
Index: test/Analysis/NewDelete-checker-test.cpp
===================================================================
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -333,3 +333,29 @@
}
}
+// Test double delete
+class DerefClass{
+public:
+ int *x;
+ DerefClass() {}
+ ~DerefClass() {*x = 1;} //expected-warning {{Use of memory after it is freed}}
+};
+
+void testDoubleDeleteClassInstance() {
+ DerefClass *foo = new DerefClass();
+ delete foo;
+ delete foo; // FIXME: We should ideally report warning here instead of inside the destructor.
+}
+
+class EmptyClass{
+public:
+ EmptyClass() {}
+ ~EmptyClass() {}
+};
+
+void testDoubleDeleteEmptyClass() {
+ EmptyClass *foo = new EmptyClass();
+ delete foo;
+ delete foo; //expected-warning {{Attempt to free released memory}}
+}
+
Index: test/Analysis/new.cpp
===================================================================
--- test/Analysis/new.cpp
+++ test/Analysis/new.cpp
@@ -206,3 +206,135 @@
}
return 1;
}
+
+// Test modelling destructor call on call to delete
+class IntPair{
+public:
+ int x;
+ int y;
+ IntPair() {};
+ ~IntPair() {x = x/y;}; //expected-warning {{Division by zero}}
+};
+
+void testCallToDestructor() {
+ IntPair *b = new IntPair();
+ b->x = 1;
+ b->y = 0;
+ delete b; // This results in divide by zero in destructor
+}
+
+// Test Deleting a value that's passed as an argument.
+class DerefClass{
+public:
+ int *x;
+ DerefClass() {};
+ ~DerefClass() {*x = 1;}; //expected-warning {{Dereference of null pointer (loaded from field 'x')}}
+};
+
+void testDestCall(DerefClass *arg) {
+ delete arg;
+}
+
+void test_delete_dtor_Arg() {
+ DerefClass *pair = new DerefClass();
+ pair->x = 0;
+ testDestCall(pair);
+}
+
+//Deleting the address of a local variable, null pointer
+void abort(void) __attribute__((noreturn));
+
+class NoReturnDtor {
+public:
+ NoReturnDtor() {}
+ ~NoReturnDtor() {abort();}
+};
+
+void test_delete_dtor_LocalVar() {
+ NoReturnDtor test;
+ delete &test; // no warn or crash
+}
+
+class DerivedNoReturn:public NoReturnDtor {
+public:
+ DerivedNoReturn() {};
+ ~DerivedNoReturn() {};
+};
+
+void testNullDtorDerived() {
+ DerivedNoReturn *p = new DerivedNoReturn();
+ delete p; // Calls the base destructor which aborts, checked below
+ clang_analyzer_eval(true); // no warn
+}
+
+//Deleting a non class pointer should not crash/warn
+void test_var_delete() {
+ int *v = new int;
+ delete v; // no crash/warn
+ clang_analyzer_eval(true); // expected-warning{{TRUE}}
+}
+
+void testDeleteNull() {
+ NoReturnDtor *foo = 0;
+ delete foo; // should not call destructor, checked below
+ clang_analyzer_eval(true); // expected-warning{{TRUE}}
+}
+
+void testNullAssigneddtor() {
+ NoReturnDtor *p = 0;
+ NoReturnDtor *s = p;
+ delete s; // should not call destructor, checked below
+ clang_analyzer_eval(true); // expected-warning{{TRUE}}
+}
+
+void deleteArg(NoReturnDtor *test) {
+ delete test;
+}
+
+void testNulldtorArg() {
+ NoReturnDtor *p = 0;
+ deleteArg(p);
+ clang_analyzer_eval(true); // expected-warning{{TRUE}}
+}
+
+void testDeleteUnknown(NoReturnDtor *foo) {
+ delete foo; // should assume non-null and call noreturn destructor
+ clang_analyzer_eval(true); // no-warning
+}
+
+// Invalidate Region even in case of default destructor
+class InvalidateDestTest {
+public:
+ int x;
+ int *y;
+ ~InvalidateDestTest();
+};
+
+int test_member_invalidation() {
+
+ //test invalidation of member variable
+ InvalidateDestTest *test = new InvalidateDestTest();
+ test->x = 5;
+ int *k = &(test->x);
+ clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
+ delete test;
+ clang_analyzer_eval(*k == 5); // expected-warning{{UNKNOWN}}
+
+ //test invalidation of member pointer
+ int localVar = 5;
+ test = new InvalidateDestTest();
+ test->y = &localVar;
+ delete test;
+ clang_analyzer_eval(localVar == 5); // expected-warning{{UNKNOWN}}
+
+ // Test aray elements are invalidated.
+ int Var1 = 5;
+ int Var2 = 5;
+ InvalidateDestTest *a = new InvalidateDestTest[2];
+ a[0].y = &Var1;
+ a[1].y = &Var2;
+ delete[] a;
+ clang_analyzer_eval(Var1 == 5); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(Var2 == 5); // expected-warning{{UNKNOWN}}
+ return 0;
+}
Index: include/clang/Analysis/CFG.h
===================================================================
--- include/clang/Analysis/CFG.h
+++ include/clang/Analysis/CFG.h
@@ -200,7 +200,7 @@
}
// Get Delete expression which triggered the destructor call.
- const CXXDeleteExpr *getDeleteExpr() {
+ const CXXDeleteExpr *getDeleteExpr() const {
return static_cast<CXXDeleteExpr *>(Data2.getPointer());
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
