Merge pull request #18696 from paldepind/shared-bb-dominates

paldepind · web-flow · commit c7412cc8394e · 2025-02-11T14:12:07.000+01:00
Shared: Use edge dominance terminology in basic block library
diff --git a/csharp/ql/lib/change-notes/2025-02-11-basic-block-rename.md b/csharp/ql/lib/change-notes/2025-02-11-basic-block-rename.md
@@ -0,0 +1,6 @@
+---
+category: deprecated
+---
+* The predicates `immediatelyControls` and `controls` on the `ConditionBlock`
+  class have been deprecated in favor of the newly added `dominatingEdge`
+  predicate.
diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/BasicBlocks.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/BasicBlocks.qll
@@ -202,6 +202,29 @@ final class BasicBlock extends BasicBlocksImpl::BasicBlock {
    */
   BasicBlock getImmediateDominator() { result = super.getImmediateDominator() }
 
+  /**
+   * Holds if the edge with successor type `s` out of this basic block is a
+   * dominating edge for `dominated`.
+   *
+   * That is, all paths reaching `dominated` from the entry point basic
+   * block must go through the `s` edge out of this basic block.
+   *
+   * Edge dominance is similar to node dominance except it concerns edges
+   * instead of nodes: A basic block is dominated by a _basic block_ `bb` if it
+   * can only be reached through `bb` and dominated by an _edge_ `e` if it can
+   * only be reached through `e`.
+   *
+   * Note that where all basic blocks (except the entry basic block) are
+   * strictly dominated by at least one basic block, a basic block may not be
+   * dominated by any edge. If an edge dominates a basic block `bb`, then
+   * both endpoints of the edge dominates `bb`. The converse is not the case,
+   * as there may be multiple paths between the endpoints with none of them
+   * dominating.
+   */
+  predicate edgeDominates(BasicBlock dominated, ControlFlow::SuccessorType s) {
+    super.edgeDominates(dominated, s)
+  }
+
   /**
    * Holds if this basic block strictly post-dominates basic block `bb`.
    *
@@ -296,11 +319,14 @@ final class JoinBlockPredecessor extends BasicBlock, BasicBlocksImpl::JoinPredec
  * control flow.
  */
 final class ConditionBlock extends BasicBlock, BasicBlocksImpl::ConditionBasicBlock {
-  predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
-    super.immediatelyControls(succ, s)
+  /** DEPRECATED: Use `edgeDominates` instead. */
+  deprecated predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
+    this.getASuccessor(s) = succ and
+    BasicBlocksImpl::dominatingEdge(this, succ)
   }
 
-  predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
-    super.controls(controlled, s)
+  /** DEPRECATED: Use `edgeDominates` instead. */
+  deprecated predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
+    super.edgeDominates(controlled, s)
   }
 }
diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll
@@ -225,6 +225,6 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
     this.controlsBlockSplit(controlled, s, cb)
     or
     cb.getLastNode() = this.getAControlFlowNode() and
-    cb.controls(controlled, s)
+    cb.edgeDominates(controlled, s)
   }
 }
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll
@@ -1119,7 +1119,7 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
     exists(ConditionBlock conditionBlock, ControlFlow::SuccessorTypes::ConditionalSuccessor s |
       guard.getAControlFlowNode() = conditionBlock.getLastNode() and
       s.getValue() = branch and
-      conditionBlock.controls(bb, s)
+      conditionBlock.edgeDominates(bb, s)
     )
   }
 
diff --git a/csharp/ql/test/library-tests/controlflow/graph/Condition.ql b/csharp/ql/test/library-tests/controlflow/graph/Condition.ql
@@ -4,7 +4,8 @@ import ControlFlow
 query predicate conditionBlock(
   BasicBlocks::ConditionBlock cb, BasicBlock controlled, boolean testIsTrue
 ) {
-  cb.controls(controlled, any(SuccessorTypes::ConditionalSuccessor s | testIsTrue = s.getValue()))
+  cb.edgeDominates(controlled,
+    any(SuccessorTypes::ConditionalSuccessor s | testIsTrue = s.getValue()))
 }
 
 ControlFlow::Node successor(ControlFlow::Node node, boolean kind) {
diff --git a/ruby/ql/lib/change-notes/2025-02-11-basic-block-rename.md b/ruby/ql/lib/change-notes/2025-02-11-basic-block-rename.md
@@ -0,0 +1,6 @@
+---
+category: deprecated
+---
+* The predicates `immediatelyControls` and `controls` on the `ConditionBlock`
+  class have been deprecated in favor of the newly added `dominatingEdge`
+  predicate.
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll
@@ -168,6 +168,29 @@ final class BasicBlock extends BasicBlocksImpl::BasicBlock {
    */
   BasicBlock getImmediateDominator() { result = super.getImmediateDominator() }
 
+  /**
+   * Holds if the edge with successor type `s` out of this basic block is a
+   * dominating edge for `dominated`.
+   *
+   * That is, all paths reaching `dominated` from the entry point basic
+   * block must go through the `s` edge out of this basic block.
+   *
+   * Edge dominance is similar to node dominance except it concerns edges
+   * instead of nodes: A basic block is dominated by a _basic block_ `bb` if it
+   * can only be reached through `bb` and dominated by an _edge_ `e` if it can
+   * only be reached through `e`.
+   *
+   * Note that where all basic blocks (except the entry basic block) are
+   * strictly dominated by at least one basic block, a basic block may not be
+   * dominated by any edge. If an edge dominates a basic block `bb`, then
+   * both endpoints of the edge dominates `bb`. The converse is not the case,
+   * as there may be multiple paths between the endpoints with none of them
+   * dominating.
+   */
+  predicate edgeDominates(BasicBlock dominated, SuccessorType s) {
+    super.edgeDominates(dominated, s)
+  }
+
   /**
    * Holds if this basic block strictly post-dominates basic block `bb`.
    *
@@ -248,21 +271,26 @@ final class JoinBlockPredecessor extends BasicBlock, BasicBlocksImpl::JoinPredec
  */
 final class ConditionBlock extends BasicBlock, BasicBlocksImpl::ConditionBasicBlock {
   /**
+   * DEPRECATED: Use `edgeDominates` instead.
+   *
    * Holds if basic block `succ` is immediately controlled by this basic
    * block with conditional value `s`. That is, `succ` is an immediate
    * successor of this block, and `succ` can only be reached from
    * the callable entry point by going via the `s` edge out of this basic block.
    */
-  predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
-    super.immediatelyControls(succ, s)
+  deprecated predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
+    this.getASuccessor(s) = succ and
+    BasicBlocksImpl::dominatingEdge(this, succ)
   }
 
   /**
+   * DEPRECATED: Use `edgeDominates` instead.
+   *
    * Holds if basic block `controlled` is controlled by this basic block with
    * conditional value `s`. That is, `controlled` can only be reached from the
    * callable entry point by going via the `s` edge out of this basic block.
    */
-  predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
-    super.controls(controlled, s)
+  deprecated predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
+    super.edgeDominates(controlled, s)
   }
 }
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll
@@ -6,6 +6,6 @@ predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean
   exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s |
     guard = conditionBlock.getLastNode() and
     s.getValue() = branch and
-    conditionBlock.controls(bb, s)
+    conditionBlock.edgeDominates(bb, s)
   )
 }
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
@@ -2387,7 +2387,7 @@ module TypeInference {
     |
       m = resolveConstantReadAccess(pattern.getExpr()) and
       cb.getLastNode() = pattern and
-      cb.controls(read.getBasicBlock(),
+      cb.edgeDominates(read.getBasicBlock(),
         any(SuccessorTypes::MatchingSuccessor match | match.getValue() = true)) and
       caseRead = def.getARead() and
       read = def.getARead() and
diff --git a/ruby/ql/lib/codeql/ruby/security/ConditionalBypassCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/ConditionalBypassCustomizations.qll
@@ -47,7 +47,7 @@ module ConditionalBypass {
 
     SensitiveActionGuardConditional() {
       exists(ConditionBlock cb, BasicBlock controlled |
-        cb.controls(controlled, _) and
+        cb.edgeDominates(controlled, _) and
         controlled.getANode() = action.asExpr() and
         cb.getLastNode() = this.asExpr()
       )
diff --git a/ruby/ql/test/library-tests/controlflow/graph/BasicBlocks.ql b/ruby/ql/test/library-tests/controlflow/graph/BasicBlocks.ql
@@ -10,8 +10,8 @@ query predicate immediateDominator(BasicBlock bb1, BasicBlock bb2) {
   bb1.getImmediateDominator() = bb2
 }
 
-query predicate controls(ConditionBlock bb1, BasicBlock bb2, SuccessorType t) {
-  bb1.controls(bb2, t)
+query predicate controls(ConditionBlock bb1, BasicBlock bb2, SuccessorTypes::ConditionalSuccessor t) {
+  bb1.edgeDominates(bb2, t)
 }
 
 query predicate successor(ConditionBlock bb1, BasicBlock bb2, SuccessorType t) {
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql
@@ -14,7 +14,7 @@ query predicate newStyleBarrierGuards(DataFlow::Node n) {
 
 query predicate controls(CfgNode condition, BasicBlock bb, SuccessorTypes::ConditionalSuccessor s) {
   exists(ConditionBlock cb |
-    cb.controls(bb, s) and
+    cb.edgeDominates(bb, s) and
     condition = cb.getLastNode()
   )
 }
diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll
@@ -456,7 +456,7 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
     exists(ConditionBasicBlock conditionBlock, ConditionalSuccessor s |
       guard = conditionBlock.getLastNode() and
       s.getValue() = branch and
-      conditionBlock.controls(bb, s)
+      conditionBlock.edgeDominates(bb, s)
     )
   }
 
diff --git a/rust/ql/test/library-tests/controlflow/BasicBlocks.ql b/rust/ql/test/library-tests/controlflow/BasicBlocks.ql
@@ -11,7 +11,7 @@ query predicate immediateDominator(BasicBlock bb1, BasicBlock bb2) {
 }
 
 query predicate controls(ConditionBasicBlock bb1, BasicBlock bb2, SuccessorType t) {
-  bb1.controls(bb2, t)
+  bb1.edgeDominates(bb2, t)
 }
 
 query predicate successor(ConditionBasicBlock bb1, BasicBlock bb2, SuccessorType t) {
diff --git a/shared/controlflow/codeql/controlflow/BasicBlock.qll b/shared/controlflow/codeql/controlflow/BasicBlock.qll
@@ -169,90 +169,59 @@ module Make<LocationSig Location, InputSig<Location> Input> {
     BasicBlock getImmediateDominator() { bbIDominates(result, this) }
 
     /**
-     * Holds if basic block `succ` is immediately controlled by this basic
-     * block with successor type `s`.
+     * Holds if the edge with successor type `s` out of this basic block is a
+     * dominating edge for `dominated`.
      *
-     * That is, `succ` is an immediate successor of this block, and `succ` can
-     * only be reached from the entry block by going via the `s` edge out of
-     * this basic block.
-     */
-    pragma[nomagic]
-    predicate immediatelyControls(BasicBlock succ, SuccessorType s) {
-      succ = this.getASuccessor(s) and
-      bbIDominates(this, succ) and
-      // The above is not sufficient to ensure that `succ` can only be reached
-      // through `s`. To see why, consider this example corresponding to an
-      // `if` statement without an `else` block and whe `A` is the basic block
-      // following the `if` statement:
-      // ```
-      // ... --> cond --[true]--> ... --> A
-      //           \                      /
-      //            ----[false]-----------
-      // ```
-      // Here `A` is a direct successor of `cond` along the `false` edge and it
-      // is immediately dominated by `cond`, but `A` is not controlled by the
-      // `false` edge since it is also possible to reach `A` via the `true`
-      // edge.
-      //
-      // Note that the first and third conjunct implies the second. But
-      // explicitly including the second conjunct leads to a better join order.
-      forall(BasicBlock pred | pred = succ.getAPredecessor() and pred != this |
-        succ.dominates(pred)
-      )
-    }
-
-    /**
-     * Holds if basic block `controlled` is controlled by this basic block with
-     * successor type `s`.
-     *
-     * That is, all paths reaching `controlled` from the entry point basic
+     * That is, all paths reaching `dominated` from the entry point basic
      * block must go through the `s` edge out of this basic block.
      *
-     * Control is similar to dominance except it concerns edges instead of
-     * nodes: A basic block is _dominated_ by a _basic block_ `bb` if it can
-     * only be reached through `bb` and _controlled_ by an _edge_ `s` if it can
-     * only be reached through `s`.
+     * Edge dominance is similar to node dominance except it concerns edges
+     * instead of nodes: A basic block is dominated by a _basic block_ `bb` if
+     * it can only be reached through `bb` and dominated by an _edge_ `e` if it
+     * can only be reached through `e`.
      *
      * Note that where all basic blocks (except the entry basic block) are
      * strictly dominated by at least one basic block, a basic block may not be
-     * controlled by any edge. If an edge controls a basic block `bb`, then
+     * dominated by any edge. If an edge dominates a basic block `bb`, then
      * both endpoints of the edge dominates `bb`. The converse is not the case,
      * as there may be multiple paths between the endpoints with none of them
      * dominating.
      */
-    predicate controls(BasicBlock controlled, SuccessorType s) {
-      // For this block to control the block `controlled` with `s` the following must be true:
-      // 1/ Execution must have passed through the test i.e. `this` must strictly dominate `controlled`.
+    predicate edgeDominates(BasicBlock dominated, SuccessorType s) {
+      // For this block to control the block `dominated` with `s` the following must be true:
+      // 1/ Execution must have passed through the test i.e. `this` must strictly dominate `dominated`.
       // 2/ Execution must have passed through the `s` edge leaving `this`.
       //
-      // Although "passed through the `s` edge" implies that `this.getASuccessor(s)` dominates `controlled`,
+      // Although "passed through the `s` edge" implies that `this.getASuccessor(s)` dominates `dominated`,
       // the reverse is not true, as flow may have passed through another edge to get to `this.getASuccessor(s)`
-      // so we need to assert that `this.getASuccessor(s)` dominates `controlled` *and* that
+      // so we need to assert that `this.getASuccessor(s)` dominates `dominated` *and* that
       // all predecessors of `this.getASuccessor(s)` are either `this` or dominated by `this.getASuccessor(s)`.
       //
       // For example, in the following C# snippet:
       // ```csharp
       // if (x)
-      //   controlled;
+      //   dominated;
       // false_successor;
       // uncontrolled;
       // ```
       // `false_successor` dominates `uncontrolled`, but not all of its predecessors are `this` (`if (x)`)
       //  or dominated by itself. Whereas in the following code:
       // ```csharp
       // if (x)
-      //   while (controlled)
+      //   while (dominated)
       //     also_controlled;
       // false_successor;
       // uncontrolled;
       // ```
-      // the block `while controlled` is controlled because all of its predecessors are `this` (`if (x)`)
+      // the block `while dominated` is dominated because all of its predecessors are `this` (`if (x)`)
       // or (in the case of `also_controlled`) dominated by itself.
       //
       // The additional constraint on the predecessors of the test successor implies
-      // that `this` strictly dominates `controlled` so that isn't necessary to check
+      // that `this` strictly dominates `dominated` so that isn't necessary to check
       // directly.
-      exists(BasicBlock succ | this.immediatelyControls(succ, s) | succ.dominates(controlled))
+      exists(BasicBlock succ |
+        succ = this.getASuccessor(s) and dominatingEdge(this, succ) and succ.dominates(dominated)
+      )
     }
 
     /**
@@ -282,6 +251,38 @@ module Make<LocationSig Location, InputSig<Location> Input> {
     string toString() { result = this.getFirstNode().toString() }
   }
 
+  /**
+   * Holds if `bb1` has `bb2` as a direct successor and the edge between `bb1`
+   * and `bb2` is a dominating edge.
+   *
+   * An edge `(bb1, bb2)` is dominating if there exists a basic block that can
+   * only be reached from the entry block by going through `(bb1, bb2)`. This
+   * implies that `(bb1, bb2)` dominates its endpoint `bb2`. I.e., `bb2` can
+   * only be reached from the entry block by going via `(bb1, bb2)`.
+   */
+  pragma[nomagic]
+  predicate dominatingEdge(BasicBlock bb1, BasicBlock bb2) {
+    bb1.getASuccessor(_) = bb2 and
+    bbIDominates(bb1, bb2) and
+    // The above is not sufficient to ensure that `bb2` can only be reached
+    // through `(bb1, bb2)`. To see why, consider this example corresponding to
+    // an `if` statement without an `else` block where `A` is the basic block
+    // following the `if` statement:
+    // ```
+    // ... --> cond --[true]--> ... --> A
+    //           \                      ^
+    //            ----[false]-----------
+    // ```
+    // Here `A` is a direct successor of `cond` along the `false` edge and it
+    // is immediately dominated by `cond`, but `A` is not dominated by the
+    // `false` edge since it is also possible to reach `A` via the `true`
+    // edge.
+    //
+    // Note that the first and third conjunct implies the second. But
+    // explicitly including the second conjunct leads to a better join order.
+    forall(BasicBlock pred | pred = bb2.getAPredecessor() and pred != bb1 | bb2.dominates(pred))
+  }
+
   cached
   private module Cached {
     /**
diff --git a/shared/controlflow/codeql/controlflow/Cfg.qll b/shared/controlflow/codeql/controlflow/Cfg.qll
@@ -1594,6 +1594,8 @@ module MakeWithSplitting<
 
     final class BasicBlock = BasicBlockImpl::BasicBlock;
 
+    predicate dominatingEdge = BasicBlockImpl::dominatingEdge/2;
+
     /**
      * An entry basic block, that is, a basic block whose first node is
      * an entry node.
diff --git a/swift/ql/lib/change-notes/2025-02-11-basic-block-rename.md b/swift/ql/lib/change-notes/2025-02-11-basic-block-rename.md
@@ -0,0 +1,6 @@
+---
+category: deprecated
+---
+* The predicates `immediatelyControls` and `controls` on the `ConditionBlock`
+  class have been deprecated in favor of the newly added `dominatingEdge`
+  predicate.
diff --git a/swift/ql/lib/codeql/swift/controlflow/BasicBlocks.qll b/swift/ql/lib/codeql/swift/controlflow/BasicBlocks.qll
diff --git a/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll

Original file line number	Diff line number	Diff line change
`@@ -225,6 +225,6 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {`
`225`	`225`	`this.controlsBlockSplit(controlled, s, cb)`
`226`	`226`	`or`
`227`	`227`	`cb.getLastNode() = this.getAControlFlowNode() and`
`228`		`- cb.controls(controlled, s)`
	`228`	`+ cb.edgeDominates(controlled, s)`
`229`	`229`	`}`
`230`	`230`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1119,7 +1119,7 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu`
`1119`	`1119`	`exists(ConditionBlock conditionBlock, ControlFlow::SuccessorTypes::ConditionalSuccessor s \|`
`1120`	`1120`	`guard.getAControlFlowNode() = conditionBlock.getLastNode() and`
`1121`	`1121`	`s.getValue() = branch and`
`1122`		`- conditionBlock.controls(bb, s)`
	`1122`	`+ conditionBlock.edgeDominates(bb, s)`
`1123`	`1123`	`)`
`1124`	`1124`	`}`
`1125`	`1125`
Original file line number	Diff line number	Diff line change
`@@ -4,7 +4,8 @@ import ControlFlow`
`4`	`4`	`query predicate conditionBlock(`
`5`	`5`	`BasicBlocks::ConditionBlock cb, BasicBlock controlled, boolean testIsTrue`
`6`	`6`	`) {`
`7`		`- cb.controls(controlled, any(SuccessorTypes::ConditionalSuccessor s \| testIsTrue = s.getValue()))`
	`7`	`+ cb.edgeDominates(controlled,`
	`8`	`+ any(SuccessorTypes::ConditionalSuccessor s \| testIsTrue = s.getValue()))`
`8`	`9`	`}`
`9`	`10`
`10`	`11`	`ControlFlow::Node successor(ControlFlow::Node node, boolean kind) {`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,6 @@ predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean`
`6`	`6`	`exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s \|`
`7`	`7`	`guard = conditionBlock.getLastNode() and`
`8`	`8`	`s.getValue() = branch and`
`9`		`- conditionBlock.controls(bb, s)`
	`9`	`+ conditionBlock.edgeDominates(bb, s)`
`10`	`10`	`)`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2387,7 +2387,7 @@ module TypeInference {`
`2387`	`2387`	`\|`
`2388`	`2388`	`m = resolveConstantReadAccess(pattern.getExpr()) and`
`2389`	`2389`	`cb.getLastNode() = pattern and`
`2390`		`- cb.controls(read.getBasicBlock(),`
	`2390`	`+ cb.edgeDominates(read.getBasicBlock(),`
`2391`	`2391`	`any(SuccessorTypes::MatchingSuccessor match \| match.getValue() = true)) and`
`2392`	`2392`	`caseRead = def.getARead() and`
`2393`	`2393`	`read = def.getARead() and`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ module ConditionalBypass {`
`47`	`47`
`48`	`48`	`SensitiveActionGuardConditional() {`
`49`	`49`	`exists(ConditionBlock cb, BasicBlock controlled \|`
`50`		`- cb.controls(controlled, _) and`
	`50`	`+ cb.edgeDominates(controlled, _) and`
`51`	`51`	`controlled.getANode() = action.asExpr() and`
`52`	`52`	`cb.getLastNode() = this.asExpr()`
`53`	`53`	`)`