Merge pull request github#1766 from aschackmull/java/nested-storestep

Java/C++/C#: Add field flow support for stores in nested fields.

Author: jbj

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

@@ -215,12 +215,14 @@ private module ImplCommon {
 
   /*
    * Calculation of `predicate store(Node node1, Content f, Node node2)`:
-   * There are three cases:
+   * There are four cases:
    * - The base case: A direct local assignment given by `storeStep`.
    * - A call to a method or constructor with two arguments, `arg1` and `arg2`,
-   *   such the call has the side-effect `arg2.f = arg1`.
+   *   such that the call has the side-effect `arg2.f = arg1`.
    * - A call to a method that returns an object in which an argument has been
    *   stored.
+   * - A reverse step through a read when the result of the read has been
+   *   stored into. This handles cases like `x.f1.f2 = y`.
    * `storeViaSideEffect` covers the first two cases, and `storeReturn` covers
    * the third case.
    */
@@ -232,7 +234,8 @@ private module ImplCommon {
   cached
   predicate store(Node node1, Content f, Node node2) {
     storeViaSideEffect(node1, f, node2) or
-    storeReturn(node1, f, node2)
+    storeReturn(node1, f, node2) or
+    read(node2.(PostUpdateNode).getPreUpdateNode(), f, node1.(PostUpdateNode).getPreUpdateNode())
   }
 
   private predicate storeViaSideEffect(Node node1, Content f, PostUpdateNode node2) {

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImplCommon.qll

@@ -215,12 +215,14 @@ private module ImplCommon {
 
   /*
    * Calculation of `predicate store(Node node1, Content f, Node node2)`:
-   * There are three cases:
+   * There are four cases:
    * - The base case: A direct local assignment given by `storeStep`.
    * - A call to a method or constructor with two arguments, `arg1` and `arg2`,
-   *   such the call has the side-effect `arg2.f = arg1`.
+   *   such that the call has the side-effect `arg2.f = arg1`.
    * - A call to a method that returns an object in which an argument has been
    *   stored.
+   * - A reverse step through a read when the result of the read has been
+   *   stored into. This handles cases like `x.f1.f2 = y`.
    * `storeViaSideEffect` covers the first two cases, and `storeReturn` covers
    * the third case.
    */
@@ -232,7 +234,8 @@ private module ImplCommon {
   cached
   predicate store(Node node1, Content f, Node node2) {
     storeViaSideEffect(node1, f, node2) or
-    storeReturn(node1, f, node2)
+    storeReturn(node1, f, node2) or
+    read(node2.(PostUpdateNode).getPreUpdateNode(), f, node1.(PostUpdateNode).getPreUpdateNode())
   }
 
   private predicate storeViaSideEffect(Node node1, Content f, PostUpdateNode node2) {

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImplCommon.qll

@@ -215,12 +215,14 @@ private module ImplCommon {
 
   /*
    * Calculation of `predicate store(Node node1, Content f, Node node2)`:
-   * There are three cases:
+   * There are four cases:
    * - The base case: A direct local assignment given by `storeStep`.
    * - A call to a method or constructor with two arguments, `arg1` and `arg2`,
-   *   such the call has the side-effect `arg2.f = arg1`.
+   *   such that the call has the side-effect `arg2.f = arg1`.
    * - A call to a method that returns an object in which an argument has been
    *   stored.
+   * - A reverse step through a read when the result of the read has been
+   *   stored into. This handles cases like `x.f1.f2 = y`.
    * `storeViaSideEffect` covers the first two cases, and `storeReturn` covers
    * the third case.
    */
@@ -232,7 +234,8 @@ private module ImplCommon {
   cached
   predicate store(Node node1, Content f, Node node2) {
     storeViaSideEffect(node1, f, node2) or
-    storeReturn(node1, f, node2)
+    storeReturn(node1, f, node2) or
+    read(node2.(PostUpdateNode).getPreUpdateNode(), f, node1.(PostUpdateNode).getPreUpdateNode())
   }
 
   private predicate storeViaSideEffect(Node node1, Content f, PostUpdateNode node2) {

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImplCommon.qll

@@ -215,12 +215,14 @@ private module ImplCommon {
 
   /*
    * Calculation of `predicate store(Node node1, Content f, Node node2)`:
-   * There are three cases:
+   * There are four cases:
    * - The base case: A direct local assignment given by `storeStep`.
    * - A call to a method or constructor with two arguments, `arg1` and `arg2`,
-   *   such the call has the side-effect `arg2.f = arg1`.
+   *   such that the call has the side-effect `arg2.f = arg1`.
    * - A call to a method that returns an object in which an argument has been
    *   stored.
+   * - A reverse step through a read when the result of the read has been
+   *   stored into. This handles cases like `x.f1.f2 = y`.
    * `storeViaSideEffect` covers the first two cases, and `storeReturn` covers
    * the third case.
    */
@@ -232,7 +234,8 @@ private module ImplCommon {
   cached
   predicate store(Node node1, Content f, Node node2) {
     storeViaSideEffect(node1, f, node2) or
-    storeReturn(node1, f, node2)
+    storeReturn(node1, f, node2) or
+    read(node2.(PostUpdateNode).getPreUpdateNode(), f, node1.(PostUpdateNode).getPreUpdateNode())
   }
 
   private predicate storeViaSideEffect(Node node1, Content f, PostUpdateNode node2) {

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

@@ -20,13 +20,19 @@ private newtype TNode =
   TInstanceParameterNode(Callable c) { exists(c.getBody()) and not c.isStatic() } or
   TImplicitInstanceAccess(InstanceAccessExt ia) { not ia.isExplicit(_) } or
   TMallocNode(ClassInstanceExpr cie) or
-  TExplicitArgPostCall(Expr e) { explicitInstanceArgument(_, e) or e instanceof Argument } or
-  TImplicitArgPostCall(InstanceAccessExt ia) { implicitInstanceArgument(_, ia) } or
-  TExplicitStoreTarget(Expr e) {
-    exists(FieldAccess fa | instanceFieldAssign(_, fa) and e = fa.getQualifier())
+  TExplicitExprPostUpdate(Expr e) {
+    explicitInstanceArgument(_, e)
+    or
+    e instanceof Argument
+    or
+    exists(FieldAccess fa | fa.getField() instanceof InstanceField and e = fa.getQualifier())
   } or
-  TImplicitStoreTarget(InstanceAccessExt ia) {
-    exists(FieldAccess fa | instanceFieldAssign(_, fa) and ia.isImplicitFieldQualifier(fa))
+  TImplicitExprPostUpdate(InstanceAccessExt ia) {
+    implicitInstanceArgument(_, ia)
+    or
+    exists(FieldAccess fa |
+      fa.getField() instanceof InstanceField and ia.isImplicitFieldQualifier(fa)
+    )
   }
 
 /**
@@ -257,23 +263,13 @@ abstract private class ImplicitPostUpdateNode extends PostUpdateNode {
   override string toString() { result = getPreUpdateNode().toString() + " [post update]" }
 }
 
-private class ExplicitArgPostCall extends ImplicitPostUpdateNode, TExplicitArgPostCall {
-  override Node getPreUpdateNode() { this = TExplicitArgPostCall(result.asExpr()) }
-}
-
-private class ImplicitArgPostCall extends ImplicitPostUpdateNode, TImplicitArgPostCall {
-  override Node getPreUpdateNode() {
-    this = TImplicitArgPostCall(result.(ImplicitInstanceAccess).getInstanceAccess())
-  }
-}
-
-private class ExplicitStoreTarget extends ImplicitPostUpdateNode, TExplicitStoreTarget {
-  override Node getPreUpdateNode() { this = TExplicitStoreTarget(result.asExpr()) }
+private class ExplicitExprPostUpdate extends ImplicitPostUpdateNode, TExplicitExprPostUpdate {
+  override Node getPreUpdateNode() { this = TExplicitExprPostUpdate(result.asExpr()) }
 }
 
-private class ImplicitStoreTarget extends ImplicitPostUpdateNode, TImplicitStoreTarget {
+private class ImplicitExprPostUpdate extends ImplicitPostUpdateNode, TImplicitExprPostUpdate {
   override Node getPreUpdateNode() {
-    this = TImplicitStoreTarget(result.(ImplicitInstanceAccess).getInstanceAccess())
+    this = TImplicitExprPostUpdate(result.(ImplicitInstanceAccess).getInstanceAccess())
   }
 }
 

java/ql/test/library-tests/dataflow/fields/D.java

@@ -0,0 +1,64 @@
+public class D {
+  Box2 boxfield;
+
+  public void f1() {
+    Elem e = new Elem();
+    Box2 b = new Box2(new Box1(null));
+    b.box.elem = e;
+    sinkWrap(b);
+  }
+
+  public void f2() {
+    Elem e = new Elem();
+    Box2 b = new Box2(new Box1(null));
+    b.box.setElem(e);
+    sinkWrap(b);
+  }
+
+  public void f3() {
+    Elem e = new Elem();
+    Box2 b = new Box2(new Box1(null));
+    b.getBox1().elem = e;
+    sinkWrap(b);
+  }
+
+  public void f4() {
+    Elem e = new Elem();
+    Box2 b = new Box2(new Box1(null));
+    b.getBox1().setElem(e);
+    sinkWrap(b);
+  }
+
+  public static void sinkWrap(Box2 b2) {
+    sink(b2.getBox1().getElem());
+  }
+
+  public void f5a() {
+    Elem e = new Elem();
+    boxfield = new Box2(new Box1(null));
+    boxfield.box.elem = e;
+    f5b();
+  }
+
+  private void f5b() {
+    sink(boxfield.box.elem);
+  }
+
+  public static void sink(Object o) { }
+
+  public static class Elem { }
+
+  public static class Box1 {
+    public Elem elem;
+    public Box1(Elem e) { elem = e; }
+    public Elem getElem() { return elem; }
+    public void setElem(Elem e) { elem = e; }
+  }
+
+  public static class Box2 {
+    public Box1 box;
+    public Box2(Box1 b) { box = b; }
+    public Box1 getBox1() { return box; }
+    public void setBox1(Box1 b) { box = b; }
+  }
+}

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

@@ -19,3 +19,8 @@
 | C.java:4:27:4:36 | new Elem(...) | C.java:19:10:19:11 | s2 |
 | C.java:6:28:6:37 | new Elem(...) | C.java:21:10:21:11 | s4 |
 | C.java:14:15:14:24 | new Elem(...) | C.java:20:10:20:11 | s3 |
+| D.java:5:14:5:23 | new Elem(...) | D.java:33:10:33:31 | getElem(...) |
+| D.java:12:14:12:23 | new Elem(...) | D.java:33:10:33:31 | getElem(...) |
+| D.java:19:14:19:23 | new Elem(...) | D.java:33:10:33:31 | getElem(...) |
+| D.java:26:14:26:23 | new Elem(...) | D.java:33:10:33:31 | getElem(...) |
+| D.java:37:14:37:23 | new Elem(...) | D.java:44:10:44:26 | boxfield.box.elem |