Ruby: Cache two more data flow predicates

Author: hvitved

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

@@ -232,6 +232,18 @@ private module Cached {
     )
   }
 
+  /** Gets a viable run-time target for the call `call`. */
+  cached
+  DataFlowCallable viableCallable(DataFlowCall call) {
+    result = TCfgScope(getTarget(call.asCall())) and
+    not call.asCall().getExpr() instanceof YieldCall // handled by `lambdaCreation`/`lambdaCall`
+    or
+    exists(LibraryCallable callable |
+      result = TLibraryCallable(callable) and
+      call.asCall().getExpr() = callable.getACall()
+    )
+  }
+
   cached
   newtype TArgumentPosition =
     TSelfArgumentPosition() or
@@ -423,17 +435,6 @@ private DataFlow::LocalSourceNode trackModule(Module tp) {
   result = trackModule(tp, TypeTracker::end())
 }
 
-/** Gets a viable run-time target for the call `call`. */
-DataFlowCallable viableCallable(DataFlowCall call) {
-  result = TCfgScope(getTarget(call.asCall())) and
-  not call.asCall().getExpr() instanceof YieldCall // handled by `lambdaCreation`/`lambdaCall`
-  or
-  exists(LibraryCallable callable |
-    result = TLibraryCallable(callable) and
-    call.asCall().getExpr() = callable.getACall()
-  )
-}
-
 /**
  * Holds if the set of viable implementations that can be called by `call`
  * might be improved by knowing the call context. This is the case if the

ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll

@@ -66,35 +66,53 @@ private CfgNodes::ExprNodes::VariableWriteAccessCfgNode variablesInPattern(
     )
 }
 
-/**
- * Holds if the additional step from `nodeFrom` to `nodeTo` should be included
- * in all global taint flow configurations.
- */
 cached
-predicate defaultAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-  // value of `case` expression into variables in patterns
-  exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::ExprNodes::InClauseCfgNode clause |
-    nodeFrom.asExpr() = case.getValue() and
-    clause = case.getBranch(_) and
-    nodeTo.(SsaDefinitionNode).getDefinition().getControlFlowNode() =
-      variablesInPattern(clause.getPattern())
-  )
-  or
-  // operation involving `nodeFrom`
-  exists(CfgNodes::ExprNodes::OperationCfgNode op |
-    op = nodeTo.asExpr() and
-    op.getAnOperand() = nodeFrom.asExpr() and
-    not op.getExpr() instanceof AssignExpr
-  )
-  or
-  // string interpolation of `nodeFrom` into `nodeTo`
-  nodeFrom.asExpr() =
-    nodeTo.asExpr().(CfgNodes::ExprNodes::StringlikeLiteralCfgNode).getAComponent()
-  or
-  FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, false)
-  or
-  // Although flow through arrays is modelled precisely using stores/reads, we still
-  // allow flow out of a _tainted_ array. This is needed in order to support taint-
-  // tracking configurations where the source is an array.
-  readStep(nodeFrom, any(DataFlow::Content::ArrayElementContent c), nodeTo)
+private module Cached {
+  /**
+   * Holds if the additional step from `nodeFrom` to `nodeTo` should be included
+   * in all global taint flow configurations.
+   */
+  cached
+  predicate defaultAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    // value of `case` expression into variables in patterns
+    exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::ExprNodes::InClauseCfgNode clause |
+      nodeFrom.asExpr() = case.getValue() and
+      clause = case.getBranch(_) and
+      nodeTo.(SsaDefinitionNode).getDefinition().getControlFlowNode() =
+        variablesInPattern(clause.getPattern())
+    )
+    or
+    // operation involving `nodeFrom`
+    exists(CfgNodes::ExprNodes::OperationCfgNode op |
+      op = nodeTo.asExpr() and
+      op.getAnOperand() = nodeFrom.asExpr() and
+      not op.getExpr() instanceof AssignExpr
+    )
+    or
+    // string interpolation of `nodeFrom` into `nodeTo`
+    nodeFrom.asExpr() =
+      nodeTo.asExpr().(CfgNodes::ExprNodes::StringlikeLiteralCfgNode).getAComponent()
+    or
+    FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, false)
+    or
+    // Although flow through arrays is modelled precisely using stores/reads, we still
+    // allow flow out of a _tainted_ array. This is needed in order to support taint-
+    // tracking configurations where the source is an array.
+    readStep(nodeFrom, any(DataFlow::Content::ArrayElementContent c), nodeTo)
+  }
+
+  /**
+   * Holds if taint propagates from `nodeFrom` to `nodeTo` in exactly one local
+   * (intra-procedural) step.
+   */
+  cached
+  predicate localTaintStepCached(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    defaultAdditionalTaintStep(nodeFrom, nodeTo)
+    or
+    // Simple flow through library code is included in the exposed local
+    // step relation, even though flow is technically inter-procedural
+    FlowSummaryImpl::Private::Steps::summaryThroughStep(nodeFrom, nodeTo, false)
+  }
 }
+
+import Cached

ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPublic.qll

@@ -20,14 +20,4 @@ predicate localExprTaint(CfgNodes::ExprCfgNode e1, CfgNodes::ExprCfgNode e2) {
   localTaint(DataFlow::exprNode(e1), DataFlow::exprNode(e2))
 }
 
-/**
- * Holds if taint propagates from `nodeFrom` to `nodeTo` in exactly one local
- * (intra-procedural) step.
- */
-predicate localTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-  defaultAdditionalTaintStep(nodeFrom, nodeTo)
-  or
-  // Simple flow through library code is included in the exposed local
-  // step relation, even though flow is technically inter-procedural
-  FlowSummaryImpl::Private::Steps::summaryThroughStep(nodeFrom, nodeTo, false)
-}
+predicate localTaintStep = localTaintStepCached/2;