Merge pull request #362 from jbj/return-this-noreturn

C++: Fix "Overloaded assignment does not return 'this'" for non-returning functions

Author: geoffw0

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,7 +83,6 @@ predicate assignOperatorWithWrongType(Operator op, string msg) {
 predicate assignOperatorWithWrongResult(Operator op, string msg) {
   op.hasName("operator=")
   and not returnsDereferenceThis(op)
-  and exists(op.getBlock())
   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."

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp

@@ -112,6 +112,65 @@ class Obj2 {
   int val;
 };
 
+struct Exception {
+  virtual ~Exception();
+};
+
+class AlwaysThrows {
+public:
+  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 (always throws)
+    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.
+  }
+};
+
+class Reachability {
+  Reachability &operator=(Reachability &that) { // GOOD
+    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
+    return returnThisReference();
+  }
+
+  Reachability &operator=(short _val) { // GOOD
+    return *returnThisPointer();
+  }
+
+  static Reachability staticInstance;
+};
+
 int main() {
   Container c;
   c = c;