From 704f1fad462a5f672eccd9cd784cca8543b4ff19 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 17:02:12 +0000 Subject: [PATCH 1/9] C++: Add switches as testcases for guard conditions. --- .../library-tests/controlflow/guards/test.cpp | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/cpp/ql/test/library-tests/controlflow/guards/test.cpp b/cpp/ql/test/library-tests/controlflow/guards/test.cpp index 46d894813f9b..3a60f5f026e7 100644 --- a/cpp/ql/test/library-tests/controlflow/guards/test.cpp +++ b/cpp/ql/test/library-tests/controlflow/guards/test.cpp @@ -52,3 +52,37 @@ bool testWithCatch0(int v) return false; } + +void use1(int); +void use2(int); +void use3(int); + +void test_switches_simple(int i) { + switch(i) { + case 0: + use1(i); + break; + case 1: + use2(i); + /* NOTE: fallthrough */ + case 2: + use3(i); + } +} + +void test_switches_range(int i) { + switch(i) { + case 0 ... 10: + use1(i); + break; + case 11 ... 20: + use2(i); + } +} + +void test_switches_default(int i) { + switch(i) { + default: + use1(i); + } +} \ No newline at end of file From 2af68d37d02d4ca391b42dfe618b569bb395ae1a Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 15:08:39 +0000 Subject: [PATCH 2/9] C++: Include 'SwitchInstruction's as 'IRGuardCondition's. --- .../semmle/code/cpp/controlflow/IRGuards.qll | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll index 0adba6c1e71f..4598fb0aa3b5 100644 --- a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll +++ b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll @@ -249,10 +249,10 @@ private predicate nonExcludedIRAndBasicBlock(IRBlock irb, BasicBlock controlled) */ cached class IRGuardCondition extends Instruction { - ConditionalBranchInstruction branch; + Instruction branch; cached - IRGuardCondition() { branch = get_branch_for_condition(this) } + IRGuardCondition() { branch = getBranchForCondition(this) } /** * Holds if this condition controls `controlled`, meaning that `controlled` is only @@ -302,7 +302,7 @@ class IRGuardCondition extends Instruction { this.controls(pred, testIsTrue) or succ = this.getBranchSuccessor(testIsTrue) and - branch.getCondition() = this and + branch.(ConditionalBranchInstruction).getCondition() = this and branch.getBlock() = pred } @@ -322,13 +322,13 @@ class IRGuardCondition extends Instruction { * ``` */ private IRBlock getBranchSuccessor(boolean testIsTrue) { - branch.getCondition() = this and + branch.(ConditionalBranchInstruction).getCondition() = this and ( testIsTrue = true and - result.getFirstInstruction() = branch.getTrueSuccessor() + result.getFirstInstruction() = branch.(ConditionalBranchInstruction).getTrueSuccessor() or testIsTrue = false and - result.getFirstInstruction() = branch.getFalseSuccessor() + result.getFirstInstruction() = branch.(ConditionalBranchInstruction).getFalseSuccessor() ) } @@ -476,12 +476,14 @@ class IRGuardCondition extends Instruction { private IRBlock getBranchBlock() { result = branch.getBlock() } } -private ConditionalBranchInstruction get_branch_for_condition(Instruction guard) { - result.getCondition() = guard +private Instruction getBranchForCondition(Instruction guard) { + result.(ConditionalBranchInstruction).getCondition() = guard or exists(LogicalNotInstruction cond | - result = get_branch_for_condition(cond) and cond.getUnary() = guard + result = getBranchForCondition(cond) and cond.getUnary() = guard ) + or + result.(SwitchInstruction).getExpression() = guard } /** From b7292fbc67e3037732f9b26ef1e87c716e71d023 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 16:49:39 +0000 Subject: [PATCH 3/9] C++: Introduce 'AbstractValue' similar to what C# has. --- .../semmle/code/cpp/controlflow/IRGuards.qll | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll index 4598fb0aa3b5..83d45840e915 100644 --- a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll +++ b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll @@ -20,6 +20,44 @@ private predicate isUnreachedBlock(IRBlock block) { block.getFirstInstruction() instanceof UnreachedInstruction } +private newtype TAbstractValue = + TBooleanValue(boolean b) { b = true or b = false } or + TMatchValue(CaseEdge c) + +/** + * An abstract value. This is either a boolean value, or a `switch` case. + */ +abstract class AbstractValue extends TAbstractValue { + /** Gets an abstract value that represents the dual of this value, if any. */ + abstract AbstractValue getDualValue(); + + /** Gets a textual representation of this abstract value. */ + abstract string toString(); +} + +/** A Boolean value. */ +class BooleanValue extends AbstractValue, TBooleanValue { + /** Gets the underlying Boolean value. */ + boolean getValue() { this = TBooleanValue(result) } + + override BooleanValue getDualValue() { result.getValue() = this.getValue().booleanNot() } + + override string toString() { result = this.getValue().toString() } +} + +/** A value that represents a match against a specific `switch` case. */ +class MatchValue extends AbstractValue, TMatchValue { + /** Gets the case. */ + CaseEdge getCase() { this = TMatchValue(result) } + + override MatchValue getDualValue() { + // A `MatchValue` has no dual. + none() + } + + override string toString() { result = this.getCase().toString() } +} + /** * A Boolean condition in the AST that guards one or more basic blocks. This includes * operands of logical operators but not switch statements. From f4eb5f5a2df063c94f08fd4e6983892dd9e2a5c2 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 16:51:15 +0000 Subject: [PATCH 4/9] C++: Convert 'getBranchSuccessor' to use abstract values. --- .../lib/semmle/code/cpp/controlflow/IRGuards.qll | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll index 83d45840e915..3566cfa58f3f 100644 --- a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll +++ b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll @@ -359,15 +359,21 @@ class IRGuardCondition extends Instruction { * return x; * ``` */ - private IRBlock getBranchSuccessor(boolean testIsTrue) { + private IRBlock getBranchSuccessor(AbstractValue v) { branch.(ConditionalBranchInstruction).getCondition() = this and - ( - testIsTrue = true and + exists(BooleanValue bv | bv = v | + bv.getValue() = true and result.getFirstInstruction() = branch.(ConditionalBranchInstruction).getTrueSuccessor() or - testIsTrue = false and + bv.getValue() = false and result.getFirstInstruction() = branch.(ConditionalBranchInstruction).getFalseSuccessor() ) + or + exists(SwitchInstruction switch, CaseEdge kind | switch = branch | + switch.getExpression() = this and + result.getFirstInstruction() = switch.getSuccessor(kind) and + kind = v.(MatchValue).getCase() + ) } /** Holds if (determined by this guard) `left < right + k` evaluates to `isLessThan` if this expression evaluates to `testIsTrue`. */ From 34decd3cf17be9722fc66236c1e3da0efd093653 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 16:53:11 +0000 Subject: [PATCH 5/9] C++: Add more general public predicates to work with abstract values. --- .../semmle/code/cpp/controlflow/IRGuards.qll | 70 +++++++++++++------ 1 file changed, 50 insertions(+), 20 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll index 3566cfa58f3f..2d37525de0de 100644 --- a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll +++ b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll @@ -72,6 +72,15 @@ class GuardCondition extends Expr { this.(BinaryLogicalOperation).getAnOperand() instanceof GuardCondition } + /** + * Holds if this condition controls `controlled`, meaning that `controlled` is only + * entered if the value of this condition is `v`. + * + * For details on what "controls" mean, see the QLDoc for `controls`. + */ + cached + predicate valueControls(BasicBlock controlled, AbstractValue v) { none() } + /** * Holds if this condition controls `controlled`, meaning that `controlled` is only * entered if the value of this condition is `testIsTrue`. @@ -99,7 +108,9 @@ class GuardCondition extends Expr { * true (for `&&`) or false (for `||`) branch. */ cached - predicate controls(BasicBlock controlled, boolean testIsTrue) { none() } + final predicate controls(BasicBlock controlled, boolean testIsTrue) { + this.valueControls(controlled, any(BooleanValue bv | bv.getValue() = testIsTrue)) + } /** Holds if (determined by this guard) `left < right + k` evaluates to `isLessThan` if this expression evaluates to `testIsTrue`. */ cached @@ -136,13 +147,13 @@ private class GuardConditionFromBinaryLogicalOperator extends GuardCondition { this.(BinaryLogicalOperation).getAnOperand() instanceof GuardCondition } - override predicate controls(BasicBlock controlled, boolean testIsTrue) { + override predicate valueControls(BasicBlock controlled, AbstractValue v) { exists(BinaryLogicalOperation binop, GuardCondition lhs, GuardCondition rhs | this = binop and lhs = binop.getLeftOperand() and rhs = binop.getRightOperand() and - lhs.controls(controlled, testIsTrue) and - rhs.controls(controlled, testIsTrue) + lhs.valueControls(controlled, v) and + rhs.valueControls(controlled, v) ) } @@ -184,10 +195,10 @@ private class GuardConditionFromIR extends GuardCondition { GuardConditionFromIR() { this = ir.getUnconvertedResultExpression() } - override predicate controls(BasicBlock controlled, boolean testIsTrue) { + override predicate valueControls(BasicBlock controlled, AbstractValue v) { // This condition must determine the flow of control; that is, this // node must be a top-level condition. - this.controlsBlock(controlled, testIsTrue) + this.controlsBlock(controlled, v) } /** Holds if (determined by this guard) `left < right + k` evaluates to `isLessThan` if this expression evaluates to `testIsTrue`. */ @@ -240,9 +251,9 @@ private class GuardConditionFromIR extends GuardCondition { * predicate does not necessarily hold for binary logical operations like * `&&` and `||`. See the detailed explanation on predicate `controls`. */ - private predicate controlsBlock(BasicBlock controlled, boolean testIsTrue) { + private predicate controlsBlock(BasicBlock controlled, AbstractValue v) { exists(IRBlock irb | - ir.controls(irb, testIsTrue) and + ir.valueControls(irb, v) and nonExcludedIRAndBasicBlock(irb, controlled) and not isUnreachedBlock(irb) ) @@ -319,29 +330,48 @@ class IRGuardCondition extends Instruction { * true (for `&&`) or false (for `||`) branch. */ cached - predicate controls(IRBlock controlled, boolean testIsTrue) { + predicate valueControls(IRBlock controlled, AbstractValue v) { // This condition must determine the flow of control; that is, this // node must be a top-level condition. - this.controlsBlock(controlled, testIsTrue) + this.controlsBlock(controlled, v) or exists(IRGuardCondition ne | this = ne.(LogicalNotInstruction).getUnary() and - ne.controls(controlled, testIsTrue.booleanNot()) + ne.valueControls(controlled, v.getDualValue()) ) } + cached + predicate controls(IRBlock controlled, boolean testIsTrue) { + this.valueControls(controlled, any(BooleanValue bv | bv.getValue() = testIsTrue)) + } + /** * Holds if the control-flow edge `(pred, succ)` may be taken only if - * the value of this condition is `testIsTrue`. + * the value of this condition is `v`. */ cached - predicate controlsEdge(IRBlock pred, IRBlock succ, boolean testIsTrue) { + predicate valueControlsEdge(IRBlock pred, IRBlock succ, AbstractValue v) { pred.getASuccessor() = succ and - this.controls(pred, testIsTrue) + this.valueControls(pred, v) or - succ = this.getBranchSuccessor(testIsTrue) and - branch.(ConditionalBranchInstruction).getCondition() = this and - branch.getBlock() = pred + succ = this.getBranchSuccessor(v) and + ( + branch.(ConditionalBranchInstruction).getCondition() = this and + branch.getBlock() = pred + or + branch.(SwitchInstruction).getExpression() = this and + branch.getBlock() = pred + ) + } + + /** + * Holds if the control-flow edge `(pred, succ)` may be taken only if + * the value of this condition is `testIsTrue`. + */ + cached + final predicate controlsEdge(IRBlock pred, IRBlock succ, boolean testIsTrue) { + this.valueControlsEdge(pred, succ, any(BooleanValue bv | bv.getValue() = testIsTrue)) } /** @@ -440,11 +470,11 @@ class IRGuardCondition extends Instruction { /** * Holds if this condition controls `block`, meaning that `block` is only - * entered if the value of this condition is `testIsTrue`. This helper + * entered if the value of this condition is `v`. This helper * predicate does not necessarily hold for binary logical operations like * `&&` and `||`. See the detailed explanation on predicate `controls`. */ - private predicate controlsBlock(IRBlock controlled, boolean testIsTrue) { + private predicate controlsBlock(IRBlock controlled, AbstractValue v) { not isUnreachedBlock(controlled) and // // For this block to control the block `controlled` with `testIsTrue` the @@ -485,7 +515,7 @@ class IRGuardCondition extends Instruction { // that `this` strictly dominates `controlled` so that isn't necessary to check // directly. exists(IRBlock succ | - succ = this.getBranchSuccessor(testIsTrue) and + succ = this.getBranchSuccessor(v) and this.hasDominatingEdgeTo(succ) and succ.dominates(controlled) ) From 07ebbb0591ce27eead96b4d43197db0c42a06fca Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 17:04:27 +0000 Subject: [PATCH 6/9] C++: Accept test changes. --- cpp/ql/test/library-tests/controlflow/guards/Guards.expected | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cpp/ql/test/library-tests/controlflow/guards/Guards.expected b/cpp/ql/test/library-tests/controlflow/guards/Guards.expected index 4193bd49fef5..6eebc960ce3c 100644 --- a/cpp/ql/test/library-tests/controlflow/guards/Guards.expected +++ b/cpp/ql/test/library-tests/controlflow/guards/Guards.expected @@ -29,3 +29,6 @@ | test.cpp:18:8:18:10 | call to get | | test.cpp:31:7:31:13 | ... == ... | | test.cpp:42:13:42:20 | call to getABool | +| test.cpp:61:10:61:10 | i | +| test.cpp:74:10:74:10 | i | +| test.cpp:84:10:84:10 | i | From fb218150e1c6e7fa2984350cbe55de5efcaa0375 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 17:05:42 +0000 Subject: [PATCH 7/9] C++: Change the testcase so that it outputs the controlling values for switch statements as well. --- .../library-tests/controlflow/guards/GuardsControl.expected | 4 ++++ .../test/library-tests/controlflow/guards/GuardsControl.ql | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected b/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected index 1e0aed878bfb..fbfaff9acf6b 100644 --- a/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected +++ b/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected @@ -86,3 +86,7 @@ | test.cpp:31:7:31:13 | ... == ... | true | 31 | 32 | | test.cpp:42:13:42:20 | call to getABool | false | 53 | 53 | | test.cpp:42:13:42:20 | call to getABool | true | 43 | 45 | +| test.cpp:61:10:61:10 | i | Case[0] | 62 | 64 | +| test.cpp:61:10:61:10 | i | Case[1] | 65 | 66 | +| test.cpp:74:10:74:10 | i | Case[0..10] | 75 | 77 | +| test.cpp:74:10:74:10 | i | Case[11..20] | 78 | 79 | diff --git a/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.ql b/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.ql index 5101f64a2c55..93ce054cd820 100644 --- a/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.ql +++ b/cpp/ql/test/library-tests/controlflow/guards/GuardsControl.ql @@ -7,10 +7,10 @@ import cpp import semmle.code.cpp.controlflow.Guards -from GuardCondition guard, boolean sense, int start, int end +from GuardCondition guard, AbstractValue value, int start, int end where exists(BasicBlock block | - guard.controls(block, sense) and + guard.valueControls(block, value) and block.hasLocationInfo(_, start, _, end, _) ) -select guard, sense, start, end +select guard, value, start, end From b5e59492bfeeffbb7d22ea812776abc9b5da6af5 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 17:17:05 +0000 Subject: [PATCH 8/9] C++: Add change note. --- .../change-notes/2024-03-15-switches-in-guard-conditions.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/lib/change-notes/2024-03-15-switches-in-guard-conditions.md diff --git a/cpp/ql/lib/change-notes/2024-03-15-switches-in-guard-conditions.md b/cpp/ql/lib/change-notes/2024-03-15-switches-in-guard-conditions.md new file mode 100644 index 000000000000..cf0b920e29dc --- /dev/null +++ b/cpp/ql/lib/change-notes/2024-03-15-switches-in-guard-conditions.md @@ -0,0 +1,4 @@ +--- +category: feature +--- +* Added a predicate `GuardCondition.valueControls` to query whether a basic block is guarded by a particular `case` of a `switch` statement. \ No newline at end of file From f4f417c3f969e1981976c6b32175019e88d8b934 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Mar 2024 17:19:36 +0000 Subject: [PATCH 9/9] C++: Fix QLoc. --- .../semmle/code/cpp/controlflow/IRGuards.qll | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll index 2d37525de0de..e7762fc9fa85 100644 --- a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll +++ b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll @@ -247,7 +247,7 @@ private class GuardConditionFromIR extends GuardCondition { /** * Holds if this condition controls `block`, meaning that `block` is only - * entered if the value of this condition is `testIsTrue`. This helper + * entered if the value of this condition is `v`. This helper * predicate does not necessarily hold for binary logical operations like * `&&` and `||`. See the detailed explanation on predicate `controls`. */ @@ -303,6 +303,24 @@ class IRGuardCondition extends Instruction { cached IRGuardCondition() { branch = getBranchForCondition(this) } + /** + * Holds if this condition controls `controlled`, meaning that `controlled` is only + * entered if the value of this condition is `v`. + * + * For details on what "controls" mean, see the QLDoc for `controls`. + */ + cached + predicate valueControls(IRBlock controlled, AbstractValue v) { + // This condition must determine the flow of control; that is, this + // node must be a top-level condition. + this.controlsBlock(controlled, v) + or + exists(IRGuardCondition ne | + this = ne.(LogicalNotInstruction).getUnary() and + ne.valueControls(controlled, v.getDualValue()) + ) + } + /** * Holds if this condition controls `controlled`, meaning that `controlled` is only * entered if the value of this condition is `testIsTrue`. @@ -329,18 +347,6 @@ class IRGuardCondition extends Instruction { * being short-circuited) then it will only control blocks dominated by the * true (for `&&`) or false (for `||`) branch. */ - cached - predicate valueControls(IRBlock controlled, AbstractValue v) { - // This condition must determine the flow of control; that is, this - // node must be a top-level condition. - this.controlsBlock(controlled, v) - or - exists(IRGuardCondition ne | - this = ne.(LogicalNotInstruction).getUnary() and - ne.valueControls(controlled, v.getDualValue()) - ) - } - cached predicate controls(IRBlock controlled, boolean testIsTrue) { this.valueControls(controlled, any(BooleanValue bv | bv.getValue() = testIsTrue)) @@ -375,7 +381,7 @@ class IRGuardCondition extends Instruction { } /** - * Gets the block to which `branch` jumps directly when this condition is `testIsTrue`. + * Gets the block to which `branch` jumps directly when the value of this condition is `v`. * * This predicate is intended to help with situations in which an inference can only be made * based on an edge between a block with multiple successors and a block with multiple