Merge pull request #1900 from jbj/dataflow-this-by-ref

C++: Fix flow out of `this` by reference

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

@@ -19,6 +19,12 @@ private newtype TNode =
   TInstanceParameterNode(MemberFunction f) { exists(f.getBlock()) and not f.isStatic() } or
   TPreConstructorInitThis(ConstructorFieldInit cfi) or
   TPostConstructorInitThis(ConstructorFieldInit cfi) or
+  TThisArgumentPostUpdate(ThisExpr ta) {
+    exists(Call c, int i |
+      ta = c.getArgument(i) and
+      not c.getTarget().getParameter(i).getUnderlyingType().(PointerType).getBaseType().isConst()
+    )
+  } or
   TUninitializedNode(LocalVariable v) { not v.hasInitializer() }
 
 /**
@@ -268,7 +274,7 @@ abstract class PostUpdateNode extends Node {
   override Location getLocation() { result = getPreUpdateNode().getLocation() }
 }
 
-class PartialDefinitionNode extends PostUpdateNode, TPartialDefinitionNode {
+private class PartialDefinitionNode extends PostUpdateNode, TPartialDefinitionNode {
   PartialDefinition pd;
 
   PartialDefinitionNode() { this = TPartialDefinitionNode(pd) }
@@ -282,6 +288,16 @@ class PartialDefinitionNode extends PostUpdateNode, TPartialDefinitionNode {
   override string toString() { result = getPreUpdateNode().toString() + " [post update]" }
 }
 
+private class ThisArgumentPostUpdateNode extends PostUpdateNode, TThisArgumentPostUpdate {
+  ThisExpr thisExpr;
+
+  ThisArgumentPostUpdateNode() { this = TThisArgumentPostUpdate(thisExpr) }
+
+  override Node getPreUpdateNode() { result.asExpr() = thisExpr }
+
+  override string toString() { result = "ref arg this" }
+}
+
 /**
  * A node representing the temporary value of an object that was just
  * constructed by a constructor call or an aggregate initializer. This is only

cpp/ql/test/library-tests/dataflow/fields/by_reference.cpp

@@ -0,0 +1,70 @@
+void sink(void *o);
+void *user_input(void);
+
+struct S {
+  void *a;
+
+  /*
+   * Setters
+   */
+
+  friend void nonMemberSetA(struct S *s, void *value) {
+    s->a = value;
+  }
+
+  void setDirectly(void *value) {
+    this->a = value;
+  }
+
+  void setIndirectly(void *value) {
+    this->setDirectly(value);
+  }
+
+  void setThroughNonMember(void *value) {
+    nonMemberSetA(this, value);
+  }
+
+  /*
+   * Getters
+   */
+
+  friend void *nonMemberGetA(const struct S *s) {
+    return s->a;
+  }
+
+  void* getDirectly() const {
+    return this->a;
+  }
+
+  void* getIndirectly() const {
+    return this->getDirectly();
+  }
+
+  void *getThroughNonMember() const {
+    return nonMemberGetA(this);
+  }
+};
+
+void test_setDirectly() {
+  S s;
+  s.setDirectly(user_input());
+  sink(s.getDirectly()); // flow
+}
+
+void test_setIndirectly() {
+  S s;
+  s.setIndirectly(user_input());
+  sink(s.getIndirectly()); // flow
+}
+
+void test_setThroughNonMember() {
+  S s;
+  s.setThroughNonMember(user_input());
+  sink(s.getThroughNonMember()); // flow
+}
+
+void test_nonMemberSetA() {
+  S s;
+  nonMemberSetA(&s, user_input());
+  sink(nonMemberGetA(&s)); // flow [NOT DETECTED due to lack of flow through &]
+}

cpp/ql/test/library-tests/dataflow/fields/flow.expected

@@ -142,6 +142,15 @@ edges
 | aliasing.cpp:92:12:92:21 | call to user_input | aliasing.cpp:92:3:92:23 | ... = ... |
 | aliasing.cpp:93:8:93:8 | w [s, m1] | aliasing.cpp:93:10:93:10 | s [m1] |
 | aliasing.cpp:93:10:93:10 | s [m1] | aliasing.cpp:93:12:93:13 | m1 |
+| by_reference.cpp:50:3:50:3 | s [post update] [a] | by_reference.cpp:51:8:51:8 | s [a] |
+| by_reference.cpp:50:17:50:26 | call to user_input | by_reference.cpp:50:3:50:3 | s [post update] [a] |
+| by_reference.cpp:51:8:51:8 | s [a] | by_reference.cpp:51:10:51:20 | call to getDirectly |
+| by_reference.cpp:56:3:56:3 | s [post update] [a] | by_reference.cpp:57:8:57:8 | s [a] |
+| by_reference.cpp:56:19:56:28 | call to user_input | by_reference.cpp:56:3:56:3 | s [post update] [a] |
+| by_reference.cpp:57:8:57:8 | s [a] | by_reference.cpp:57:10:57:22 | call to getIndirectly |
+| by_reference.cpp:62:3:62:3 | s [post update] [a] | by_reference.cpp:63:8:63:8 | s [a] |
+| by_reference.cpp:62:25:62:34 | call to user_input | by_reference.cpp:62:3:62:3 | s [post update] [a] |
+| by_reference.cpp:63:8:63:8 | s [a] | by_reference.cpp:63:10:63:28 | call to getThroughNonMember |
 | complex.cpp:34:15:34:15 | b [f, a_] | complex.cpp:44:8:44:8 | b [f, a_] |
 | complex.cpp:34:15:34:15 | b [f, b_] | complex.cpp:45:8:45:8 | b [f, b_] |
 | complex.cpp:44:8:44:8 | b [f, a_] | complex.cpp:44:10:44:10 | f [a_] |
@@ -233,6 +242,9 @@ edges
 | aliasing.cpp:30:11:30:12 | m1 | aliasing.cpp:13:10:13:19 | call to user_input | aliasing.cpp:30:11:30:12 | m1 | m1 flows from $@ | aliasing.cpp:13:10:13:19 | call to user_input | call to user_input |
 | aliasing.cpp:62:14:62:15 | m1 | aliasing.cpp:60:11:60:20 | call to user_input | aliasing.cpp:62:14:62:15 | m1 | m1 flows from $@ | aliasing.cpp:60:11:60:20 | call to user_input | call to user_input |
 | aliasing.cpp:93:12:93:13 | m1 | aliasing.cpp:92:12:92:21 | call to user_input | aliasing.cpp:93:12:93:13 | m1 | m1 flows from $@ | aliasing.cpp:92:12:92:21 | call to user_input | call to user_input |
+| by_reference.cpp:51:10:51:20 | call to getDirectly | by_reference.cpp:50:17:50:26 | call to user_input | by_reference.cpp:51:10:51:20 | call to getDirectly | call to getDirectly flows from $@ | by_reference.cpp:50:17:50:26 | call to user_input | call to user_input |
+| by_reference.cpp:57:10:57:22 | call to getIndirectly | by_reference.cpp:56:19:56:28 | call to user_input | by_reference.cpp:57:10:57:22 | call to getIndirectly | call to getIndirectly flows from $@ | by_reference.cpp:56:19:56:28 | call to user_input | call to user_input |
+| by_reference.cpp:63:10:63:28 | call to getThroughNonMember | by_reference.cpp:62:25:62:34 | call to user_input | by_reference.cpp:63:10:63:28 | call to getThroughNonMember | call to getThroughNonMember flows from $@ | by_reference.cpp:62:25:62:34 | call to user_input | call to user_input |
 | complex.cpp:44:12:44:12 | call to a | complex.cpp:55:13:55:22 | call to user_input | complex.cpp:44:12:44:12 | call to a | call to a flows from $@ | complex.cpp:55:13:55:22 | call to user_input | call to user_input |
 | complex.cpp:44:12:44:12 | call to a | complex.cpp:57:13:57:22 | call to user_input | complex.cpp:44:12:44:12 | call to a | call to a flows from $@ | complex.cpp:57:13:57:22 | call to user_input | call to user_input |
 | complex.cpp:45:12:45:12 | call to b | complex.cpp:56:13:56:22 | call to user_input | complex.cpp:45:12:45:12 | call to b | call to b flows from $@ | complex.cpp:56:13:56:22 | call to user_input | call to user_input |