diff --git a/csharp/ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md b/csharp/ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md new file mode 100644 index 000000000000..a3c69932917d --- /dev/null +++ b/csharp/ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md @@ -0,0 +1,4 @@ +--- +category: deprecated +--- +* `ControlFlowElement.controlsBlock` has been deprecated in favor of the Guards library. diff --git a/csharp/ql/lib/semmle/code/csharp/Caching.qll b/csharp/ql/lib/semmle/code/csharp/Caching.qll index 50a789f79897..bbe310fe69e5 100644 --- a/csharp/ql/lib/semmle/code/csharp/Caching.qll +++ b/csharp/ql/lib/semmle/code/csharp/Caching.qll @@ -33,8 +33,6 @@ module Stages { cached private predicate forceCachingInSameStageRev() { - any(ControlFlowElement cfe).controlsBlock(_, _, _) - or exists(GuardedExpr ge) or forceCachingInSameStageRev() diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll index 784dab415f3e..0d0ed6819698 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll @@ -87,148 +87,20 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element { result.getAControlFlowNode() } - pragma[noinline] - private predicate immediatelyControlsBlockSplit0( - ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s - ) { - // Only calculate dominance by explicit recursion for split nodes; - // all other nodes can use regular CFG dominance - this instanceof Impl::SplitAstNode and - cb.getLastNode() = this.getAControlFlowNode() and - succ = cb.getASuccessor(s) - } - - pragma[noinline] - private predicate immediatelyControlsBlockSplit1( - ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s, BasicBlock pred, SuccessorType t - ) { - this.immediatelyControlsBlockSplit0(cb, succ, s) and - pred = succ.getAPredecessorByType(t) and - pred != cb - } - - pragma[noinline] - private predicate immediatelyControlsBlockSplit2( - ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s, BasicBlock pred, SuccessorType t - ) { - this.immediatelyControlsBlockSplit1(cb, succ, s, pred, t) and - ( - succ.dominates(pred) - or - // `pred` might be another split of this element - pred.getLastNode().getAstNode() = this and - t = s - ) - } - /** - * Holds if basic block `succ` is immediately controlled by this control flow - * element with conditional value `s`. That is, `succ` can only be reached from - * the callable entry point by going via the `s` edge out of *some* basic block - * `pred` ending with this element, and `pred` is an immediate predecessor - * of `succ`. - * - * Moreover, this control flow element corresponds to multiple control flow nodes, - * which is why - * - * ```ql - * exists(ConditionBlock cb | - * cb.getLastNode() = this.getAControlFlowNode() | - * cb.immediatelyControls(succ, s) - * ) - * ``` + * DEPRECATED: Use `Guard` class instead. * - * does not work. - * - * `cb` records all of the possible condition blocks for this control flow element - * that a path from the callable entry point to `succ` may go through. - */ - pragma[nomagic] - private predicate immediatelyControlsBlockSplit( - BasicBlock succ, ConditionalSuccessor s, ConditionBlock cb - ) { - this.immediatelyControlsBlockSplit0(cb, succ, s) and - forall(BasicBlock pred, SuccessorType t | - this.immediatelyControlsBlockSplit1(cb, succ, s, pred, t) - | - this.immediatelyControlsBlockSplit2(cb, succ, s, pred, t) - ) - } - - pragma[noinline] - private predicate controlsJoinBlockPredecessor( - JoinBlock controlled, ConditionalSuccessor s, int i, ConditionBlock cb - ) { - this.controlsBlockSplit(controlled.getJoinBlockPredecessor(i), s, cb) - } - - private predicate controlsJoinBlockSplit(JoinBlock controlled, ConditionalSuccessor s, int i) { - i = -1 and - this.controlsJoinBlockPredecessor(controlled, s, _, _) - or - this.controlsJoinBlockSplit(controlled, s, i - 1) and - ( - this.controlsJoinBlockPredecessor(controlled, s, i, _) - or - controlled.dominates(controlled.getJoinBlockPredecessor(i)) - ) - } - - cached - private predicate controlsBlockSplit( - BasicBlock controlled, ConditionalSuccessor s, ConditionBlock cb - ) { - Stages::GuardsStage::forceCachingInSameStage() and - this.immediatelyControlsBlockSplit(controlled, s, cb) - or - // Equivalent with - // - // ```ql - // exists(JoinBlockPredecessor pred | pred = controlled.getAPredecessor() | - // this.controlsBlockSplit(pred, s) - // ) and - // forall(JoinBlockPredecessor pred | pred = controlled.getAPredecessor() | - // this.controlsBlockSplit(pred, s) - // or - // controlled.dominates(pred) - // ) - // ``` - // - // but uses no universal recursion for better performance. - exists(int last | - last = max(int i | exists(controlled.(JoinBlock).getJoinBlockPredecessor(i))) - | - this.controlsJoinBlockSplit(controlled, s, last) - ) and - this.controlsJoinBlockPredecessor(controlled, s, _, cb) - or - not controlled instanceof JoinBlock and - this.controlsBlockSplit(controlled.getAPredecessor(), s, cb) - } - - /** * Holds if basic block `controlled` is controlled by this control flow element * with conditional value `s`. That is, `controlled` can only be reached from * the callable entry point by going via the `s` edge out of *some* basic block * ending with this element. * - * This predicate is different from - * - * ```ql - * exists(ConditionBlock cb | - * cb.getLastNode() = this.getAControlFlowNode() | - * cb.controls(controlled, s) - * ) - * ``` - * - * as control flow splitting is taken into account. - * * `cb` records all of the possible condition blocks for this control flow element * that a path from the callable entry point to `controlled` may go through. */ - predicate controlsBlock(BasicBlock controlled, ConditionalSuccessor s, ConditionBlock cb) { - this.controlsBlockSplit(controlled, s, cb) - or + deprecated predicate controlsBlock( + BasicBlock controlled, ConditionalSuccessor s, ConditionBlock cb + ) { cb.getLastNode() = this.getAControlFlowNode() and cb.edgeDominates(controlled, s) } diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll index 4914450dfc9a..040ea47f7859 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll @@ -49,8 +49,15 @@ private module GuardsInput implements override predicate isNull() { any() } } - private class BooleanConstant extends ConstantExpr instanceof BoolLiteral { - override boolean asBooleanValue() { result = super.getBoolValue() } + private predicate boolConst(Expr e, boolean b) { + e.getType() instanceof BoolType and + e.getValue() = b.toString() + } + + private class BooleanConstant extends ConstantExpr { + BooleanConstant() { boolConst(this, _) } + + override boolean asBooleanValue() { boolConst(this, result) } } private predicate intConst(Expr e, int i) { diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll index f4a76b2f5770..4f7f0141da2a 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll @@ -2583,10 +2583,10 @@ class NodeRegion instanceof ControlFlow::BasicBlock { * Holds if the nodes in `nr` are unreachable when the call context is `call`. */ predicate isUnreachableInCall(NodeRegion nr, DataFlowCall call) { - exists(ExplicitParameterNode paramNode, Guard guard, ControlFlow::BooleanSuccessor bs | - viableConstantBooleanParamArg(paramNode, bs.getValue().booleanNot(), call) and + exists(ExplicitParameterNode paramNode, Guard guard, GuardValue val | + viableConstantParamArg(paramNode, val.getDualValue(), call) and paramNode.getSsaDefinition().getARead() = guard and - guard.controlsBlock(nr, bs, _) + guard.valueControls(nr, val) ) } @@ -2904,33 +2904,19 @@ class CastNode extends Node { class DataFlowExpr = Expr; -/** Holds if `e` is an expression that always has the same Boolean value `val`. */ -private predicate constantBooleanExpr(Expr e, boolean val) { - e.getType() instanceof BoolType and - e.getValue() = val.toString() - or - exists(Ssa::ExplicitDefinition def, Expr src | - e = def.getARead() and - src = def.getADefinition().getSource() and - constantBooleanExpr(src, val) - ) -} +/** An argument that always has the same value. */ +private class ConstantArgumentNode extends ExprNode { + ConstantArgumentNode() { Guards::InternalUtil::exprHasValue(this.(ArgumentNode).asExpr(), _) } -/** An argument that always has the same Boolean value. */ -private class ConstantBooleanArgumentNode extends ExprNode { - ConstantBooleanArgumentNode() { constantBooleanExpr(this.(ArgumentNode).asExpr(), _) } - - /** Gets the Boolean value of this expression. */ - boolean getBooleanValue() { constantBooleanExpr(this.getExpr(), result) } + /** Gets the value of this expression. */ + GuardValue getValue() { Guards::InternalUtil::exprHasValue(this.getExpr(), result) } } pragma[noinline] -private predicate viableConstantBooleanParamArg( - ParameterNode paramNode, boolean b, DataFlowCall call -) { - exists(ConstantBooleanArgumentNode arg | +private predicate viableConstantParamArg(ParameterNode paramNode, GuardValue val, DataFlowCall call) { + exists(ConstantArgumentNode arg | viableParamArg(call, paramNode, arg) and - b = arg.getBooleanValue() + val = arg.getValue() ) } diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll index f2b46e4ebac1..53b44f873a64 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll @@ -72,17 +72,10 @@ class ReverseDnsSource extends Source { } } -pragma[noinline] -private predicate conditionControlsCall0( - SensitiveExecutionMethodCall call, Expr e, ControlFlow::BooleanSuccessor s -) { - forex(BasicBlock bb | bb = call.getAControlFlowNode().getBasicBlock() | e.controlsBlock(bb, s, _)) -} - private predicate conditionControlsCall( SensitiveExecutionMethodCall call, SensitiveExecutionMethod def, Expr e, boolean cond ) { - exists(ControlFlow::BooleanSuccessor s | cond = s.getValue() | conditionControlsCall0(call, e, s)) and + e.(Guard).directlyControls(call.getBasicBlock(), cond) and def = call.getTarget().getUnboundDeclaration() }