C++: Replace SkippableInstruction with local flow steps.

Author: MathiasVP

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

@@ -346,24 +346,6 @@ private class ArrayToPointerConvertInstruction extends ConvertInstruction {
   }
 }
 
-/**
- * These two predicates look like copy-paste from the two predicates with the same name in DataFlowUtil,
- * but crucially they only skip past `CopyValueInstruction`s. This is because we use a special case of
- * a `ConvertInstruction` to detect some read steps from arrays that undergoes array-to-pointer
- * conversion.
- */
-private Instruction skipOneCopyValueInstructionRec(CopyValueInstruction copy) {
-  copy.getUnary() = result and not result instanceof CopyValueInstruction
-  or
-  result = skipOneCopyValueInstructionRec(copy.getUnary())
-}
-
-private Instruction skipCopyValueInstructions(Operand op) {
-  not result instanceof CopyValueInstruction and result = op.getDef()
-  or
-  result = skipOneCopyValueInstructionRec(op.getDef())
-}
-
 private class InexactLoadOperand extends LoadOperand {
   InexactLoadOperand() { this.isDefinitionInexact() }
 }
@@ -375,7 +357,8 @@ private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
     node1.asInstruction() = operand.getAnyDef() and
     not node1.asInstruction().isResultConflated() and
     operand = node2.asOperand() and
-    address = skipCopyValueInstructions(operand.getAddressOperand()) and
+    instructionOperandLocalFlowStep+(instructionNode(address),
+      operandNode(operand.getAddressOperand())) and
     (
       address instanceof LoadInstruction or
       address instanceof ArrayToPointerConvertInstruction or

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

@@ -185,54 +185,15 @@ class OperandNode extends Node, TOperandNode {
   override string toString() { result = this.getOperand().toString() }
 }
 
-/** An abstract class that defines conversion-like instructions. */
-abstract private class SkippableInstruction extends Instruction {
-  abstract Instruction getSourceInstruction();
-}
-
-/**
- * Gets the instruction that is propaged through a non-empty sequence of conversion-like instructions.
- */
-private Instruction skipSkippableInstructionsRec(SkippableInstruction skip) {
-  result = skip.getSourceInstruction() and not result instanceof SkippableInstruction
-  or
-  result = skipSkippableInstructionsRec(skip.getSourceInstruction())
-}
-
-/**
- * Gets the instruction that is propagated through a (possibly empty) sequence of conversion-like
- * instructions.
- */
-private Instruction skipSkippableInstructions(Instruction instr) {
-  result = instr and not result instanceof SkippableInstruction
-  or
-  result = skipSkippableInstructionsRec(instr)
-}
-
-private class SkippableCopyValueInstruction extends SkippableInstruction, CopyValueInstruction {
-  override Instruction getSourceInstruction() { result = this.getSourceValue() }
-}
-
-private class SkippableConvertInstruction extends SkippableInstruction, ConvertInstruction {
-  override Instruction getSourceInstruction() { result = this.getUnary() }
-}
-
-private class SkippableCheckedConvertInstruction extends SkippableInstruction,
-  CheckedConvertOrNullInstruction {
-  override Instruction getSourceInstruction() { result = this.getUnary() }
-}
-
-private class SkippableInheritanceConversionInstruction extends SkippableInstruction,
-  InheritanceConversionInstruction {
-  override Instruction getSourceInstruction() { result = this.getUnary() }
-}
-
 /**
  * INTERNAL: do not use. Gets the `FieldNode` corresponding to `instr`, if
  * `instr` is an instruction that propagates an address of a `FieldAddressInstruction`.
  */
 FieldNode getFieldNodeForFieldInstruction(Instruction instr) {
-  result.getFieldInstruction() = skipSkippableInstructions(instr)
+  result.getFieldInstruction() =
+    any(FieldAddressInstruction fai |
+      instructionOperandLocalFlowStep*(instructionNode(fai), instructionNode(instr))
+    )
 }
 
 /**
@@ -651,6 +612,11 @@ class VariableNode extends Node, TVariableNode {
  */
 InstructionNode instructionNode(Instruction instr) { result.getInstruction() = instr }
 
+/**
+ * Gets the node corresponding to `operand`.
+ */
+OperandNode operandNode(Operand operand) { result.getOperand() = operand }
+
 /**
  * DEPRECATED: use `definitionByReferenceNodeFromArgument` instead.
  *
@@ -770,17 +736,26 @@ private predicate flowIntoReadNode(Node nodeFrom, FieldNode nodeTo) {
 
 /**
  * INTERNAL: do not use.
- *
- * This is the local flow predicate that's used as a building block in global
- * data flow. It may have less flow than the `localFlowStep` predicate.
+ * Holds if `nodeFrom` is an operand and `nodeTo` is an instruction node that uses this operand, or
+ * if `nodeFrom` is an instruction and `nodeTo` is an operand that refers to this instruction.
  */
-cached
-predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
+predicate instructionOperandLocalFlowStep(Node nodeFrom, Node nodeTo) {
   // Operand -> Instruction flow
   simpleInstructionLocalFlowStep(nodeFrom.asOperand(), nodeTo.asInstruction())
   or
   // Instruction -> Operand flow
   simpleOperandLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asOperand())
+}
+
+/**
+ * INTERNAL: do not use.
+ *
+ * This is the local flow predicate that's used as a building block in global
+ * data flow. It may have less flow than the `localFlowStep` predicate.
+ */
+cached
+predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
+  instructionOperandLocalFlowStep(nodeFrom, nodeTo)
   or
   flowIntoReadNode(nodeFrom, nodeTo)
   or

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

@@ -58,9 +58,9 @@ edges
 | aliasing.cpp:79:11:79:20 | call to user_input | aliasing.cpp:80:12:80:13 | m1 |
 | aliasing.cpp:86:10:86:19 | call to user_input | aliasing.cpp:87:12:87:13 | m1 |
 | aliasing.cpp:92:12:92:21 | call to user_input | aliasing.cpp:93:12:93:13 | m1 |
-| aliasing.cpp:98:5:98:6 | m1 [post update] [m1] | aliasing.cpp:100:14:100:14 | Store [m1] |
+| aliasing.cpp:98:5:98:6 | m1 [post update] [m1] | aliasing.cpp:101:21:101:22 | m1 [m1] |
 | aliasing.cpp:98:10:98:19 | call to user_input | aliasing.cpp:98:5:98:6 | m1 [post update] [m1] |
-| aliasing.cpp:100:14:100:14 | Store [m1] | aliasing.cpp:102:8:102:10 | * ... |
+| aliasing.cpp:101:21:101:22 | m1 [m1] | aliasing.cpp:102:8:102:10 | * ... |
 | aliasing.cpp:106:3:106:20 | Chi [array content] | aliasing.cpp:121:15:121:16 | taint_a_ptr output argument [array content] |
 | aliasing.cpp:106:3:106:20 | Chi [array content] | aliasing.cpp:126:15:126:20 | taint_a_ptr output argument [array content] |
 | aliasing.cpp:106:3:106:20 | Chi [array content] | aliasing.cpp:131:15:131:16 | taint_a_ptr output argument [array content] |
@@ -373,7 +373,7 @@ nodes
 | aliasing.cpp:93:12:93:13 | m1 | semmle.label | m1 |
 | aliasing.cpp:98:5:98:6 | m1 [post update] [m1] | semmle.label | m1 [post update] [m1] |
 | aliasing.cpp:98:10:98:19 | call to user_input | semmle.label | call to user_input |
-| aliasing.cpp:100:14:100:14 | Store [m1] | semmle.label | Store [m1] |
+| aliasing.cpp:101:21:101:22 | m1 [m1] | semmle.label | m1 [m1] |
 | aliasing.cpp:102:8:102:10 | * ... | semmle.label | * ... |
 | aliasing.cpp:106:3:106:20 | Chi [array content] | semmle.label | Chi [array content] |
 | aliasing.cpp:106:3:106:20 | ChiTotal [post update] [array content] | semmle.label | ChiTotal [post update] [array content] |