From 47a548f56429eba3a41b2a401d679238aebafb91 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 24 Oct 2018 15:38:45 +0200 Subject: [PATCH 1/4] C++: FP test for "operator= doesn't return *this" This rule should not apply to functions that never return. --- .../4.10 Classes/AV Rule 82/AV Rule 82.cpp | 22 +++++++++++++++++++ .../AV Rule 82/AV Rule 82.expected | 2 ++ 2 files changed, 24 insertions(+) diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp index 95a609809efe..b6a28c69be0e 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp @@ -112,6 +112,28 @@ class Obj2 { int val; }; +struct Exception { + virtual ~Exception(); +}; + +class AlwaysThrows { +public: + AlwaysThrows &operator=(int _val) { // GOOD [FALSE POSITIVE] + throw Exception(); + // No `return` statement is generated by the C++ front end because it can + // statically see that the end of the function is unreachable. + } + + AlwaysThrows &operator=(int *_val) { // GOOD [FALSE POSITIVE] + int one = 1; + if (one) + throw Exception(); + // A `return` statement is generated by the C++ front end, but the + // control-flow pruning in QL will establish that this is unreachable. + } +}; + + int main() { Container c; c = c; diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index 0981be469c3e..bebbe47d3ea3 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -2,3 +2,5 @@ | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | +| AV Rule 82.cpp:121:17:121:25 | operator= | Assignment operator in class AlwaysThrows does not return a reference to *this. | +| AV Rule 82.cpp:127:17:127:25 | operator= | Assignment operator in class AlwaysThrows does not return a reference to *this. | From 3c6bed4de6a77d9142683cf9a64d2a34df456fd8 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 24 Oct 2018 15:40:01 +0200 Subject: [PATCH 2/4] C++: FP fix for "operator= doesn't return *this" --- cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql | 5 ++++- .../query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp | 4 ++-- .../jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected | 2 -- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql index e48827979400..6afa597782e4 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql @@ -72,7 +72,10 @@ predicate assignOperatorWithWrongType(Operator op, string msg) { predicate assignOperatorWithWrongResult(Operator op, string msg) { op.hasName("operator=") and not returnsDereferenceThis(op) - and exists(op.getBlock()) + // If a function does not have a reachable `ReturnStmt` then either its body + // was not in the snapshot or it was established by the extractor or the CFG + // pruning that the function never returns. + and exists(ReturnStmt ret | ret.getEnclosingFunction() = op and reachable(ret)) and not op.getType() instanceof VoidType and not assignOperatorWithWrongType(op, _) and msg = "Assignment operator in class " + op.getDeclaringType().getName() + " does not return a reference to *this." diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp index b6a28c69be0e..89d633324815 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp @@ -118,13 +118,13 @@ struct Exception { class AlwaysThrows { public: - AlwaysThrows &operator=(int _val) { // GOOD [FALSE POSITIVE] + AlwaysThrows &operator=(int _val) { // GOOD (always throws) throw Exception(); // No `return` statement is generated by the C++ front end because it can // statically see that the end of the function is unreachable. } - AlwaysThrows &operator=(int *_val) { // GOOD [FALSE POSITIVE] + AlwaysThrows &operator=(int *_val) { // GOOD (always throws) int one = 1; if (one) throw Exception(); diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index bebbe47d3ea3..0981be469c3e 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -2,5 +2,3 @@ | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | -| AV Rule 82.cpp:121:17:121:25 | operator= | Assignment operator in class AlwaysThrows does not return a reference to *this. | -| AV Rule 82.cpp:127:17:127:25 | operator= | Assignment operator in class AlwaysThrows does not return a reference to *this. | From d144f0d154c05ba42b09a4af49aa49fc14294fdf Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 25 Oct 2018 09:37:57 +0200 Subject: [PATCH 3/4] C++: Test for unreachable return statement This test shows that the previous fix did not solve the problem where a bad return statement exists but is unreachable. --- .../4.10 Classes/AV Rule 82/AV Rule 82.cpp | 37 +++++++++++++++++++ .../AV Rule 82/AV Rule 82.expected | 3 ++ 2 files changed, 40 insertions(+) diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp index 89d633324815..00197d6f165a 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp @@ -133,6 +133,43 @@ class AlwaysThrows { } }; +class Reachability { + Reachability &operator=(Reachability &that) { // GOOD [FALSE POSITIVE] + int one = 1; + if (one) + return *this; + else + return that; // unreachable + } + + // helper function that always returns a reference to `*this`. + Reachability &returnThisReference() { + int one = 1; + if (one) + return *this; + else + return staticInstance; // unreachable + } + + // helper function that always returns `this`. + Reachability *const returnThisPointer() { + int one = 1; + if (one) + return this; + else + return &staticInstance; // unreachable + } + + Reachability &operator=(int _val) { // GOOD [FALSE POSITIVE] + return returnThisReference(); + } + + Reachability &operator=(short _val) { // GOOD [FALSE POSITIVE] + return *returnThisPointer(); + } + + static Reachability staticInstance; +}; int main() { Container c; diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index 0981be469c3e..e900818c8d9a 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -2,3 +2,6 @@ | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | +| AV Rule 82.cpp:137:17:137:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. | +| AV Rule 82.cpp:163:17:163:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. | +| AV Rule 82.cpp:167:17:167:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. | From 5cbfdd1029462bd84b9a16ceb2289bd5545067dc Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 25 Oct 2018 09:52:57 +0200 Subject: [PATCH 4/4] C++: Cover more cases of returning `*this` --- cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql | 23 ++++++++++++------- .../4.10 Classes/AV Rule 82/AV Rule 82.cpp | 6 ++--- .../AV Rule 82/AV Rule 82.expected | 3 --- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql index 6afa597782e4..9021529c5468 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql @@ -25,7 +25,7 @@ predicate pointerThis(Expr e) { // `f(...)` // (includes `this = ...`, where `=` is overloaded so a `FunctionCall`) exists(FunctionCall fc | fc = e and callOnThis(fc) | - exists(fc.getTarget().getBlock()) implies returnsPointerThis(fc.getTarget()) + returnsPointerThis(fc.getTarget()) ) or // `this = ...` (where `=` is not overloaded, so an `AssignExpr`) @@ -38,22 +38,33 @@ predicate dereferenceThis(Expr e) { // `f(...)` // (includes `*this = ...`, where `=` is overloaded so a `FunctionCall`) exists(FunctionCall fc | fc = e and callOnThis(fc) | - exists(fc.getTarget().getBlock()) implies returnsDereferenceThis(fc.getTarget()) + returnsDereferenceThis(fc.getTarget()) ) or // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`) dereferenceThis(e.(AssignExpr).getLValue()) } +/** + * Holds if all `return` statements in `f` return `this`, possibly indirectly. + * This includes functions whose body is not in the database. + */ predicate returnsPointerThis(Function f) { - forex(ReturnStmt s | s.getEnclosingFunction() = f | + f.getType().getUnspecifiedType() instanceof PointerType and + forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) | // `return this` pointerThis(s.getExpr()) ) } +/** + * Holds if all `return` statements in `f` return a reference to `*this`, + * possibly indirectly. This includes functions whose body is not in the + * database. + */ predicate returnsDereferenceThis(Function f) { - forex(ReturnStmt s | s.getEnclosingFunction() = f | + f.getType().getUnspecifiedType() instanceof ReferenceType and + forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) | // `return *this` dereferenceThis(s.getExpr()) ) @@ -72,10 +83,6 @@ predicate assignOperatorWithWrongType(Operator op, string msg) { predicate assignOperatorWithWrongResult(Operator op, string msg) { op.hasName("operator=") and not returnsDereferenceThis(op) - // If a function does not have a reachable `ReturnStmt` then either its body - // was not in the snapshot or it was established by the extractor or the CFG - // pruning that the function never returns. - and exists(ReturnStmt ret | ret.getEnclosingFunction() = op and reachable(ret)) and not op.getType() instanceof VoidType and not assignOperatorWithWrongType(op, _) and msg = "Assignment operator in class " + op.getDeclaringType().getName() + " does not return a reference to *this." diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp index 00197d6f165a..95d9901836df 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp @@ -134,7 +134,7 @@ class AlwaysThrows { }; class Reachability { - Reachability &operator=(Reachability &that) { // GOOD [FALSE POSITIVE] + Reachability &operator=(Reachability &that) { // GOOD int one = 1; if (one) return *this; @@ -160,11 +160,11 @@ class Reachability { return &staticInstance; // unreachable } - Reachability &operator=(int _val) { // GOOD [FALSE POSITIVE] + Reachability &operator=(int _val) { // GOOD return returnThisReference(); } - Reachability &operator=(short _val) { // GOOD [FALSE POSITIVE] + Reachability &operator=(short _val) { // GOOD return *returnThisPointer(); } diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index e900818c8d9a..0981be469c3e 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -2,6 +2,3 @@ | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | -| AV Rule 82.cpp:137:17:137:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. | -| AV Rule 82.cpp:163:17:163:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. | -| AV Rule 82.cpp:167:17:167:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. |