Merge pull request #1888 from jbj/ir-dataflow-node-ipa

C++: Hide that IR DataFlow::Node is Instruction

Author: Robert Marsh

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

@@ -133,6 +133,14 @@ module TaintTracking {
     // Taint can flow into using ordinary data flow.
     DataFlow::localFlowStep(nodeFrom, nodeTo)
     or
+    localInstructionTaintStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
+  }
+
+  /**
+   * Holds if taint propagates from `nodeFrom` to `nodeTo` in exactly one local
+   * (intra-procedural) step.
+   */
+  private predicate localInstructionTaintStep(Instruction nodeFrom, Instruction nodeTo) {
     // Taint can flow through expressions that alter the value but preserve
     // more than one bit of it _or_ expressions that follow data through
     // pointer indirections.

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

@@ -8,16 +8,16 @@ private import DataFlowDispatch
  * to the callable. Instance arguments (`this` pointer) are also included.
  */
 class ArgumentNode extends Node {
-  ArgumentNode() { exists(CallInstruction call | this = call.getAnArgument()) }
+  ArgumentNode() { exists(CallInstruction call | this.asInstruction() = call.getAnArgument()) }
 
   /**
    * 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 = call.getPositionalArgument(pos)
+    this.asInstruction() = call.getPositionalArgument(pos)
     or
-    this = call.getThisArgument() and pos = -1
+    this.asInstruction() = call.getThisArgument() and pos = -1
   }
 
   /** Gets the call in which this node is an argument. */
@@ -37,24 +37,26 @@ class ReturnKind extends TReturnKind {
 
 /** A data flow node that occurs as the result of a `ReturnStmt`. */
 class ReturnNode extends Node {
-  ReturnNode() { exists(ReturnValueInstruction ret | this = ret.getReturnValue()) }
+  ReturnNode() { exists(ReturnValueInstruction ret | this.asInstruction() = ret.getReturnValue()) }
 
   /** Gets the kind of this returned value. */
   ReturnKind getKind() { result = TNormalReturnKind() }
 }
 
 /** A data flow node that represents the output of a call. */
-class OutNode extends Node, CallInstruction {
+class OutNode extends Node {
+  override CallInstruction instr;
+
   /** Gets the underlying call. */
-  DataFlowCall getCall() { result = this }
+  DataFlowCall getCall() { result = instr }
 }
 
 /**
  * Gets a node that can read the value returned from `call` with return kind
  * `kind`.
  */
 OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
-  result = call and
+  result.getCall() = call and
   kind = TNormalReturnKind()
 }
 
@@ -192,11 +194,13 @@ class DataFlowType = Type;
 class DataFlowLocation = Location;
 
 /** A function call relevant for data flow. */
-class DataFlowCall extends CallInstruction, Node {
+class DataFlowCall extends CallInstruction {
   /**
    * Gets the nth argument for this call.
    *
    * The range of `n` is from `0` to `getNumberOfArguments() - 1`.
    */
-  Node getArgument(int n) { result = this.getPositionalArgument(n) }
+  Node getArgument(int n) { result.asInstruction() = this.getPositionalArgument(n) }
+
+  Function getEnclosingCallable() { result = this.getEnclosingFunction() }
 }

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

@@ -6,21 +6,36 @@ private import cpp
 private import semmle.code.cpp.ir.IR
 private import semmle.code.cpp.controlflow.IRGuards
 
+/**
+ * A newtype wrapper to prevent accidental casts between `Node` and
+ * `Instruction`. This ensures we can add `Node`s that are not `Instruction`s
+ * in the future.
+ */
+private newtype TIRDataFlowNode = MkIRDataFlowNode(Instruction i)
+
 /**
  * A node in a data flow graph.
  *
  * A node can be either an expression, a parameter, or an uninitialized local
  * variable. Such nodes are created with `DataFlow::exprNode`,
  * `DataFlow::parameterNode`, and `DataFlow::uninitializedNode` respectively.
  */
-class Node extends Instruction {
+class Node extends TIRDataFlowNode {
+  Instruction instr;
+
+  Node() { this = MkIRDataFlowNode(instr) }
+
   /**
    * INTERNAL: Do not use. Alternative name for `getFunction`.
    */
-  Function getEnclosingCallable() { result = this.getEnclosingFunction() }
+  Function getEnclosingCallable() { result = this.getFunction() }
+
+  Function getFunction() { result = instr.getEnclosingFunction() }
 
   /** Gets the type of this node. */
-  Type getType() { result = this.getResultType() }
+  Type getType() { result = instr.getResultType() }
+
+  Instruction asInstruction() { this = MkIRDataFlowNode(result) }
 
   /**
    * Gets the non-conversion expression corresponding to this node, if any. If
@@ -29,30 +44,33 @@ class Node extends Instruction {
    * expression.
    */
   Expr asExpr() {
-    result.getConversion*() = this.getConvertedResultExpression() and
+    result.getConversion*() = instr.getConvertedResultExpression() and
     not result instanceof Conversion
   }
 
   /**
    * Gets the expression corresponding to this node, if any. The returned
    * expression may be a `Conversion`.
    */
-  Expr asConvertedExpr() { result = this.getConvertedResultExpression() }
+  Expr asConvertedExpr() { result = instr.getConvertedResultExpression() }
 
   /** Gets the parameter corresponding to this node, if any. */
-  Parameter asParameter() { result = this.(InitializeParameterInstruction).getParameter() }
+  Parameter asParameter() { result = instr.(InitializeParameterInstruction).getParameter() }
 
   /**
    * Gets the uninitialized local variable corresponding to this node, if
    * any.
    */
-  LocalVariable asUninitialized() { result = this.(UninitializedInstruction).getLocalVariable() }
+  LocalVariable asUninitialized() { result = instr.(UninitializedInstruction).getLocalVariable() }
 
   /**
    * Gets an upper bound on the type of this node.
    */
   Type getTypeBound() { result = getType() }
 
+  /** Gets the location of this element. */
+  Location getLocation() { result = instr.getLocation() }
+
   /**
    * Holds if this element is at the specified location.
    * The location spans column `startcolumn` of line `startline` to
@@ -63,8 +81,10 @@ class Node extends Instruction {
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn
   ) {
-    getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
+
+  string toString() { result = instr.toString() }
 }
 
 /**
@@ -92,19 +112,27 @@ class ExprNode extends Node {
  * The value of a parameter at function entry, viewed as a node in a data
  * flow graph.
  */
-class ParameterNode extends Node, InitializeParameterInstruction {
+class ParameterNode extends Node {
+  override InitializeParameterInstruction instr;
+
   /**
    * Holds if this node is the parameter of `c` at the specified (zero-based)
    * position. The implicit `this` parameter is considered to have index `-1`.
    */
-  predicate isParameterOf(Function f, int i) { f.getParameter(i) = getParameter() }
+  predicate isParameterOf(Function f, int i) { f.getParameter(i) = instr.getParameter() }
+
+  Parameter getParameter() { result = instr.getParameter() }
 }
 
 /**
  * The value of an uninitialized local variable, viewed as a node in a data
  * flow graph.
  */
-class UninitializedNode extends Node, UninitializedInstruction { }
+class UninitializedNode extends Node {
+  override UninitializedInstruction instr;
+
+  LocalVariable getLocalVariable() { result = instr.getLocalVariable() }
+}
 
 /**
  * A node associated with an object after an operation that might have
@@ -164,10 +192,14 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { simpleLocalFlowStep(nodeFr
  * data flow. It may have less flow than the `localFlowStep` predicate.
  */
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
-  nodeTo.(CopyInstruction).getSourceValue() = nodeFrom or
-  nodeTo.(PhiInstruction).getAnOperand().getDef() = nodeFrom or
+  simpleInstructionLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
+}
+
+private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction iTo) {
+  iTo.(CopyInstruction).getSourceValue() = iFrom or
+  iTo.(PhiInstruction).getAnOperand().getDef() = iFrom or
   // Treat all conversions as flow, even conversions between different numeric types.
-  nodeTo.(ConvertInstruction).getUnary() = nodeFrom
+  iTo.(ConvertInstruction).getUnary() = iFrom
 }
 
 /**