Merge pull request github#3000 from asger-semmle/js/late-barrier-guards

Approved by erik-krogh

Author: semmle-qlci

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -149,7 +149,7 @@ abstract class Configuration extends string {
   predicate isBarrier(DataFlow::Node node) {
     exists(BarrierGuardNode guard |
       isBarrierGuardInternal(guard) and
-      guard.internalBlocks(node, "")
+      barrierGuardBlocksNode(guard, node, "")
     )
   }
 
@@ -183,7 +183,7 @@ abstract class Configuration extends string {
   predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
     exists(BarrierGuardNode guard |
       isBarrierGuardInternal(guard) and
-      guard.internalBlocks(node, lbl)
+      barrierGuardBlocksNode(guard, node, lbl)
     )
     or
     none() // relax type inference to account for overriding
@@ -199,6 +199,10 @@ abstract class Configuration extends string {
    */
   predicate isBarrierGuard(BarrierGuardNode guard) { none() }
 
+  /**
+   * Holds if `guard` is a barrier guard for this configuration, added through
+   * `isBarrierGuard` or `AdditionalBarrierGuardNode`.
+   */
   private predicate isBarrierGuardInternal(BarrierGuardNode guard) {
     isBarrierGuard(guard)
     or
@@ -319,44 +323,6 @@ module FlowLabel {
  * implementations of `blocks` will _both_ apply to any configuration that includes either of them.
  */
 abstract class BarrierGuardNode extends DataFlow::Node {
-  /**
-   * Holds if data flow node `nd` acts as a barrier for data flow, possibly due to aliasing
-   * through an access path.
-   *
-   * `label` is bound to the blocked label, or the empty string if all labels should be blocked.
-   *
-   * INTERNAL: this predicate should only be used from within `blocks(boolean, Expr)`.
-   */
-  predicate internalBlocks(DataFlow::Node nd, string label) {
-    // 1) `nd` is a use of a refinement node that blocks its input variable
-    exists(SsaRefinementNode ref, boolean outcome |
-      nd = DataFlow::ssaDefinitionNode(ref) and
-      outcome = ref.getGuard().(ConditionGuardNode).getOutcome() and
-      ssaRefinementBlocks(outcome, ref, label)
-    )
-    or
-    // 2) `nd` is an instance of an access path `p`, and dominated by a barrier for `p`
-    exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
-      nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and
-      getEnclosingExpr() = cond.getTest() and
-      outcome = cond.getOutcome() and
-      barrierGuardBlocksAccessPath(this, outcome, p, label) and
-      cond.dominates(bb)
-    )
-  }
-
-  /**
-   * Holds if there exists an input variable of `ref` that blocks the label `label`.
-   *
-   * This predicate is outlined to give the optimizer a hint about the join ordering.
-   */
-  private predicate ssaRefinementBlocks(boolean outcome, SsaRefinementNode ref, string label) {
-    getEnclosingExpr() = ref.getGuard().getTest() and
-    forex(SsaVariable input | input = ref.getAnInput() |
-      barrierGuardBlocksExpr(this, outcome, input.getAUse(), label)
-    )
-  }
-
   /**
    * Holds if this node blocks expression `e` provided it evaluates to `outcome`.
    *
@@ -387,6 +353,17 @@ private predicate barrierGuardBlocksExpr(
   guard.(AdditionalBarrierGuardCall).internalBlocksLabel(outcome, test, label)
 }
 
+/**
+ * Holds if `guard` blocks the flow of a value reachable through exploratory flow.
+ */
+pragma[noinline]
+private predicate barrierGuardIsRelevant(BarrierGuardNode guard) {
+  exists(Expr e |
+    barrierGuardBlocksExpr(guard, _, e, _) and
+    isRelevantForward(e.flow(), _)
+  )
+}
+
 /**
  * Holds if data flow node `nd` acts as a barrier for data flow due to aliasing through
  * an access path.
@@ -397,9 +374,50 @@ pragma[noinline]
 private predicate barrierGuardBlocksAccessPath(
   BarrierGuardNode guard, boolean outcome, AccessPath ap, string label
 ) {
+  barrierGuardIsRelevant(guard) and
   barrierGuardBlocksExpr(guard, outcome, ap.getAnInstance(), label)
 }
 
+/**
+ * Holds if there exists an input variable of `ref` that blocks the label `label`.
+ *
+ * This predicate is outlined to give the optimizer a hint about the join ordering.
+ */
+private predicate barrierGuardBlocksSsaRefinement(
+  BarrierGuardNode guard, boolean outcome, SsaRefinementNode ref, string label
+) {
+  barrierGuardIsRelevant(guard) and
+  guard.getEnclosingExpr() = ref.getGuard().getTest() and
+  forex(SsaVariable input | input = ref.getAnInput() |
+    barrierGuardBlocksExpr(guard, outcome, input.getAUse(), label)
+  )
+}
+
+/**
+ * Holds if data flow node `nd` acts as a barrier for data flow, possibly due to aliasing
+ * through an access path.
+ *
+ * `label` is bound to the blocked label, or the empty string if all labels should be blocked.
+ */
+private predicate barrierGuardBlocksNode(BarrierGuardNode guard, DataFlow::Node nd, string label) {
+  // 1) `nd` is a use of a refinement node that blocks its input variable
+  exists(SsaRefinementNode ref, boolean outcome |
+    nd = DataFlow::ssaDefinitionNode(ref) and
+    outcome = ref.getGuard().(ConditionGuardNode).getOutcome() and
+    barrierGuardBlocksSsaRefinement(guard, outcome, ref, label)
+  )
+  or
+  // 2) `nd` is an instance of an access path `p`, and dominated by a barrier for `p`
+  barrierGuardIsRelevant(guard) and
+  exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
+    nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and
+    guard.getEnclosingExpr() = cond.getTest() and
+    outcome = cond.getOutcome() and
+    barrierGuardBlocksAccessPath(guard, outcome, p, label) and
+    cond.dominates(bb)
+  )
+}
+
 /**
  * Holds if `guard` should block flow along the edge `pred -> succ`.
  *
@@ -408,6 +426,7 @@ private predicate barrierGuardBlocksAccessPath(
 private predicate barrierGuardBlocksEdge(
   BarrierGuardNode guard, DataFlow::Node pred, DataFlow::Node succ, string label
 ) {
+  barrierGuardIsRelevant(guard) and
   exists(
     SsaVariable input, SsaPhiNode phi, BasicBlock bb, ConditionGuardNode cond, boolean outcome
   |
@@ -569,11 +588,12 @@ private class FlowStepThroughImport extends AdditionalFlowStep, DataFlow::ValueN
 
 /**
  * Holds if there is a flow step from `pred` to `succ` described by `summary`
- * under configuration `cfg`.
+ * under configuration `cfg`, disregarding barriers.
  *
  * Summary steps through function calls are not taken into account.
  */
-private predicate basicFlowStep(
+pragma[inline]
+private predicate basicFlowStepNoBarrier(
   DataFlow::Node pred, DataFlow::Node succ, PathSummary summary, DataFlow::Configuration cfg
 ) {
   isLive() and
@@ -582,8 +602,7 @@ private predicate basicFlowStep(
     // Local flow
     exists(FlowLabel predlbl, FlowLabel succlbl |
       localFlowStep(pred, succ, cfg, predlbl, succlbl) and
-      not isLabeledBarrierEdge(cfg, pred, succ, predlbl) and
-      not isBarrierEdge(cfg, pred, succ) and
+      not cfg.isBarrierEdge(pred, succ) and
       summary = MkPathSummary(false, false, predlbl, succlbl)
     )
     or
@@ -605,6 +624,20 @@ private predicate basicFlowStep(
   )
 }
 
+/**
+ * Holds if there is a flow step from `pred` to `succ` described by `summary`
+ * under configuration `cfg`.
+ *
+ * Summary steps through function calls are not taken into account.
+ */
+private predicate basicFlowStep(
+  DataFlow::Node pred, DataFlow::Node succ, PathSummary summary, DataFlow::Configuration cfg
+) {
+  basicFlowStepNoBarrier(pred, succ, summary, cfg) and
+  not isLabeledBarrierEdge(cfg, pred, succ, summary.getStartLabel()) and
+  not isBarrierEdge(cfg, pred, succ)
+}
+
 /**
  * Holds if there is a flow step from `pred` to `succ` under configuration `cfg`,
  * including both basic flow steps and steps into/out of properties.
@@ -615,7 +648,7 @@ private predicate basicFlowStep(
 private predicate exploratoryFlowStep(
   DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg
 ) {
-  basicFlowStep(pred, succ, _, cfg) or
+  basicFlowStepNoBarrier(pred, succ, _, cfg) or
   basicStoreStep(pred, succ, _) or
   basicLoadStep(pred, succ, _) or
   isAdditionalStoreStep(pred, succ, _, cfg) or
@@ -1442,6 +1475,7 @@ private class BarrierGuardFunction extends Function {
   string label;
 
   BarrierGuardFunction() {
+    barrierGuardIsRelevant(guard) and
     exists(Expr e |
       exists(Expr returnExpr |
         returnExpr = guard.asExpr()