Merge pull request github#4573 from MathiasVP/interleave-op-instr-field-flow

C++: instruction -> operand field flow

Author: Dave Bartolomeo

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

@@ -228,7 +228,7 @@ private class ArrayContent extends Content, TArrayContent {
 private predicate fieldStoreStepNoChi(Node node1, FieldContent f, PostUpdateNode node2) {
   exists(StoreInstruction store, Class c |
     store = node2.asInstruction() and
-    store.getSourceValue() = node1.asInstruction() and
+    store.getSourceValueOperand() = node1.asOperand() and
     getWrittenField(store, f.(FieldContent).getAField(), c) and
     f.hasOffset(c, _, _)
   )
@@ -253,27 +253,32 @@ private predicate getWrittenField(Instruction instr, Field f, Class c) {
 }
 
 private predicate fieldStoreStepChi(Node node1, FieldContent f, PostUpdateNode node2) {
-  exists(StoreInstruction store, ChiInstruction chi |
-    node1.asInstruction() = store and
+  exists(ChiPartialOperand operand, ChiInstruction chi |
+    chi.getPartialOperand() = operand and
+    node1.asOperand() = operand and
     node2.asInstruction() = chi and
-    chi.getPartial() = store and
     exists(Class c |
       c = chi.getResultType() and
       exists(int startBit, int endBit |
         chi.getUpdatedInterval(startBit, endBit) and
         f.hasOffset(c, startBit, endBit)
       )
       or
-      getWrittenField(store, f.getAField(), c) and
+      getWrittenField(operand.getDef(), f.getAField(), c) and
       f.hasOffset(c, _, _)
     )
   )
 }
 
 private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode node2) {
   a = TArrayContent() and
-  exists(StoreInstruction store |
-    node1.asInstruction() = store and
+  exists(ChiPartialOperand operand, ChiInstruction chi, StoreInstruction store |
+    chi.getPartialOperand() = operand and
+    store = operand.getDef() and
+    node1.asOperand() = operand and
+    // This `ChiInstruction` will always have a non-conflated result because both `ArrayStoreNode`
+    // and `PointerStoreNode` require it in their characteristic predicates.
+    node2.asInstruction() = chi and
     (
       // `x[i] = taint()`
       // This matches the characteristic predicate in `ArrayStoreNode`.
@@ -282,10 +287,7 @@ private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode n
       // `*p = taint()`
       // This matches the characteristic predicate in `PointerStoreNode`.
       store.getDestinationAddress().(CopyValueInstruction).getUnary() instanceof LoadInstruction
-    ) and
-    // This `ChiInstruction` will always have a non-conflated result because both `ArrayStoreNode`
-    // and `PointerStoreNode` require it in their characteristic predicates.
-    node2.asInstruction().(ChiInstruction).getPartial() = store
+    )
   )
 }
 
@@ -306,7 +308,7 @@ predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
 private predicate fieldStoreStepAfterArraySuppression(
   Node node1, FieldContent f, PostUpdateNode node2
 ) {
-  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi, Class c |
+  exists(WriteSideEffectInstruction write, ChiInstruction chi, Class c |
     not chi.isResultConflated() and
     node1.asInstruction() = chi and
     node2.asInstruction() = chi and
@@ -334,17 +336,17 @@ private predicate getLoadedField(LoadInstruction load, Field f, Class c) {
  * `node2`.
  */
 private predicate fieldReadStep(Node node1, FieldContent f, Node node2) {
-  exists(LoadInstruction load |
-    node2.asInstruction() = load and
-    node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
+  exists(LoadOperand operand |
+    node2.asOperand() = operand and
+    node1.asInstruction() = operand.getAnyDef() and
     exists(Class c |
-      c = load.getSourceValueOperand().getAnyDef().getResultType() and
+      c = operand.getAnyDef().getResultType() and
       exists(int startBit, int endBit |
-        load.getSourceValueOperand().getUsedInterval(unbindInt(startBit), unbindInt(endBit)) and
+        operand.getUsedInterval(unbindInt(startBit), unbindInt(endBit)) and
         f.hasOffset(c, startBit, endBit)
       )
       or
-      getLoadedField(load, f.getAField(), c) and
+      getLoadedField(operand.getUse(), f.getAField(), c) and
       f.hasOffset(c, _, _)
     )
   )
@@ -365,7 +367,7 @@ private predicate fieldReadStep(Node node1, FieldContent f, Node node2) {
  */
 predicate suppressArrayRead(Node node1, ArrayContent a, Node node2) {
   a = TArrayContent() and
-  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi |
+  exists(WriteSideEffectInstruction write, ChiInstruction chi |
     node1.asInstruction() = write and
     node2.asInstruction() = chi and
     chi.getPartial() = write and
@@ -386,20 +388,20 @@ private Instruction skipOneCopyValueInstructionRec(CopyValueInstruction copy) {
   result = skipOneCopyValueInstructionRec(copy.getUnary())
 }
 
-private Instruction skipCopyValueInstructions(Instruction instr) {
-  not result instanceof CopyValueInstruction and result = instr
+private Instruction skipCopyValueInstructions(Operand op) {
+  not result instanceof CopyValueInstruction and result = op.getDef()
   or
-  result = skipOneCopyValueInstructionRec(instr)
+  result = skipOneCopyValueInstructionRec(op.getDef())
 }
 
 private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
   a = TArrayContent() and
   // Explicit dereferences such as `*p` or `p[i]` where `p` is a pointer or array.
-  exists(LoadInstruction load, Instruction address |
-    load.getSourceValueOperand().isDefinitionInexact() and
-    node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
-    load = node2.asInstruction() and
-    address = skipCopyValueInstructions(load.getSourceAddress()) and
+  exists(LoadOperand operand, Instruction address |
+    operand.isDefinitionInexact() and
+    node1.asInstruction() = operand.getAnyDef() and
+    operand = node2.asOperand() and
+    address = skipCopyValueInstructions(operand.getAddressOperand()) and
     (
       address instanceof LoadInstruction or
       address instanceof ArrayToPointerConvertInstruction or
@@ -420,18 +422,18 @@ private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
  * use(x);
  * ```
  * the load on `x` in `use(x)` will exactly overlap with its definition (in this case the definition
- * is a `BufferMayWriteSideEffect`). This predicate pops the `ArrayContent` (pushed by the store in `f`)
+ * is a `WriteSideEffect`). This predicate pops the `ArrayContent` (pushed by the store in `f`)
  * from the access path.
  */
 private predicate exactReadStep(Node node1, ArrayContent a, Node node2) {
   a = TArrayContent() and
-  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi |
+  exists(WriteSideEffectInstruction write, ChiInstruction chi |
     not chi.isResultConflated() and
     chi.getPartial() = write and
     node1.asInstruction() = write and
     node2.asInstruction() = chi and
     // To distinquish this case from the `arrayReadStep` case we require that the entire variable was
-    // overwritten by the `BufferMayWriteSideEffectInstruction` (i.e., there is a load that reads the
+    // overwritten by the `WriteSideEffectInstruction` (i.e., there is a load that reads the
     // entire variable).
     exists(LoadInstruction load | load.getSourceValue() = chi)
   )

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

@@ -396,16 +396,16 @@ private FieldAddressInstruction getFieldInstruction(Instruction instr) {
 
 /**
  * The target of a `fieldStoreStepAfterArraySuppression` store step, which is used to convert
- * an `ArrayContent` to a `FieldContent` when the `BufferMayWriteSideEffect` instruction stores
+ * an `ArrayContent` to a `FieldContent` when the `WriteSideEffect` instruction stores
  * into a field. See the QLDoc for `suppressArrayRead` for an example of where such a conversion
  * is inserted.
  */
-private class BufferMayWriteSideEffectFieldStoreQualifierNode extends PartialDefinitionNode {
+private class WriteSideEffectFieldStoreQualifierNode extends PartialDefinitionNode {
   override ChiInstruction instr;
-  BufferMayWriteSideEffectInstruction write;
+  WriteSideEffectInstruction write;
   FieldAddressInstruction field;
 
-  BufferMayWriteSideEffectFieldStoreQualifierNode() {
+  WriteSideEffectFieldStoreQualifierNode() {
     not instr.isResultConflated() and
     instr.getPartial() = write and
     field = getFieldInstruction(write.getDestinationAddress())

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

@@ -121,6 +121,14 @@ postWithInFlow
 | complex.cpp:12:22:12:27 | Chi | PostUpdateNode should not be the target of local flow. |
 | complex.cpp:14:26:14:26 | Chi | PostUpdateNode should not be the target of local flow. |
 | complex.cpp:14:33:14:33 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:22:11:22:17 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:25:7:25:7 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:42:16:42:16 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:43:16:43:16 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:53:12:53:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:54:12:54:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:55:12:55:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:56:12:56:12 | Chi | PostUpdateNode should not be the target of local flow. |
 | constructors.cpp:20:24:20:29 | Chi | PostUpdateNode should not be the target of local flow. |
 | constructors.cpp:21:24:21:29 | Chi | PostUpdateNode should not be the target of local flow. |
 | constructors.cpp:23:28:23:28 | Chi | PostUpdateNode should not be the target of local flow. |