Merge pull request #9195 from aschackmull/java/perf-local-flow

Java: Performance fixes for local flow relation

Author: aschackmull

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

@@ -769,7 +769,7 @@ private module Cached {
     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, true)
+    FlowSummaryImpl::Private::Steps::summaryThroughStepValue(nodeFrom, nodeTo)
   }
 
   cached

csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll

@@ -791,24 +791,37 @@ module Private {
     private ParamNode summaryArgParam(ArgNode arg, ReturnNodeExt ret, OutNodeExt out) {
       exists(DataFlowCall call, ReturnKindExt rk |
         result = summaryArgParam0(call, arg) and
-        pragma[only_bind_out](ret).getKind() = pragma[only_bind_into](rk) and
+        ret.getKind() = pragma[only_bind_into](rk) and
         out = pragma[only_bind_into](rk).getAnOutNode(call)
       )
     }
 
     /**
-     * Holds if `arg` flows to `out` using a simple flow summary, that is, a flow
-     * summary without reads and stores.
+     * Holds if `arg` flows to `out` using a simple value-preserving flow
+     * summary, that is, a flow summary without reads and stores.
      *
      * NOTE: This step should not be used in global data-flow/taint-tracking, but may
      * be useful to include in the exposed local data-flow/taint-tracking relations.
      */
-    predicate summaryThroughStep(ArgNode arg, Node out, boolean preservesValue) {
-      exists(ReturnNodeExt ret |
-        summaryLocalStep(summaryArgParam(arg, ret, out), ret, preservesValue)
+    predicate summaryThroughStepValue(ArgNode arg, Node out) {
+      exists(ReturnKind rk, ReturnNode ret, DataFlowCall call |
+        summaryLocalStep(summaryArgParam0(call, arg), ret, true) and
+        ret.getKind() = pragma[only_bind_into](rk) and
+        out = getAnOutNode(call, pragma[only_bind_into](rk))
       )
     }
 
+    /**
+     * Holds if `arg` flows to `out` using a simple flow summary involving taint
+     * step, that is, a flow summary without reads and stores.
+     *
+     * NOTE: This step should not be used in global data-flow/taint-tracking, but may
+     * be useful to include in the exposed local data-flow/taint-tracking relations.
+     */
+    predicate summaryThroughStepTaint(ArgNode arg, Node out) {
+      exists(ReturnNodeExt ret | summaryLocalStep(summaryArgParam(arg, ret, out), ret, false))
+    }
+
     /**
      * Holds if there is a read(+taint) of `c` from `arg` to `out` using a
      * flow summary.

csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll

@@ -117,7 +117,7 @@ private module Cached {
     (
       // 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)
+      FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(nodeFrom, nodeTo)
       or
       // Taint collection by adding a tainted element
       exists(DataFlow::ElementContent c |

csharp/ql/test/library-tests/dataflow/external-models/steps.ql

@@ -31,7 +31,9 @@ class SummaryModelTest extends SummaryModelCsv {
 query predicate summaryThroughStep(
   DataFlow::Node node1, DataFlow::Node node2, boolean preservesValue
 ) {
-  FlowSummaryImpl::Private::Steps::summaryThroughStep(node1, node2, preservesValue)
+  FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2) and preservesValue = true
+  or
+  FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(node1, node2) and preservesValue = false
 }
 
 query predicate summaryGetterStep(DataFlow::Node arg, DataFlow::Node out, Content c) {

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -75,7 +75,9 @@ private module ThisFlow {
  * local (intra-procedural) steps.
  */
 pragma[inline]
-predicate localFlow(Node node1, Node node2) { localFlowStep*(node1, node2) }
+predicate localFlow(Node node1, Node node2) { node1 = node2 or localFlowStepPlus(node1, node2) }
+
+private predicate localFlowStepPlus(Node node1, Node node2) = fastTC(localFlowStep/2)(node1, node2)
 
 /**
  * Holds if data can flow from `e1` to `e2` in zero or more
@@ -97,27 +99,43 @@ predicate hasNonlocalValue(FieldRead fr) {
   )
 }
 
-/**
- * Holds if data can flow from `node1` to `node2` in one local step.
- */
-predicate localFlowStep(Node node1, Node node2) {
-  simpleLocalFlowStep(node1, node2)
-  or
-  adjacentUseUse(node1.asExpr(), node2.asExpr())
-  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(node1, node2, true)
+cached
+private module Cached {
+  /**
+   * Holds if data can flow from `node1` to `node2` in one local step.
+   */
+  cached
+  predicate localFlowStep(Node node1, Node node2) {
+    simpleLocalFlowStep0(node1, node2)
+    or
+    adjacentUseUse(node1.asExpr(), node2.asExpr())
+    or
+    // Simple flow through library code is included in the exposed local
+    // step relation, even though flow is technically inter-procedural
+    FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2)
+  }
+
+  /**
+   * INTERNAL: do not use.
+   *
+   * 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 node1, Node node2) {
+    simpleLocalFlowStep0(node1, node2)
+    or
+    any(AdditionalValueStep a).step(node1, node2) and
+    pragma[only_bind_out](node1.getEnclosingCallable()) =
+      pragma[only_bind_out](node2.getEnclosingCallable()) and
+    // prevent recursive call
+    (any(AdditionalValueStep a).step(_, _) implies any())
+  }
 }
 
-/**
- * INTERNAL: do not use.
- *
- * 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 node1, Node node2) {
+import Cached
+
+private predicate simpleLocalFlowStep0(Node node1, Node node2) {
   TaintTrackingUtil::forceCachingInSameStage() and
   // Variable flow steps through adjacent def-use and use-use pairs.
   exists(SsaExplicitUpdate upd |
@@ -166,10 +184,6 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
   )
   or
   FlowSummaryImpl::Private::Steps::summaryLocalStep(node1, node2, true)
-  or
-  any(AdditionalValueStep a).step(node1, node2) and
-  pragma[only_bind_out](node1.getEnclosingCallable()) =
-    pragma[only_bind_out](node2.getEnclosingCallable())
 }
 
 private newtype TContent =

java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll

@@ -791,24 +791,37 @@ module Private {
     private ParamNode summaryArgParam(ArgNode arg, ReturnNodeExt ret, OutNodeExt out) {
       exists(DataFlowCall call, ReturnKindExt rk |
         result = summaryArgParam0(call, arg) and
-        pragma[only_bind_out](ret).getKind() = pragma[only_bind_into](rk) and
+        ret.getKind() = pragma[only_bind_into](rk) and
         out = pragma[only_bind_into](rk).getAnOutNode(call)
       )
     }
 
     /**
-     * Holds if `arg` flows to `out` using a simple flow summary, that is, a flow
-     * summary without reads and stores.
+     * Holds if `arg` flows to `out` using a simple value-preserving flow
+     * summary, that is, a flow summary without reads and stores.
      *
      * NOTE: This step should not be used in global data-flow/taint-tracking, but may
      * be useful to include in the exposed local data-flow/taint-tracking relations.
      */
-    predicate summaryThroughStep(ArgNode arg, Node out, boolean preservesValue) {
-      exists(ReturnNodeExt ret |
-        summaryLocalStep(summaryArgParam(arg, ret, out), ret, preservesValue)
+    predicate summaryThroughStepValue(ArgNode arg, Node out) {
+      exists(ReturnKind rk, ReturnNode ret, DataFlowCall call |
+        summaryLocalStep(summaryArgParam0(call, arg), ret, true) and
+        ret.getKind() = pragma[only_bind_into](rk) and
+        out = getAnOutNode(call, pragma[only_bind_into](rk))
       )
     }
 
+    /**
+     * Holds if `arg` flows to `out` using a simple flow summary involving taint
+     * step, that is, a flow summary without reads and stores.
+     *
+     * NOTE: This step should not be used in global data-flow/taint-tracking, but may
+     * be useful to include in the exposed local data-flow/taint-tracking relations.
+     */
+    predicate summaryThroughStepTaint(ArgNode arg, Node out) {
+      exists(ReturnNodeExt ret | summaryLocalStep(summaryArgParam(arg, ret, out), ret, false))
+    }
+
     /**
      * Holds if there is a read(+taint) of `c` from `arg` to `out` using a
      * flow summary.

java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

@@ -51,7 +51,7 @@ private module Cached {
     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(src, sink, false)
+    FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(src, sink)
     or
     // Treat container flow as taint for the local taint flow relation
     exists(DataFlow::Content c | containerContent(c) |

java/ql/test/library-tests/dataflow/external-models/steps.ql

@@ -22,5 +22,5 @@ class SummaryModelTest extends SummaryModelCsv {
 }
 
 from DataFlow::Node node1, DataFlow::Node node2
-where FlowSummaryImpl::Private::Steps::summaryThroughStep(node1, node2, false)
+where FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(node1, node2)
 select node1, node2

java/ql/test/library-tests/dataflow/local-additional-taint/localAdditionalTaintStep.ql

@@ -16,7 +16,7 @@ from DataFlow::Node src, DataFlow::Node sink
 where
   (
     localAdditionalTaintStep(src, sink) or
-    FlowSummaryImpl::Private::Steps::summaryThroughStep(src, sink, false)
+    FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(src, sink)
   ) and
   not FlowSummaryImpl::Private::Steps::summaryLocalStep(src, sink, false) and
   not FlowSummaryImpl::Private::Steps::summaryReadStep(src, _, sink) and

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

@@ -272,7 +272,7 @@ private module Cached {
     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, true)
+    FlowSummaryImpl::Private::Steps::summaryThroughStepValue(nodeFrom, nodeTo)
   }
 
   /** This is the local flow predicate that is used in type tracking. */