C++: Don't rely on reverse reads to obtain nested store steps, as this

creates bad performance.
In the AST dataflow library the 'final' reverse read step will read from
the beginning of a chain of field dereferences (i.e., the x in x.y.z =
source()). Typically, this start node will not be a source in the
localFlowStep relation.
This is different in the IR, where the 'final' reverse read step (up
until this commit) was the total chi operand, and this total chi operand
will typically be the source of several simpleLocalFlowStep results.
This can create pathological cases where a node can both end a big step
in the dataflow library, but also be part of a _bigger_ big step. This
caused a huge performance regression on openjdk.

This commit avoids reverse reads, and instead creates explicit store
steps for nested field dereferences. We compute the final step from the
FieldNode to the total chi operand as a simpleFlowStep instead, thus
avoiding the performance problem.

Lesson learned: Be sure that the target of your readStep isn't also a
source in your localFlowStep relation.

Author: MathiasVP

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

@@ -284,6 +284,16 @@ private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode n
   )
 }
 
+/**
+ * A store step from one `FieldNode` that is used in a `storeStep` to another `FieldNode`.
+ * For instance in `a.b.c = source();`, where they will be a `nestedFieldStore` from
+ * the `FieldNode` of `c` to the `FieldNode` of `b`.
+ */
+predicate nestedFieldStore(PostUpdateNode node1, FieldContent f, FieldNode node2) {
+  node2 = node1.getPreUpdateNode() and
+  f.getADirectField() = node2.getField()
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via an assignment to `f`.
  * Thus, `node2` references an object with a field `f` that contains the
@@ -293,7 +303,8 @@ predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
   instrToFieldNodeStoreStepNoChi(node1, f, node2) or
   instrToFieldNodeStoreStepChi(node1, f, node2) or
   arrayStoreStepChi(node1, f, node2) or
-  fieldStoreStepAfterArraySuppression(node1, f, node2)
+  fieldStoreStepAfterArraySuppression(node1, f, node2) or
+  nestedFieldStore(node1, f, node2)
 }
 
 /**
@@ -396,22 +407,12 @@ private predicate exactReadStep(Node node1, ArrayContent a, Node node2) {
 }
 
 /**
- * Given two nodes `node1` and `node2`, the shared dataflow library infers a store step
- * from `node1` to `node2` if there is a read step from the the pre update node of `node2` to the
- * the pre update node of `node1`. This ensures that a field assignment such as
- * ```cpp
- * x.y.z = source();
- * ```
- * generates the access path `[z, y, x]`. This predicate ensures that there's a read step from
- * `x` (the pre update node of `y`) to `y` (the pre update node of `z`), which means that the
- * shared dataflow library will infer a store step from `z` to `y`.
+ * A read step that reads from a nested `FieldNode` in cases such as
+ * `x = a.b.c;`. In this case, there is a `nestedFieldReap` read step from the `FieldNode` of `b`
+ * to the `FieldNode` of `c`.
  */
-private predicate reverseReadStep(Node node1, FieldContent f, FieldNode node2) {
-  (
-    node1 = node2.getObjectNode()
-    or
-    not exists(node2.getObjectNode()) and node1 = node2.getRootNode()
-  ) and
+private predicate nestedFieldRead(Node node1, FieldContent f, FieldNode node2) {
+  node1 = node2.getObjectNode() and
   f.getADirectField() = node2.getField()
 }
 
@@ -455,7 +456,7 @@ predicate readStep(Node node1, Content f, Node node2) {
   arrayReadStep(node1, f, node2) or
   exactReadStep(node1, f, node2) or
   suppressArrayRead(node1, f, node2) or
-  reverseReadStep(node1, f, node2) or
+  nestedFieldRead(node1, f, node2) or
   instrToFieldNodeReadStep(node1, f, node2)
 }