diff --git a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExprSpecific.qll b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExprSpecific.qll index 1b83ae959d2c..224f968ce693 100644 --- a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExprSpecific.qll +++ b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExprSpecific.qll @@ -264,10 +264,6 @@ module SemanticExprConfig { Guard comparisonGuard(Expr e) { getSemanticExpr(result) = e } - predicate implies_v2(Guard g1, boolean b1, Guard g2, boolean b2) { - none() // TODO - } - /** Gets the expression associated with `instr`. */ SemExpr getSemanticExpr(IR::Instruction instr) { result = instr } } diff --git a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticGuard.qll b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticGuard.qll index 14755d59f5b4..a28d03a0666c 100644 --- a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticGuard.qll +++ b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticGuard.qll @@ -18,11 +18,11 @@ class SemGuard instanceof Specific::Guard { Specific::equalityGuard(this, e1, e2, polarity) } - final predicate directlyControls(SemBasicBlock controlled, boolean branch) { + final predicate controls(SemBasicBlock controlled, boolean branch) { Specific::guardDirectlyControlsBlock(this, controlled, branch) } - final predicate hasBranchEdge(SemBasicBlock bb1, SemBasicBlock bb2, boolean branch) { + final predicate controlsBranchEdge(SemBasicBlock bb1, SemBasicBlock bb2, boolean branch) { Specific::guardHasBranchEdge(this, bb1, bb2, branch) } @@ -31,8 +31,4 @@ class SemGuard instanceof Specific::Guard { final SemExpr asExpr() { result = Specific::getGuardAsExpr(this) } } -predicate semImplies_v2(SemGuard g1, boolean b1, SemGuard g2, boolean b2) { - Specific::implies_v2(g1, b1, g2, b2) -} - SemGuard semGetComparisonGuard(SemRelationalExpr e) { result = Specific::comparisonGuard(e) } diff --git a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/RangeAnalysisImpl.qll b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/RangeAnalysisImpl.qll index 22acb6fc1ca8..0281037e2795 100644 --- a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/RangeAnalysisImpl.qll +++ b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/RangeAnalysisImpl.qll @@ -77,8 +77,6 @@ module Sem implements Semantic { class Guard = SemGuard; - predicate implies_v2 = semImplies_v2/4; - class Type = SemType; class IntegerType = SemIntegerType; diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/ModulusAnalysis.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/ModulusAnalysis.qll index 5b2a39ad6c9e..3e5a45da247d 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/ModulusAnalysis.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/ModulusAnalysis.qll @@ -17,7 +17,7 @@ private predicate valueFlowStepSsa(SsaVariable v, SsaReadPosition pos, Expr e, i exists(Guard guard, boolean testIsTrue | pos.hasReadOfVar(v) and guard = eqFlowCond(v, e, delta, true, testIsTrue) and - guardDirectlyControlsSsaRead(guard, pos, testIsTrue) + guardControlsSsaRead(guard, pos, testIsTrue) ) } diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll index 4a9c3cdbe6d5..c9c3a937ef9e 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll @@ -32,8 +32,6 @@ module Private { class LeftShiftExpr = RU::ExprNode::LeftShiftExpr; - predicate guardDirectlyControlsSsaRead = RU::guardControlsSsaRead/3; - predicate guardControlsSsaRead = RU::guardControlsSsaRead/3; predicate valueFlowStep = RU::valueFlowStep/3; diff --git a/java/ql/lib/semmle/code/java/controlflow/Guards.qll b/java/ql/lib/semmle/code/java/controlflow/Guards.qll index 5cedb97ec05e..4042e7b29624 100644 --- a/java/ql/lib/semmle/code/java/controlflow/Guards.qll +++ b/java/ql/lib/semmle/code/java/controlflow/Guards.qll @@ -145,7 +145,7 @@ private predicate isNonFallThroughPredecessor(SwitchCase sc, ControlFlowNode pre * Evaluating a switch case to true corresponds to taking that switch case, and * evaluating it to false corresponds to taking some other branch. */ -class Guard extends ExprParent { +final class Guard extends ExprParent { Guard() { this.(Expr).getType() instanceof BooleanType and not this instanceof BooleanLiteral or @@ -360,6 +360,18 @@ private predicate guardControls_v3(Guard guard, BasicBlock controlled, boolean b ) } +pragma[nomagic] +private predicate guardControlsBranchEdge_v2( + Guard guard, BasicBlock bb1, BasicBlock bb2, boolean branch +) { + guard.hasBranchEdge(bb1, bb2, branch) + or + exists(Guard g, boolean b | + guardControlsBranchEdge_v2(g, bb1, bb2, b) and + implies_v2(g, b, guard, branch) + ) +} + pragma[nomagic] private predicate guardControlsBranchEdge_v3( Guard guard, BasicBlock bb1, BasicBlock bb2, boolean branch @@ -372,6 +384,27 @@ private predicate guardControlsBranchEdge_v3( ) } +/** INTERNAL: Use `Guard` instead. */ +final class Guard_v2 extends Guard { + /** + * Holds if this guard evaluating to `branch` controls the control-flow + * branch edge from `bb1` to `bb2`. That is, following the edge from + * `bb1` to `bb2` implies that this guard evaluated to `branch`. + */ + predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) { + guardControlsBranchEdge_v2(this, bb1, bb2, branch) + } + + /** + * Holds if this guard evaluating to `branch` directly or indirectly controls + * the block `controlled`. That is, the evaluation of `controlled` is + * dominated by this guard evaluating to `branch`. + */ + predicate controls(BasicBlock controlled, boolean branch) { + guardControls_v2(this, controlled, branch) + } +} + private predicate equalityGuard(Guard g, Expr e1, Expr e2, boolean polarity) { exists(EqualityTest eqtest | eqtest = g and diff --git a/java/ql/lib/semmle/code/java/dataflow/ModulusAnalysis.qll b/java/ql/lib/semmle/code/java/dataflow/ModulusAnalysis.qll index 5b2a39ad6c9e..3e5a45da247d 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ModulusAnalysis.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ModulusAnalysis.qll @@ -17,7 +17,7 @@ private predicate valueFlowStepSsa(SsaVariable v, SsaReadPosition pos, Expr e, i exists(Guard guard, boolean testIsTrue | pos.hasReadOfVar(v) and guard = eqFlowCond(v, e, delta, true, testIsTrue) and - guardDirectlyControlsSsaRead(guard, pos, testIsTrue) + guardControlsSsaRead(guard, pos, testIsTrue) ) } diff --git a/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll b/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll index b43990e14553..64f68b9c075a 100644 --- a/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll +++ b/java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll @@ -219,16 +219,10 @@ module Sem implements Semantic { int getBlockId1(BasicBlock bb) { idOf(bb, result) } - final private class FinalGuard = GL::Guard; - - class Guard extends FinalGuard { + class Guard extends GL::Guard_v2 { Expr asExpr() { result = this } } - predicate implies_v2(Guard g1, boolean b1, Guard g2, boolean b2) { - GL::implies_v2(g1, b1, g2, b2) - } - class Type = J::Type; class IntegerType extends J::IntegralType { diff --git a/java/ql/lib/semmle/code/java/dataflow/RangeUtils.qll b/java/ql/lib/semmle/code/java/dataflow/RangeUtils.qll index e96d591ced54..444fec8f8659 100644 --- a/java/ql/lib/semmle/code/java/dataflow/RangeUtils.qll +++ b/java/ql/lib/semmle/code/java/dataflow/RangeUtils.qll @@ -19,8 +19,6 @@ predicate ssaUpdateStep = U::ssaUpdateStep/3; predicate valueFlowStep = U::valueFlowStep/3; -predicate guardDirectlyControlsSsaRead = U::guardDirectlyControlsSsaRead/3; - predicate guardControlsSsaRead = U::guardControlsSsaRead/3; predicate eqFlowCond = U::eqFlowCond/5; diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll b/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll index c88b9946faad..b639947793b5 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll @@ -4,7 +4,6 @@ module Private { private import semmle.code.java.dataflow.RangeUtils as RU private import semmle.code.java.controlflow.Guards as G private import semmle.code.java.controlflow.BasicBlocks as BB - private import semmle.code.java.controlflow.internal.GuardsLogic as GL private import SsaReadPositionCommon class BasicBlock = BB::BasicBlock; @@ -15,7 +14,7 @@ module Private { class Expr = J::Expr; - class Guard = G::Guard; + class Guard = G::Guard_v2; class ConstantIntegerExpr = RU::ConstantIntegerExpr; @@ -101,29 +100,17 @@ module Private { } } - /** - * Holds if `guard` directly controls the position `controlled` with the - * value `testIsTrue`. - */ - pragma[nomagic] - predicate guardDirectlyControlsSsaRead(Guard guard, SsaReadPosition controlled, boolean testIsTrue) { - guard.directlyControls(controlled.(SsaReadPositionBlock).getBlock(), testIsTrue) - or - exists(SsaReadPositionPhiInputEdge controlledEdge | controlledEdge = controlled | - guard.directlyControls(controlledEdge.getOrigBlock(), testIsTrue) or - guard.hasBranchEdge(controlledEdge.getOrigBlock(), controlledEdge.getPhiBlock(), testIsTrue) - ) - } - /** * Holds if `guard` controls the position `controlled` with the value `testIsTrue`. */ predicate guardControlsSsaRead(Guard guard, SsaReadPosition controlled, boolean testIsTrue) { - guardDirectlyControlsSsaRead(guard, controlled, testIsTrue) + guard.controls(controlled.(SsaReadPositionBlock).getBlock(), testIsTrue) or - exists(Guard guard0, boolean testIsTrue0 | - GL::implies_v2(guard0, testIsTrue0, guard, testIsTrue) and - guardControlsSsaRead(guard0, controlled, testIsTrue0) + exists(SsaReadPositionPhiInputEdge controlledEdge | controlledEdge = controlled | + guard.controls(controlledEdge.getOrigBlock(), testIsTrue) or + guard + .controlsBranchEdge(controlledEdge.getOrigBlock(), controlledEdge.getPhiBlock(), + testIsTrue) ) } diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll b/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll index ee2e3bb24123..04e896b26011 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll @@ -7,13 +7,12 @@ module Private { private import semmle.code.java.dataflow.SSA as Ssa private import semmle.code.java.controlflow.Guards as G private import SsaReadPositionCommon - private import semmle.code.java.controlflow.internal.GuardsLogic as GL private import Sign import Impl class ConstantIntegerExpr = RU::ConstantIntegerExpr; - class Guard = G::Guard; + class Guard = G::Guard_v2; class SsaVariable = Ssa::SsaVariable; @@ -170,31 +169,17 @@ module Private { predicate ssaRead = RU::ssaRead/2; - /** - * Holds if `guard` directly controls the position `controlled` with the - * value `testIsTrue`. - */ - pragma[nomagic] - private predicate guardDirectlyControlsSsaRead( - Guard guard, SsaReadPosition controlled, boolean testIsTrue - ) { - guard.directlyControls(controlled.(SsaReadPositionBlock).getBlock(), testIsTrue) - or - exists(SsaReadPositionPhiInputEdge controlledEdge | controlledEdge = controlled | - guard.directlyControls(controlledEdge.getOrigBlock(), testIsTrue) or - guard.hasBranchEdge(controlledEdge.getOrigBlock(), controlledEdge.getPhiBlock(), testIsTrue) - ) - } - /** * Holds if `guard` controls the position `controlled` with the value `testIsTrue`. */ predicate guardControlsSsaRead(Guard guard, SsaReadPosition controlled, boolean testIsTrue) { - guardDirectlyControlsSsaRead(guard, controlled, testIsTrue) + guard.controls(controlled.(SsaReadPositionBlock).getBlock(), testIsTrue) or - exists(Guard guard0, boolean testIsTrue0 | - GL::implies_v2(guard0, testIsTrue0, guard, testIsTrue) and - guardControlsSsaRead(guard0, controlled, testIsTrue0) + exists(SsaReadPositionPhiInputEdge controlledEdge | controlledEdge = controlled | + guard.controls(controlledEdge.getOrigBlock(), testIsTrue) or + guard + .controlsBranchEdge(controlledEdge.getOrigBlock(), controlledEdge.getPhiBlock(), + testIsTrue) ) } } diff --git a/shared/rangeanalysis/codeql/rangeanalysis/ModulusAnalysis.qll b/shared/rangeanalysis/codeql/rangeanalysis/ModulusAnalysis.qll index 88d816b8644c..db3377ff3cc1 100644 --- a/shared/rangeanalysis/codeql/rangeanalysis/ModulusAnalysis.qll +++ b/shared/rangeanalysis/codeql/rangeanalysis/ModulusAnalysis.qll @@ -34,7 +34,7 @@ module ModulusAnalysis< exists(Sem::Guard guard, boolean testIsTrue | hasReadOfVarInlineLate(pos, v) and guard = eqFlowCond(v, e, D::fromInt(delta), true, testIsTrue) and - guardDirectlyControlsSsaRead(guard, pos, testIsTrue) + guardControlsSsaRead(guard, pos, testIsTrue) ) } diff --git a/shared/rangeanalysis/codeql/rangeanalysis/RangeAnalysis.qll b/shared/rangeanalysis/codeql/rangeanalysis/RangeAnalysis.qll index 60888c9f93f6..445ec9f0b8d3 100644 --- a/shared/rangeanalysis/codeql/rangeanalysis/RangeAnalysis.qll +++ b/shared/rangeanalysis/codeql/rangeanalysis/RangeAnalysis.qll @@ -181,21 +181,6 @@ signature module Semantic { */ Expr asExpr(); - /** - * Holds if the guard directly controls a given basic block. For example in - * the following code, the guard `(x > y)` directly controls the block - * beneath it: - * ``` - * if (x > y) - * { - * Console.WriteLine("x is greater than y"); - * } - * ``` - * `branch` indicates whether the basic block is entered when the guard - * evaluates to `true` or when it evaluates to `false`. - */ - predicate directlyControls(BasicBlock controlled, boolean branch); - /** * Holds if this guard is an equality test between `e1` and `e2`. If the * test is negated, that is `!=`, then `polarity` is false, otherwise @@ -204,24 +189,19 @@ signature module Semantic { predicate isEquality(Expr e1, Expr e2, boolean polarity); /** - * Holds if there is a branch edge between two basic blocks. For example - * in the following C code, there are two branch edges from the basic block - * containing the condition `(x > y)` to the beginnings of the true and - * false blocks that follow: - * ``` - * if (x > y) { - * printf("x is greater than y\n"); - * } else { - * printf("x is not greater than y\n"); - * } - * ``` - * `branch` indicates whether the second basic block is the one entered - * when the guard evaluates to `true` or when it evaluates to `false`. + * Holds if this guard evaluating to `branch` controls the control-flow + * branch edge from `bb1` to `bb2`. That is, following the edge from + * `bb1` to `bb2` implies that this guard evaluated to `branch`. */ - predicate hasBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch); - } + predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch); - predicate implies_v2(Guard g1, boolean b1, Guard g2, boolean b2); + /** + * Holds if this guard evaluating to `branch` directly or indirectly controls + * the block `controlled`. That is, the evaluation of `controlled` is + * dominated by this guard evaluating to `branch`. + */ + predicate controls(BasicBlock controlled, boolean branch); + } class Type; @@ -670,19 +650,14 @@ module RangeStage< delta = D::fromFloat(D::toFloat(d1) + d2 + d3) ) or - exists(boolean testIsTrue0 | - Sem::implies_v2(result, testIsTrue, boundFlowCond(v, e, delta, upper, testIsTrue0), - testIsTrue0) - ) - or result = eqFlowCond(v, e, delta, true, testIsTrue) and (upper = true or upper = false) or - // guard that tests whether `v2` is bounded by `e + delta + d1 - d2` and - // exists a guard `guardEq` such that `v = v2 - d1 + d2`. + // guard that tests whether `v2` is bounded by `e + delta - d` and + // exists a guard `guardEq` such that `v = v2 + d`. exists(Sem::SsaVariable v2, D::Delta oldDelta, float d | // equality needs to control guard - result.getBasicBlock() = eqSsaCondDirectlyControls(v, v2, d) and + result.getBasicBlock() = eqSsaCondControls(v, v2, d) and result = boundFlowCond(v2, e, oldDelta, upper, testIsTrue) and delta = D::fromFloat(D::toFloat(oldDelta) + d) ) @@ -692,13 +667,11 @@ module RangeStage< * Gets a basic block in which `v1` equals `v2 + delta`. */ pragma[nomagic] - private Sem::BasicBlock eqSsaCondDirectlyControls( - Sem::SsaVariable v1, Sem::SsaVariable v2, float delta - ) { + private Sem::BasicBlock eqSsaCondControls(Sem::SsaVariable v1, Sem::SsaVariable v2, float delta) { exists(Sem::Guard guardEq, D::Delta d1, D::Delta d2, boolean eqIsTrue | guardEq = eqFlowCond(v1, ssaRead(v2, d1), d2, true, eqIsTrue) and delta = D::toFloat(d2) - D::toFloat(d1) and - guardEq.directlyControls(result, eqIsTrue) + guardEq.controls(result, eqIsTrue) ) } @@ -749,7 +722,7 @@ module RangeStage< exists(Sem::Guard guard, boolean testIsTrue | pos.hasReadOfVar(v) and guard = boundFlowCond(v, e, delta, upper, testIsTrue) and - guardDirectlyControlsSsaRead(guard, pos, testIsTrue) and + guardControlsSsaRead(guard, pos, testIsTrue) and reason = TSemCondReason(guard) ) } @@ -762,7 +735,7 @@ module RangeStage< exists(Sem::Guard guard, boolean testIsTrue | pos.hasReadOfVar(v) and guard = eqFlowCond(v, e, delta, false, testIsTrue) and - guardDirectlyControlsSsaRead(guard, pos, testIsTrue) and + guardControlsSsaRead(guard, pos, testIsTrue) and reason = TSemCondReason(guard) ) } diff --git a/shared/rangeanalysis/codeql/rangeanalysis/internal/RangeUtils.qll b/shared/rangeanalysis/codeql/rangeanalysis/internal/RangeUtils.qll index 05aef4805081..d6eeb781f391 100644 --- a/shared/rangeanalysis/codeql/rangeanalysis/internal/RangeUtils.qll +++ b/shared/rangeanalysis/codeql/rangeanalysis/internal/RangeUtils.qll @@ -52,10 +52,6 @@ module MakeUtils Lang, DeltaSig D> { (testIsTrue = true or testIsTrue = false) and eqpolarity.booleanXor(testIsTrue).booleanNot() = isEq ) - or - exists(boolean testIsTrue0 | - implies_v2(result, testIsTrue, eqFlowCond(v, e, delta, isEq, testIsTrue0), testIsTrue0) - ) } /** @@ -173,29 +169,17 @@ module MakeUtils Lang, DeltaSig D> { override string toString() { result = "edge" } } - /** - * Holds if `guard` directly controls the position `controlled` with the - * value `testIsTrue`. - */ - pragma[nomagic] - predicate guardDirectlyControlsSsaRead(Guard guard, SsaReadPosition controlled, boolean testIsTrue) { - guard.directlyControls(controlled.(SsaReadPositionBlock).getBlock(), testIsTrue) - or - exists(SsaReadPositionPhiInputEdge controlledEdge | controlledEdge = controlled | - guard.directlyControls(controlledEdge.getOrigBlock(), testIsTrue) or - guard.hasBranchEdge(controlledEdge.getOrigBlock(), controlledEdge.getPhiBlock(), testIsTrue) - ) - } - /** * Holds if `guard` controls the position `controlled` with the value `testIsTrue`. */ predicate guardControlsSsaRead(Guard guard, SsaReadPosition controlled, boolean testIsTrue) { - guardDirectlyControlsSsaRead(guard, controlled, testIsTrue) + guard.controls(controlled.(SsaReadPositionBlock).getBlock(), testIsTrue) or - exists(Guard guard0, boolean testIsTrue0 | - implies_v2(guard0, testIsTrue0, guard, testIsTrue) and - guardControlsSsaRead(guard0, controlled, testIsTrue0) + exists(SsaReadPositionPhiInputEdge controlledEdge | controlledEdge = controlled | + guard.controls(controlledEdge.getOrigBlock(), testIsTrue) or + guard + .controlsBranchEdge(controlledEdge.getOrigBlock(), controlledEdge.getPhiBlock(), + testIsTrue) ) }