Hi Jordan,
Thanks for the review and sorry for keeping this simple patch on hold for
such a long time. I'm a bit new to SA Core and was bit confused about the
UnknownVal thing. I think i understand now what you wanted to communicate.
So if my understanding is correct this is what we want to do-
Memory region can be null here in 2 cases here-
1) When it is actually bound to a null value.
2) In case the SVal is Unknown and we try to get the region corresponding to
it.
In the 1st case we can conclude that this is a null region and hence should
not run the destructor but in the second case we cannot be sure if it is
actually bound to a null region and hence should run destructor in this case.
Am i right here?
Handled the same in ProcessDeleteDtor. What we are doing here is if we can
conclude that the SVal is null don't run the destructor.
For all other cases call VisitCXXDestructor so it is handled in the same way
as it is currently being done for object destruction.
Also added a fixme in malloc checker and modified test cases to follow llvm
coding guidelines.
Thanks!
Hi jordan_rose,
http://llvm-reviews.chandlerc.com/D1594
CHANGE SINCE LAST DIFF
http://llvm-reviews.chandlerc.com/D1594?vs=4222&id=4261#toc
Files:
test/Analysis/new.cpp
include/clang/Analysis/CFG.h
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Core/CallEvent.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
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());
}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1790,7 +1790,10 @@
bool MallocChecker::checkUseAfterFree(SymbolRef Sym, CheckerContext &C,
const Stmt *S) const {
- if (isReleased(Sym, C)) {
+ // FIXME: In case of double delete of class instance. The call to destructor
+ // on second delete result in use of memory after free but doesn't correspond
+ // to any stmt. Currently skipping through handle the same.
+ 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/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -569,7 +569,23 @@
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 (!ArgVal.isUnknown() &&
+ 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,
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
