From 7107cec5035d0ef4d2be14ebed44ef9b76c4c033 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 27 Nov 2018 17:20:14 +0000 Subject: [PATCH 1/4] CPP: Add test cases. --- .../AV Rule 79/AV Rule 79.expected | 2 + .../4.10 Classes/AV Rule 79/DeleteThis.cpp | 41 +++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected index 048d63477d66..a528235f3496 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected @@ -8,6 +8,8 @@ | DeleteThis.cpp:56:3:56:24 | ... = ... | Resource ptr10 is acquired by class MyClass3 but not released anywhere in this class. | | DeleteThis.cpp:58:3:58:24 | ... = ... | Resource ptr12 is acquired by class MyClass3 but not released anywhere in this class. | | DeleteThis.cpp:60:3:60:24 | ... = ... | Resource ptr14 is acquired by class MyClass3 but not released anywhere in this class. | +| DeleteThis.cpp:111:3:111:20 | ... = ... | Resource b is acquired by class MyClass7 but not released anywhere in this class. | +| DeleteThis.cpp:112:3:112:20 | ... = ... | Resource c is acquired by class MyClass7 but not released anywhere in this class. | | ExternalOwners.cpp:49:3:49:20 | ... = ... | Resource a is acquired by class MyScreen but not released anywhere in this class. | | ListDelete.cpp:21:3:21:21 | ... = ... | Resource first is acquired by class MyThingColection but not released anywhere in this class. | | NoDestructor.cpp:23:3:23:20 | ... = ... | Resource n is acquired by class MyClass5 but not released anywhere in this class. | diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp index 2ada90f1d2e9..db3787cded30 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp @@ -82,3 +82,44 @@ class MyClass3 MyClass2 *ptr10, *ptr11, *ptr12, *ptr13, *ptr14, *ptr15; MyClass2 *ptr20; }; + +class MyClass4 +{ +public: + virtual void Release() = 0; +}; + +class MyClass5 : public MyClass4 +{ +public: + void Release() + { + delete this; + } +}; + +class MyClass6 : public MyClass5 +{ +}; + +class MyClass7 +{ +public: + MyClass7() + { + a = new MyClass5(); // GOOD + b = new MyClass5(); // GOOD [FALSE POSITIVE] + c = new MyClass6(); // GOOD [FALSE POSITIVE] + } + + ~MyClass7() + { + a->Release(); + b->Release(); + c->Release(); + } + + MyClass5 *a; + MyClass4 *b; + MyClass4 *c; +}; From 0eb0bf988ebe42652957544b610e7da3e62bd844 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 28 Nov 2018 13:47:34 +0000 Subject: [PATCH 2/4] CPP: Fix for virtual method calls. --- cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql | 10 ++++++++-- .../jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected | 2 -- .../jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp | 4 ++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql index 1b2f40809928..91c18d8b389c 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql @@ -96,13 +96,19 @@ private predicate exprReleases(Expr e, Expr released, string kind) { ) or exists(Function f, int arg | // `e` is a call to a function that releases one of it's parameters, // and `released` is the corresponding argument - e.(FunctionCall).getTarget() = f and + ( + e.(FunctionCall).getTarget() = f or + e.(FunctionCall).getTarget().(MemberFunction).getAnOverridingFunction*() = f + ) and e.(FunctionCall).getArgument(arg) = released and exprReleases(_, exprOrDereference(f.getParameter(arg).getAnAccess()), kind) ) or exists(Function f, ThisExpr innerThis | // `e` is a call to a method that releases `this`, and `released` // is the object that is called - e.(FunctionCall).getTarget() = f and + ( + e.(FunctionCall).getTarget() = f or + e.(FunctionCall).getTarget().(MemberFunction).getAnOverridingFunction*() = f + ) and e.(FunctionCall).getQualifier() = exprOrDereference(released) and innerThis.getEnclosingFunction() = f and exprReleases(_, innerThis, kind) diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected index a528235f3496..048d63477d66 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected @@ -8,8 +8,6 @@ | DeleteThis.cpp:56:3:56:24 | ... = ... | Resource ptr10 is acquired by class MyClass3 but not released anywhere in this class. | | DeleteThis.cpp:58:3:58:24 | ... = ... | Resource ptr12 is acquired by class MyClass3 but not released anywhere in this class. | | DeleteThis.cpp:60:3:60:24 | ... = ... | Resource ptr14 is acquired by class MyClass3 but not released anywhere in this class. | -| DeleteThis.cpp:111:3:111:20 | ... = ... | Resource b is acquired by class MyClass7 but not released anywhere in this class. | -| DeleteThis.cpp:112:3:112:20 | ... = ... | Resource c is acquired by class MyClass7 but not released anywhere in this class. | | ExternalOwners.cpp:49:3:49:20 | ... = ... | Resource a is acquired by class MyScreen but not released anywhere in this class. | | ListDelete.cpp:21:3:21:21 | ... = ... | Resource first is acquired by class MyThingColection but not released anywhere in this class. | | NoDestructor.cpp:23:3:23:20 | ... = ... | Resource n is acquired by class MyClass5 but not released anywhere in this class. | diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp index db3787cded30..49fa8bbd2db5 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp @@ -108,8 +108,8 @@ class MyClass7 MyClass7() { a = new MyClass5(); // GOOD - b = new MyClass5(); // GOOD [FALSE POSITIVE] - c = new MyClass6(); // GOOD [FALSE POSITIVE] + b = new MyClass5(); // GOOD + c = new MyClass6(); // GOOD } ~MyClass7() From dfbccc4bcf52b3f93a7236ecc1d2efebe11d1158 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 3 Dec 2018 11:30:20 +0000 Subject: [PATCH 3/4] CPP: Additional test cases. --- .../AV Rule 79/AV Rule 79.expected | 1 + .../4.10 Classes/AV Rule 79/DeleteThis.cpp | 41 +++++++++++++++---- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected index 048d63477d66..84f1a769b72c 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected @@ -8,6 +8,7 @@ | DeleteThis.cpp:56:3:56:24 | ... = ... | Resource ptr10 is acquired by class MyClass3 but not released anywhere in this class. | | DeleteThis.cpp:58:3:58:24 | ... = ... | Resource ptr12 is acquired by class MyClass3 but not released anywhere in this class. | | DeleteThis.cpp:60:3:60:24 | ... = ... | Resource ptr14 is acquired by class MyClass3 but not released anywhere in this class. | +| DeleteThis.cpp:127:3:127:20 | ... = ... | Resource d is acquired by class MyClass9 but not released anywhere in this class. | | ExternalOwners.cpp:49:3:49:20 | ... = ... | Resource a is acquired by class MyScreen but not released anywhere in this class. | | ListDelete.cpp:21:3:21:21 | ... = ... | Resource first is acquired by class MyThingColection but not released anywhere in this class. | | NoDestructor.cpp:23:3:23:20 | ... = ... | Resource n is acquired by class MyClass5 but not released anywhere in this class. | diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp index 49fa8bbd2db5..fc7ad7de26d3 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/DeleteThis.cpp @@ -102,24 +102,47 @@ class MyClass6 : public MyClass5 { }; -class MyClass7 +class MyClass7 : public MyClass4 { public: - MyClass7() + void Release() + { + // do nothing + } +}; + +class MyClass8 : public MyClass7 +{ +}; + +class MyClass9 +{ +public: + MyClass9() { a = new MyClass5(); // GOOD b = new MyClass5(); // GOOD c = new MyClass6(); // GOOD - } - ~MyClass7() - { - a->Release(); - b->Release(); - c->Release(); + d = new MyClass7(); // BAD + e = new MyClass7(); // BAD [NOT DETECTED] + f = new MyClass8(); // BAD [NOT DETECTED] } + ~MyClass9() + { + a->Release(); // MyClass5::Release() + b->Release(); // MyClass5::Release() + c->Release(); // MyClass5::Release() - MyClass5 *a; + d->Release(); // MyClass7::Release() + e->Release(); // MyClass7::Release() + f->Release(); // MyClass7::Release() + } + MyClass5 *a; MyClass4 *b; MyClass4 *c; + + MyClass7 *d; + MyClass4 *e; + MyClass4 *f; }; From d8c753755796c4525d6ceeb076816c26a684cc18 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 3 Dec 2018 11:42:24 +0000 Subject: [PATCH 4/4] CPP: * -> + --- cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql index 91c18d8b389c..8f04625c508d 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql @@ -98,7 +98,7 @@ private predicate exprReleases(Expr e, Expr released, string kind) { // and `released` is the corresponding argument ( e.(FunctionCall).getTarget() = f or - e.(FunctionCall).getTarget().(MemberFunction).getAnOverridingFunction*() = f + e.(FunctionCall).getTarget().(MemberFunction).getAnOverridingFunction+() = f ) and e.(FunctionCall).getArgument(arg) = released and exprReleases(_, exprOrDereference(f.getParameter(arg).getAnAccess()), kind) @@ -107,7 +107,7 @@ private predicate exprReleases(Expr e, Expr released, string kind) { // is the object that is called ( e.(FunctionCall).getTarget() = f or - e.(FunctionCall).getTarget().(MemberFunction).getAnOverridingFunction*() = f + e.(FunctionCall).getTarget().(MemberFunction).getAnOverridingFunction+() = f ) and e.(FunctionCall).getQualifier() = exprOrDereference(released) and innerThis.getEnclosingFunction() = f and