diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/BasicBlocks.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/BasicBlocks.qll index eb375dc4450e6..7bd1a83fe882b 100644 --- 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_ `s` if it can + * only be reached through `s`. + * + * 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,13 @@ 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 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 index 9fb450cd56d3f..5649f43b8818c 100644 --- 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 index 00665d836e1d9..4921bf5b13a2a 100644 --- 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 index 4cb815016c4cc..25de07e9d5b52 100644 --- 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/codeql/ruby/controlflow/BasicBlocks.qll b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll index 6ca386ce0e632..8b9e958742c63 100644 --- 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_ `s` if it can + * only be reached through `s`. + * + * 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`. * @@ -253,16 +276,19 @@ final class ConditionBlock extends BasicBlock, BasicBlocksImpl::ConditionBasicBl * 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 index c240595161c77..1161a061581cc 100644 --- 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 index 4c0adc95f25c3..ca5b661181bbb 100644 --- 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 index 45fa579425094..a64cd1039c174 100644 --- 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 index fb265997ab319..e7879fa600ae4 100644 --- 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/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll index 389539d9d8eca..3dfb0f951a4a2 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll @@ -491,7 +491,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 index 28083ef147851..2d072fa5b7cf2 100644 --- 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 index 57f6a310d182c..1deb7cb429253 100644 --- a/shared/controlflow/codeql/controlflow/BasicBlock.qll +++ b/shared/controlflow/codeql/controlflow/BasicBlock.qll @@ -169,71 +169,38 @@ module Make 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_ `s` if it + * can only be reached through `s`. * * 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; // ``` @@ -241,18 +208,20 @@ module Make Input> { // 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 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 `bb1` can only be reached + // through `(bb1, bb2)`. 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 = 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 index 3b9f00c669a1d..9b5922598c963 100644 --- 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/codeql/swift/controlflow/BasicBlocks.qll b/swift/ql/lib/codeql/swift/controlflow/BasicBlocks.qll index b0094045113c1..ab52c9f0d6816 100644 --- a/swift/ql/lib/codeql/swift/controlflow/BasicBlocks.qll +++ b/swift/ql/lib/codeql/swift/controlflow/BasicBlocks.qll @@ -44,6 +44,10 @@ final class BasicBlock extends BasicBlocksImpl::BasicBlock { BasicBlock getImmediateDominator() { result = super.getImmediateDominator() } + predicate edgeDominates(BasicBlock dominated, SuccessorType s) { + super.edgeDominates(dominated, s) + } + predicate strictlyPostDominates(BasicBlock bb) { super.strictlyPostDominates(bb) } predicate postDominates(BasicBlock bb) { super.postDominates(bb) } @@ -89,16 +93,19 @@ final class ConditionBlock extends BasicBlock, BasicBlocksImpl::ConditionBasicBl * 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/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll index 541607aaa436b..72608f50fe66a 100644 --- a/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll @@ -112,7 +112,7 @@ private class DefaultPathInjectionBarrier extends PathInjectionBarrier { bb.getANode().getNode().asAstNode().(IfStmt).getACondition() = getImmediateParent*(starts) and b.getValue() = true | - bb.controls(this.getCfgNode().getBasicBlock(), b) + bb.edgeDominates(this.getCfgNode().getBasicBlock(), b) ) ) }