C++: Remove chi -> load rule from simpleLocalFlowStep and accept tests

Author: MathiasVP

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -229,7 +229,6 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // Flow from an element to an array or union that contains it.
   i2.(ChiInstruction).getPartial() = i1 and
   not i2.isResultConflated() and
-  not exists(PartialDefinitionNode n | n.asInstruction() = i2) and
   exists(Type t | i2.getResultLanguageType().hasType(t, false) |
     t instanceof Union
     or

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

@@ -239,8 +239,6 @@ abstract class PostUpdateNode extends InstructionNode {
 }
 
 /**
- * INTERNAL: do not use.
- *
  * The base class for nodes that perform "partial definitions".
  *
  * In contrast to a normal "definition", which provides a new value for
@@ -253,7 +251,7 @@ abstract class PostUpdateNode extends InstructionNode {
  * setY(&x); // a partial definition of the object `x`.
  * ```
  */
-abstract class PartialDefinitionNode extends PostUpdateNode, TInstructionNode { }
+abstract private class PartialDefinitionNode extends PostUpdateNode, TInstructionNode { }
 
 private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
   override ChiInstruction instr;
@@ -272,17 +270,6 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
   override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
 }
 
-private class FieldStoreWriteSideEffectNode extends PartialDefinitionNode {
-  override ChiInstruction instr;
-
-  FieldStoreWriteSideEffectNode() {
-    not instr.isResultConflated() and
-    exists(WriteSideEffectInstruction sideEffect | instr.getPartial() = sideEffect)
-  }
-
-  override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
-}
-
 /**
  * Not every store instruction generates a chi instruction that we can attach a PostUpdateNode to.
  * For instance, an update to a field of a struct containing only one field. For these cases we
@@ -434,32 +421,6 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { simpleLocalFlowStep(nodeFr
  */
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   simpleInstructionLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
-  or
-  // The next two rules allow flow from partial definitions in setters to succeeding loads in the caller.
-  // First, we add flow from write side-effects to non-conflated chi instructions through their
-  // partial operands. Consider the following example:
-  // ```
-  // void setX(Point* p, int new_x) {
-  //   p->x = new_x;
-  // }
-  // ...
-  // setX(&p, taint());
-  // ```
-  // Here, a `WriteSideEffectInstruction` will provide a new definition for `p->x` after the call to
-  // `setX`, which will be melded into `p` through a chi instruction.
-  nodeTo instanceof FieldStoreWriteSideEffectNode and
-  exists(ChiInstruction chi | chi = nodeTo.asInstruction() |
-    chi.getPartialOperand().getDef() = nodeFrom.asInstruction().(WriteSideEffectInstruction) and
-    not chi.isResultConflated()
-  )
-  or
-  // Next, we add flow from non-conflated chi instructions to loads (even when they are not precise).
-  // This ensures that loads of `p->x` gets data flow from the `WriteSideEffectInstruction` above.
-  nodeFrom instanceof FieldStoreWriteSideEffectNode and
-  exists(ChiInstruction chi | chi = nodeFrom.asInstruction() |
-    not chi.isResultConflated() and
-    nodeTo.asInstruction().(LoadInstruction).getSourceValueOperand().getAnyDef() = chi
-  )
 }
 
 pragma[noinline]
@@ -497,6 +458,23 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // for now.
   iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
   or
+  // Add flow from write side-effects to non-conflated chi instructions through their
+  // partial operands. From there, a `readStep` will find subsequent reads of that field.
+  // Consider the following example:
+  // ```
+  // void setX(Point* p, int new_x) {
+  //   p->x = new_x;
+  // }
+  // ...
+  // setX(&p, taint());
+  // ```
+  // Here, a `WriteSideEffectInstruction` will provide a new definition for `p->x` after the call to
+  // `setX`, which will be melded into `p` through a chi instruction.
+  exists(ChiInstruction chi | chi = iTo |
+    chi.getPartialOperand().getDef() = iFrom.(WriteSideEffectInstruction) and
+    not chi.isResultConflated()
+  )
+  or
   // Flow from stores to structs with a single field to a load of that field.
   iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = iFrom and
   exists(int size, Type type |

cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected

@@ -31,7 +31,6 @@ postIsNotPre
 postHasUniquePre
 uniquePostUpdate
 | ref.cpp:83:5:83:17 | Chi | Node has multiple PostUpdateNodes. |
-| ref.cpp:100:34:100:36 | InitializeIndirection | Node has multiple PostUpdateNodes. |
 | ref.cpp:109:5:109:22 | Chi | Node has multiple PostUpdateNodes. |
 postIsInSameCallable
 reverseRead