C++: Wire up param/arg indirections in data flow

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

@@ -8,16 +8,28 @@ private import DataFlowDispatch
  * to the callable. Instance arguments (`this` pointer) are also included.
  */
 class ArgumentNode extends InstructionNode {
-  ArgumentNode() { exists(CallInstruction call | this.getInstruction() = call.getAnArgument()) }
+  ArgumentNode() {
+    exists(CallInstruction call |
+      instr = call.getAnArgument()
+      or
+      instr.(ReadSideEffectInstruction).getPrimaryInstruction() = call
+    )
+  }
 
   /**
    * Holds if this argument occurs at the given position in the given call.
    * The instance argument is considered to have index `-1`.
    */
   predicate argumentOf(DataFlowCall call, int pos) {
-    this.getInstruction() = call.getPositionalArgument(pos)
+    instr = call.getPositionalArgument(pos)
     or
-    this.getInstruction() = call.getThisArgument() and pos = -1
+    instr = call.getThisArgument() and pos = -1
+    or
+    exists(ReadSideEffectInstruction read |
+      read = instr and
+      read.getPrimaryInstruction() = call and
+      pos = getArgumentPosOfSideEffect(read.getIndex())
+    )
   }
 
   /** Gets the call in which this node is an argument. */

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

@@ -54,8 +54,8 @@ class Node extends TIRDataFlowNode {
   /** Gets the argument that defines this `DefinitionByReferenceNode`, if any. */
   Expr asDefiningArgument() { result = this.(DefinitionByReferenceNode).getArgument() }
 
-  /** Gets the parameter corresponding to this node, if any. */
+  /** Gets the positional parameter corresponding to this node, if any. */
-  Parameter asParameter() { result = this.(ParameterNode).getParameter() }
+  Parameter asParameter() { result = this.(PositionalParameterNode).getParameter() }
 
   /**
    * Gets the variable corresponding to this node, if any. This can be used for
@@ -142,30 +142,72 @@ class ExprNode extends InstructionNode {
   override string toString() { result = this.asConvertedExpr().toString() }
 }
 
+/**
+ * INTERNAL: do not use. Translates a parameter/argument index into a negative
+ * number that denotes the index of its side effect (pointer indirection).
+ */
+bindingset[index]
+int getArgumentPosOfSideEffect(int index) {
+  // -1 -> -2
+  //  0 -> -3
+  //  1 -> -4
+  // ...
+  result = -3 - index
+}
+
 /**
  * The value of a parameter at function entry, viewed as a node in a data
  * flow graph.
  */
-class ParameterNode extends InstructionNode {
+abstract class ParameterNode extends InstructionNode {
-  override InitializeParameterInstruction instr;
-
   /**
-   * Holds if this node is the parameter of `c` at the specified (zero-based)
+   * Holds if this node is the parameter of `f` at the specified position. The
-   * position. The implicit `this` parameter is considered to have index `-1`.
+   * implicit `this` parameter is considered to have position `-1`, and
+   * pointer-indirection parameters are at further negative positions.
    */
-  predicate isParameterOf(Function f, int i) { f.getParameter(i) = instr.getParameter() }
+  abstract predicate isParameterOf(Function f, int pos);
-
-  Parameter getParameter() { result = instr.getParameter() }
-
-  override string toString() { result = instr.getParameter().toString() }
 }
 
-private class ThisParameterNode extends InstructionNode {
+private class ThisParameterNode extends ParameterNode {
   override InitializeThisInstruction instr;
 
+  override predicate isParameterOf(Function f, int pos) {
+    pos = -1 and
+    instr.getEnclosingFunction() = f
+  }
+
   override string toString() { result = "this" }
 }
 
+class PositionalParameterNode extends ParameterNode {
+  override InitializeParameterInstruction instr;
+
+  override predicate isParameterOf(Function f, int pos) {
+    f.getParameter(pos) = instr.getParameter()
+  }
+
+  /** Gets the `Parameter` associated with this node. */
+  Parameter getParameter() { result = instr.getParameter() }
+
+  override string toString() { result = this.getParameter().toString() }
+}
+
+private class ParameterIndirectionNode extends ParameterNode {
+  override InitializeIndirectionInstruction instr;
+
+  override predicate isParameterOf(Function f, int pos) {
+    exists(int index |
+      f.getParameter(index) = instr.getParameter() and
+      pos = getArgumentPosOfSideEffect(index)
+    )
+  }
+
+  /** Gets the `Parameter` associated with this node. */
+  Parameter getParameter() { result = instr.getParameter() }
+
+  override string toString() { result = "*" + this.getParameter().toString() }
+}
+
 /**
  * DEPRECATED: Data flow was never an accurate way to determine what
  * expressions might be uninitialized. It errs on the side of saying that
@@ -294,7 +336,7 @@ ExprNode convertedExprNode(Expr e) { result.getExpr() = e }
 /**
  * Gets the `Node` corresponding to the value of `p` at function entry.
  */
-ParameterNode parameterNode(Parameter p) { result.getParameter() = p }
+PositionalParameterNode parameterNode(Parameter p) { result.getParameter() = p }
 
 /** Gets the `VariableNode` corresponding to the variable `v`. */
 VariableNode variableNode(Variable v) { result.getVariable() = v }
@@ -328,6 +370,24 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   or
   iTo.(PhiInstruction).getAnOperand().getDef() = iFrom
   or
+  // A read side effect is almost never exact since we don't know exactly how
+  // much memory the callee will read.
+  iTo.(ReadSideEffectInstruction).getSideEffectOperand().getAnyDef() = iFrom and
+  not iFrom.isResultConflated()
+  or
+  // Loading a single `int` from an `int *` parameter is not an exact load since
+  // the parameter may point to an entire array rather than a single `int`. The
+  // following rule ensures that any flow going into the
+  // `InitializeIndirectionInstruction`, even if it's for a different array
+  // element, will propagate to a load of the first element.
+  //
+  // Since we're linking `InitializeIndirectionInstruction` and
+  // `LoadInstruction` together directly, this rule will break if there's any
+  // reassignment of the parameter indirection, including a conditional one that
+  // leads to a phi node.
+  iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() =
+    iFrom.(InitializeIndirectionInstruction)
+  or
   // Treat all conversions as flow, even conversions between different numeric types.
   iTo.(ConvertInstruction).getUnary() = iFrom
   or

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected

@@ -23,7 +23,6 @@
 | lambdas.cpp:8:10:8:15 | lambdas.cpp:18:8:18:8 | AST only |
 | lambdas.cpp:8:10:8:15 | lambdas.cpp:21:3:21:6 | AST only |
 | lambdas.cpp:8:10:8:15 | lambdas.cpp:29:3:29:6 | AST only |
-| lambdas.cpp:8:10:8:15 | lambdas.cpp:41:8:41:8 | AST only |
 | lambdas.cpp:43:7:43:12 | lambdas.cpp:46:7:46:7 | AST only |
 | ref.cpp:29:11:29:16 | ref.cpp:62:10:62:11 | AST only |
 | ref.cpp:53:9:53:10 | ref.cpp:56:10:56:11 | AST only |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected

@@ -39,6 +39,7 @@
 | globals.cpp:12:10:12:24 | flowTestGlobal1 | globals.cpp:13:23:13:28 | call to source |
 | globals.cpp:19:10:19:24 | flowTestGlobal2 | globals.cpp:23:23:23:28 | call to source |
 | lambdas.cpp:35:8:35:8 | a | lambdas.cpp:8:10:8:15 | call to source |
+| lambdas.cpp:41:8:41:8 | (reference dereference) | lambdas.cpp:8:10:8:15 | call to source |
 | test.cpp:7:8:7:9 | t1 | test.cpp:6:12:6:17 | call to source |
 | test.cpp:9:8:9:9 | t1 | test.cpp:6:12:6:17 | call to source |
 | test.cpp:10:8:10:9 | t2 | test.cpp:6:12:6:17 | call to source |

cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected

@@ -20,7 +20,6 @@
 | taint.cpp:130:7:130:9 | taint.cpp:127:8:127:13 | IR only |
 | taint.cpp:137:7:137:9 | taint.cpp:120:11:120:16 | AST only |
 | taint.cpp:173:8:173:13 | taint.cpp:164:19:164:24 | AST only |
-| taint.cpp:181:8:181:9 | taint.cpp:185:11:185:16 | AST only |
 | taint.cpp:195:7:195:7 | taint.cpp:192:23:192:28 | AST only |
 | taint.cpp:195:7:195:7 | taint.cpp:193:6:193:6 | AST only |
 | taint.cpp:216:7:216:7 | taint.cpp:207:6:207:11 | AST only |
@@ -28,7 +27,6 @@
 | taint.cpp:233:8:233:8 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:236:3:236:6 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:244:3:244:6 | taint.cpp:223:10:223:15 | AST only |
-| taint.cpp:256:8:256:8 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:261:7:261:7 | taint.cpp:258:7:258:12 | AST only |
 | taint.cpp:351:7:351:7 | taint.cpp:330:6:330:11 | AST only |
 | taint.cpp:352:7:352:7 | taint.cpp:330:6:330:11 | AST only |

cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected

@@ -8,9 +8,11 @@
 | taint.cpp:151:7:151:12 | call to select | taint.cpp:151:20:151:25 | call to source |
 | taint.cpp:167:8:167:13 | call to source | taint.cpp:167:8:167:13 | call to source |
 | taint.cpp:168:8:168:14 | tainted | taint.cpp:164:19:164:24 | call to source |
+| taint.cpp:181:8:181:9 | * ... | taint.cpp:185:11:185:16 | call to source |
 | taint.cpp:210:7:210:7 | x | taint.cpp:207:6:207:11 | call to source |
 | taint.cpp:215:7:215:7 | x | taint.cpp:207:6:207:11 | call to source |
 | taint.cpp:250:8:250:8 | a | taint.cpp:223:10:223:15 | call to source |
+| taint.cpp:256:8:256:8 | (reference dereference) | taint.cpp:223:10:223:15 | call to source |
 | taint.cpp:280:7:280:7 | t | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:289:7:289:7 | t | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:290:7:290:7 | x | taint.cpp:275:6:275:11 | call to source |