Merge pull request github#5773 from hvitved/dataflow/aggressive-caching

Data flow: Cache most language-dependent predicates

Author: aschackmull

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll

@@ -526,7 +526,6 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) {
  * 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) {
   // Expr -> Expr
   exprToExprStep_nocfg(nodeFrom.asExpr(), nodeTo.asExpr())

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl2.qll

@@ -45,6 +45,7 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
  * local data flow steps. That is, `nodeFrom` and `nodeTo` are likely to represent
  * different objects.
  */
+cached
 predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
   // Taint can flow through expressions that alter the value but preserve
   // more than one bit of it _or_ expressions that follow data through

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl3.qll

@@ -165,106 +165,133 @@ private predicate nodeIsBarrierEqualityCandidate(
   any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
 }
 
-private predicate nodeIsBarrier(DataFlow::Node node) {
-  exists(Variable checkedVar |
-    readsVariable(node.asInstruction(), checkedVar) and
-    hasUpperBoundsCheck(checkedVar)
-  )
-  or
-  exists(Variable checkedVar, Operand access |
-    /*
-     * This node is guarded by a condition that forces the accessed variable
-     * to equal something else.  For example:
-     * ```
-     * x = taintsource()
-     * if (x == 10) {
-     *   taintsink(x); // not considered tainted
-     * }
-     * ```
-     */
+cached
+private module Cached {
+  cached
+  predicate nodeIsBarrier(DataFlow::Node node) {
+    exists(Variable checkedVar |
+      readsVariable(node.asInstruction(), checkedVar) and
+      hasUpperBoundsCheck(checkedVar)
+    )
+    or
+    exists(Variable checkedVar, Operand access |
+      /*
+       * This node is guarded by a condition that forces the accessed variable
+       * to equal something else.  For example:
+       * ```
+       * x = taintsource()
+       * if (x == 10) {
+       *   taintsink(x); // not considered tainted
+       * }
+       * ```
+       */
+
+      nodeIsBarrierEqualityCandidate(node, access, checkedVar) and
+      readsVariable(access.getDef(), checkedVar)
+    )
+  }
 
-    nodeIsBarrierEqualityCandidate(node, access, checkedVar) and
-    readsVariable(access.getDef(), checkedVar)
-  )
-}
+  cached
+  predicate nodeIsBarrierIn(DataFlow::Node node) {
+    // don't use dataflow into taint sources, as this leads to duplicate results.
+    exists(Expr source | isUserInput(source, _) |
+      node = DataFlow::exprNode(source)
+      or
+      // This case goes together with the similar (but not identical) rule in
+      // `getNodeForSource`.
+      node = DataFlow::definitionByReferenceNodeFromArgument(source)
+    )
+    or
+    // don't use dataflow into binary instructions if both operands are unpredictable
+    exists(BinaryInstruction iTo |
+      iTo = node.asInstruction() and
+      not predictableInstruction(iTo.getLeft()) and
+      not predictableInstruction(iTo.getRight()) and
+      // propagate taint from either the pointer or the offset, regardless of predictability
+      not iTo instanceof PointerArithmeticInstruction
+    )
+    or
+    // don't use dataflow through calls to pure functions if two or more operands
+    // are unpredictable
+    exists(Instruction iFrom1, Instruction iFrom2, CallInstruction iTo |
+      iTo = node.asInstruction() and
+      isPureFunction(iTo.getStaticCallTarget().getName()) and
+      iFrom1 = iTo.getAnArgument() and
+      iFrom2 = iTo.getAnArgument() and
+      not predictableInstruction(iFrom1) and
+      not predictableInstruction(iFrom2) and
+      iFrom1 != iFrom2
+    )
+  }
 
-private predicate nodeIsBarrierIn(DataFlow::Node node) {
-  // don't use dataflow into taint sources, as this leads to duplicate results.
-  exists(Expr source | isUserInput(source, _) |
-    node = DataFlow::exprNode(source)
+  cached
+  Element adjustedSink(DataFlow::Node sink) {
+    // TODO: is it more appropriate to use asConvertedExpr here and avoid
+    // `getConversion*`? Or will that cause us to miss some cases where there's
+    // flow to a conversion (like a `ReferenceDereferenceExpr`) and we want to
+    // pretend there was flow to the converted `Expr` for the sake of
+    // compatibility.
+    sink.asExpr().getConversion*() = result
     or
-    // This case goes together with the similar (but not identical) rule in
-    // `getNodeForSource`.
-    node = DataFlow::definitionByReferenceNodeFromArgument(source)
-  )
-  or
-  // don't use dataflow into binary instructions if both operands are unpredictable
-  exists(BinaryInstruction iTo |
-    iTo = node.asInstruction() and
-    not predictableInstruction(iTo.getLeft()) and
-    not predictableInstruction(iTo.getRight()) and
-    // propagate taint from either the pointer or the offset, regardless of predictability
-    not iTo instanceof PointerArithmeticInstruction
-  )
-  or
-  // don't use dataflow through calls to pure functions if two or more operands
-  // are unpredictable
-  exists(Instruction iFrom1, Instruction iFrom2, CallInstruction iTo |
-    iTo = node.asInstruction() and
-    isPureFunction(iTo.getStaticCallTarget().getName()) and
-    iFrom1 = iTo.getAnArgument() and
-    iFrom2 = iTo.getAnArgument() and
-    not predictableInstruction(iFrom1) and
-    not predictableInstruction(iFrom2) and
-    iFrom1 != iFrom2
-  )
-}
+    // For compatibility, send flow from arguments to parameters, even for
+    // functions with no body.
+    exists(FunctionCall call, int i |
+      sink.asExpr() = call.getArgument(i) and
+      result = resolveCall(call).getParameter(i)
+    )
+    or
+    // For compatibility, send flow into a `Variable` if there is flow to any
+    // Load or Store of that variable.
+    exists(CopyInstruction copy |
+      copy.getSourceValue() = sink.asInstruction() and
+      (
+        readsVariable(copy, result) or
+        writesVariable(copy, result)
+      ) and
+      not hasUpperBoundsCheck(result)
+    )
+    or
+    // For compatibility, send flow into a `NotExpr` even if it's part of a
+    // short-circuiting condition and thus might get skipped.
+    result.(NotExpr).getOperand() = sink.asExpr()
+    or
+    // Taint postfix and prefix crement operations when their operand is tainted.
+    result.(CrementOperation).getAnOperand() = sink.asExpr()
+    or
+    // Taint `e1 += e2`, `e &= e2` and friends when `e1` or `e2` is tainted.
+    result.(AssignOperation).getAnOperand() = sink.asExpr()
+    or
+    result =
+      sink.asOperand()
+          .(SideEffectOperand)
+          .getUse()
+          .(ReadSideEffectInstruction)
+          .getArgumentDef()
+          .getUnconvertedResultExpression()
+  }
 
-private Element adjustedSink(DataFlow::Node sink) {
-  // TODO: is it more appropriate to use asConvertedExpr here and avoid
-  // `getConversion*`? Or will that cause us to miss some cases where there's
-  // flow to a conversion (like a `ReferenceDereferenceExpr`) and we want to
-  // pretend there was flow to the converted `Expr` for the sake of
-  // compatibility.
-  sink.asExpr().getConversion*() = result
-  or
-  // For compatibility, send flow from arguments to parameters, even for
-  // functions with no body.
-  exists(FunctionCall call, int i |
-    sink.asExpr() = call.getArgument(i) and
-    result = resolveCall(call).getParameter(i)
-  )
-  or
-  // For compatibility, send flow into a `Variable` if there is flow to any
-  // Load or Store of that variable.
-  exists(CopyInstruction copy |
-    copy.getSourceValue() = sink.asInstruction() and
-    (
-      readsVariable(copy, result) or
-      writesVariable(copy, result)
-    ) and
-    not hasUpperBoundsCheck(result)
-  )
-  or
-  // For compatibility, send flow into a `NotExpr` even if it's part of a
-  // short-circuiting condition and thus might get skipped.
-  result.(NotExpr).getOperand() = sink.asExpr()
-  or
-  // Taint postfix and prefix crement operations when their operand is tainted.
-  result.(CrementOperation).getAnOperand() = sink.asExpr()
-  or
-  // Taint `e1 += e2`, `e &= e2` and friends when `e1` or `e2` is tainted.
-  result.(AssignOperation).getAnOperand() = sink.asExpr()
-  or
-  result =
-    sink.asOperand()
-        .(SideEffectOperand)
-        .getUse()
-        .(ReadSideEffectInstruction)
-        .getArgumentDef()
-        .getUnconvertedResultExpression()
+  /**
+   * Step to return value of a modeled function when an input taints the
+   * dereference of the return value.
+   */
+  cached
+  predicate additionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(CallInstruction call, Function func, FunctionInput modelIn, FunctionOutput modelOut |
+      n1.asOperand() = callInput(call, modelIn) and
+      (
+        func.(TaintFunction).hasTaintFlow(modelIn, modelOut)
+        or
+        func.(DataFlowFunction).hasDataFlow(modelIn, modelOut)
+      ) and
+      call.getStaticCallTarget() = func and
+      modelOut.isReturnValueDeref() and
+      call = n2.asInstruction()
+    )
+  }
 }
 
+private import Cached
+
 /**
  * Holds if `tainted` may contain taint from `source`.
  *
@@ -402,19 +429,7 @@ module TaintedWithPath {
         readsVariable(n2.asInstruction(), n1.asVariable().(GlobalOrNamespaceVariable))
       )
       or
-      // Step to return value of a modeled function when an input taints the
-      // dereference of the return value
-      exists(CallInstruction call, Function func, FunctionInput modelIn, FunctionOutput modelOut |
-        n1.asOperand() = callInput(call, modelIn) and
-        (
-          func.(TaintFunction).hasTaintFlow(modelIn, modelOut)
-          or
-          func.(DataFlowFunction).hasDataFlow(modelIn, modelOut)
-        ) and
-        call.getStaticCallTarget() = func and
-        modelOut.isReturnValueDeref() and
-        call = n2.asInstruction()
-      )
+      additionalTaintStep(n1, n2)
     }
 
     override predicate isSanitizer(DataFlow::Node node) {

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl4.qll

@@ -2,11 +2,14 @@ private import cpp
 private import semmle.code.cpp.ir.IR
 private import semmle.code.cpp.ir.dataflow.DataFlow
 private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate
+private import DataFlowImplCommon as DataFlowImplCommon
 
 /**
  * Gets a function that might be called by `call`.
  */
+cached
 Function viableCallable(CallInstruction call) {
+  DataFlowImplCommon::forceCachingInSameStage() and
   result = call.getStaticCallTarget()
   or
   // If the target of the call does not have a body in the snapshot, it might
@@ -43,7 +46,6 @@ private module VirtualDispatch {
     abstract DataFlow::Node getDispatchValue();
 
     /** Gets a candidate target for this call. */
-    cached
     abstract Function resolve();
 
     /**