C++: Respond to review comments.

Author: MathiasVP

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

@@ -350,20 +350,25 @@ private class InexactLoadOperand extends LoadOperand {
   InexactLoadOperand() { this.isDefinitionInexact() }
 }
 
+/** Get the result type of an `Instruction` i, if it is a `PointerType`. */
+private PointerType getPointerType(Instruction i) {
+  // We are done if the type is a pointer type that is not a glvalue
+  i.getResultLanguageType().hasType(result, false)
+  or
+  // Some instructions produce a glvalue. Recurse past those to get the actual `PointerType`.
+  result = getPointerType(i.(PointerOffsetInstruction).getLeft())
+}
+
 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(InexactLoadOperand operand, Instruction address |
+  exists(InexactLoadOperand operand, LoadInstruction load |
+    load.getSourceValueOperand() = operand and
     node1.asInstruction() = operand.getAnyDef() and
     not node1.asInstruction().isResultConflated() and
     operand = node2.asOperand() and
-    instructionOperandLocalFlowStep+(instructionNode(address),
-      operandNode(operand.getAddressOperand())) and
-    (
-      address instanceof LoadInstruction or
-      address instanceof ArrayToPointerConvertInstruction or
-      address instanceof PointerOffsetInstruction
-    )
+    // Ensure that the load is actually loading from an array or a pointer.
+    getPointerType(load.getSourceAddress()).getBaseType() = load.getResultType()
   )
 }
 
@@ -392,19 +397,18 @@ bindingset[result, i]
 private int unbindInt(int i) { i <= result and i >= result }
 
 pragma[noinline]
-private predicate getFieldNodeFromLoadOperand(FieldNode fieldNode, LoadOperand loadOperand) {
-  fieldNode = getFieldNodeForFieldInstruction(loadOperand.getAddressOperand().getDef())
+private FieldNode getFieldNodeFromLoadOperand(LoadOperand loadOperand) {
+  result = getFieldNodeForFieldInstruction(loadOperand.getAddressOperand().getDef())
 }
 
-// Sometimes there's no explicit field dereference. In such cases we use the IR alias analysis to
-// determine the offset being, and deduce the field from this information.
+/**
+ * Sometimes there's no explicit field dereference. In such cases we use the IR alias analysis to
+ * determine the offset being, and deduce the field from this information.
+ */
 private predicate aliasedReadStep(Node node1, FieldContent f, Node node2) {
   exists(LoadOperand operand, Class c, int startBit, int endBit |
     // Ensure that we don't already catch this store step using a `FieldNode`.
-    not exists(FieldNode node |
-      getFieldNodeFromLoadOperand(node, operand) and
-      instrToFieldNodeReadStep(node, f, _)
-    ) and
+    not instrToFieldNodeReadStep(getFieldNodeFromLoadOperand(operand), f, _) and
     node1.asInstruction() = operand.getAnyDef() and
     node2.asOperand() = operand and
     not node1.asInstruction().isResultConflated() and
@@ -414,6 +418,11 @@ private predicate aliasedReadStep(Node node1, FieldContent f, Node node2) {
   )
 }
 
+/** Get the result type of an `Instruction` i, if it is a `ReferenceType`. */
+private ReferenceType getReferenceType(Instruction i) {
+  i.getResultLanguageType().hasType(result, false)
+}
+
 /**
  * In cases such as:
  * ```cpp
@@ -431,7 +440,7 @@ private predicate aliasedReadStep(Node node1, FieldContent f, Node node2) {
  */
 private predicate innerReadSteap(Node node1, Content a, Node node2) {
   a = TArrayContent() and
-  exists(WriteSideEffectInstruction write, CallInstruction call, Expr arg |
+  exists(WriteSideEffectInstruction write, CallInstruction call, CopyValueInstruction copyValue |
     write.getPrimaryInstruction() = call and
     node1.asInstruction() = write and
     (
@@ -440,8 +449,9 @@ private predicate innerReadSteap(Node node1, Content a, Node node2) {
       exists(ChiInstruction chi | chi.getPartial() = write and not chi.isResultConflated())
     ) and
     node2.asInstruction() = write and
-    arg = call.getArgument(write.getIndex()).getUnconvertedResultExpression() and
-    (arg instanceof AddressOfExpr or arg.getConversion() instanceof ReferenceToExpr)
+    copyValue = call.getArgument(write.getIndex()) and
+    [getPointerType(copyValue).getBaseType(), getReferenceType(copyValue).getBaseType()] =
+      copyValue.getSourceValue().getResultType()
   )
 }
 

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

@@ -192,7 +192,7 @@ class OperandNode extends Node, TOperandNode {
 FieldNode getFieldNodeForFieldInstruction(Instruction instr) {
   result.getFieldInstruction() =
     any(FieldAddressInstruction fai |
-      instructionOperandLocalFlowStep*(instructionNode(fai), instructionNode(instr))
+      longestRegisterInstructionOperandLocalFlowStep(instructionNode(fai), instructionNode(instr))
     )
 }
 
@@ -251,13 +251,19 @@ class FieldNode extends Node, TFieldNode {
 /**
  * INTERNAL: do not use. A partial definition of a `FieldNode`.
  */
-class PartialFieldDefinition extends PartialDefinition {
-  override FieldNode node;
-
-  override FieldNode getPreUpdateNode() { result = node }
+class PartialFieldDefinition extends FieldNode, PartialDefinition {
+  /**
+   * The pre-update node of a partial definition of a `FieldNode` is the `FieldNode` itself. This ensures
+   * that the data flow library's reverse read mechanism builds up the correct access path for nested
+   * fields.
+   * For instance, in `a.b.c = x` there is a partial definition for `c` (let's call it `post[c]`) and a
+   * partial definition for `b` (let's call it `post[b]`), and there is a read step from `b` to `c`
+   * (using `instrToFieldNodeReadStep`), so there is a store step from `post[c]` to `post[b]`.
+   */
+  override FieldNode getPreUpdateNode() { result = this }
 
   override Expr getDefinedExpr() {
-    result = node.getFieldInstruction().getObjectAddress().getUnconvertedResultExpression()
+    result = this.getFieldInstruction().getObjectAddress().getUnconvertedResultExpression()
   }
 }
 
@@ -411,39 +417,13 @@ abstract class PostUpdateNode extends Node {
   override Location getLocation() { result = getPreUpdateNode().getLocation() }
 }
 
-/**
- * A partial definition of a node. A partial definition that targets arrays or pointers is attached to
- * an `InstructionNode` (specifially, to the `ChiInstruction` that follows the `StoreInstruction`), and
- * a partial definition that targets a `FieldNode` is attached to the `FieldNode`.
- *
- * The pre-update node of a partial definition of a `FieldNode` is the `FieldNode` itself. This ensures
- * that the data flow library's reverse read mechanism builds up the correct access path for nested
- * fields.
- * For instance, in `a.b.c = x` there is a partial definition for `c` (let's call it `post[c]`) and a
- * partial definition for `b` (let's call it `post[b]`), and there is a read step from `b` to `c`
- * (using `instrToFieldNodeReadStep`), so there is a store step from `post[c]` to `post[b]`.
- */
-private newtype TPartialDefinition =
-  MkPartialDefinition(Node node) {
-    isPointerStoreNode(node, _, _) or
-    isArrayStoreNode(node, _, _) or
-    node instanceof FieldNode
-  }
-
 /** INTERNAL: do not use. A partial definition of a node. */
-abstract class PartialDefinition extends TPartialDefinition {
-  Node node;
-
-  PartialDefinition() { this = MkPartialDefinition(node) }
-
+abstract class PartialDefinition extends Node {
   /** Gets the node before the state update. */
   abstract Node getPreUpdateNode();
 
   /** Gets the expression that is partially defined by this node. */
   abstract Expr getDefinedExpr();
-
-  /** Gets a string representation of this partial definition. */
-  string toString() { result = node.toString() + " [partial definition]" }
 }
 
 /**
@@ -475,52 +455,44 @@ class PartialDefinitionNode extends PostUpdateNode, TPartialDefinitionNode {
   override string toString() { result = getPreUpdateNode().toString() + " [post update]" }
 }
 
-private predicate isArrayStoreNode(
-  InstructionNode node, ChiInstruction chi, PointerAddInstruction add
-) {
-  chi = node.getInstruction() and
-  not chi.isResultConflated() and
-  exists(StoreInstruction store |
-    chi.getPartial() = store and
-    add = store.getDestinationAddress()
-  )
-}
-
 /**
  * The `PostUpdateNode` that is the target of a `arrayStoreStepChi` store step. The overriden
  * `ChiInstruction` corresponds to the instruction represented by `node2` in `arrayStoreStepChi`.
  */
-private class ArrayStoreNode extends PartialDefinition {
-  override InstructionNode node;
+private class ArrayStoreNode extends InstructionNode, PartialDefinition {
   ChiInstruction chi;
   PointerAddInstruction add;
 
-  ArrayStoreNode() { isArrayStoreNode(node, chi, add) }
+  ArrayStoreNode() {
+    chi = this.getInstruction() and
+    not chi.isResultConflated() and
+    exists(StoreInstruction store |
+      chi.getPartial() = store and
+      add = store.getDestinationAddress()
+    )
+  }
 
   override Node getPreUpdateNode() { result.asOperand() = chi.getTotalOperand() }
 
   override Expr getDefinedExpr() { result = add.getLeft().getUnconvertedResultExpression() }
 }
 
-private predicate isPointerStoreNode(InstructionNode node, ChiInstruction chi, LoadInstruction load) {
-  chi = node.getInstruction() and
-  not chi.isResultConflated() and
-  exists(StoreInstruction store |
-    chi.getPartial() = store and
-    load = store.getDestinationAddress().(CopyValueInstruction).getUnary()
-  )
-}
-
 /**
  * The `PostUpdateNode` that is the target of a `arrayStoreStepChi` store step. The overriden
  * `ChiInstruction` corresponds to the instruction represented by `node2` in `arrayStoreStepChi`.
  */
-private class PointerStoreNode extends PartialDefinition {
-  override InstructionNode node;
+private class PointerStoreNode extends InstructionNode, PartialDefinition {
   ChiInstruction chi;
   LoadInstruction load;
 
-  PointerStoreNode() { isPointerStoreNode(node, chi, load) }
+  PointerStoreNode() {
+    chi = this.getInstruction() and
+    not chi.isResultConflated() and
+    exists(StoreInstruction store |
+      chi.getPartial() = store and
+      load = store.getDestinationAddress().(CopyValueInstruction).getUnary()
+    )
+  }
 
   override Node getPreUpdateNode() { result.asOperand() = chi.getTotalOperand() }
 
@@ -734,12 +706,40 @@ private predicate flowIntoReadNode(Node nodeFrom, FieldNode nodeTo) {
   )
 }
 
+/** Holds if `node` holds an `Instruction` or `Operand` that has a register result. */
+private predicate hasRegisterResult(Node node) {
+  node.asOperand() instanceof RegisterOperand
+  or
+  exists(Instruction i | i = node.asInstruction() and not i.hasMemoryResult())
+}
+
+/**
+ * Holds if there is a `Instruction` or `Operand` flow step from `nodeFrom` to `nodeTo` and both
+ * `nodeFrom` and `nodeTo` wraps register results.
+ */
+private predicate registerInstructionOperandLocalFlowStep(Node nodeFrom, Node nodeTo) {
+  hasRegisterResult(nodeFrom) and
+  hasRegisterResult(nodeTo) and
+  instructionOperandLocalFlowStep(nodeFrom, nodeTo)
+}
+
+/**
+ * INTERNAL: do not use.
+ * Holds if `nodeFrom` has no incoming local `Operand` or `Instruction` register flow and `nodeFrom` can
+ * reach `nodeTo` using only local `Instruction` or `Operand` register flow steps.
+ */
+bindingset[nodeTo]
+private predicate longestRegisterInstructionOperandLocalFlowStep(Node nodeFrom, Node nodeTo) {
+  registerInstructionOperandLocalFlowStep*(nodeFrom, nodeTo) and
+  not registerInstructionOperandLocalFlowStep(_, nodeFrom)
+}
+
 /**
  * INTERNAL: do not use.
  * 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.
  */
-predicate instructionOperandLocalFlowStep(Node nodeFrom, Node nodeTo) {
+private predicate instructionOperandLocalFlowStep(Node nodeFrom, Node nodeTo) {
   // Operand -> Instruction flow
   simpleInstructionLocalFlowStep(nodeFrom.asOperand(), nodeTo.asInstruction())
   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:101:21:101:22 | m1 [m1] |
+| aliasing.cpp:98:5:98:6 | m1 [post update] [m1] | aliasing.cpp:100:14:100:14 | Store [m1] |
 | aliasing.cpp:98:10:98:19 | call to user_input | aliasing.cpp:98:5:98:6 | m1 [post update] [m1] |
-| aliasing.cpp:101:21:101:22 | m1 [m1] | aliasing.cpp:102:8:102:10 | * ... |
+| aliasing.cpp:100:14:100:14 | Store [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:101:21:101:22 | m1 [m1] | semmle.label | m1 [m1] |
+| aliasing.cpp:100:14:100:14 | Store [m1] | semmle.label | Store [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] |