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 00665d836e1d..ccc4b0f87fe4 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll @@ -1078,6 +1078,7 @@ class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig { private import csharp as Cs private import semmle.code.csharp.controlflow.BasicBlocks + private import codeql.util.Boolean class Expr extends ControlFlow::Node { predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) { this = bb.getNode(i) } @@ -1114,6 +1115,58 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu } } + abstract class LogicalOperationGuard extends Guard { + abstract Guard getOperand(int i); + + abstract predicate lift(string id, int i, boolean operandBranch, boolean branch); + } + + private class NotGuard extends LogicalOperationGuard, LogicalNotExpr { + override Guard getOperand(int i) { i = 0 and result = this.getOperand() } + + override predicate lift(string id, int i, boolean operandBranch, boolean branch) { + operandBranch instanceof Boolean and + id = operandBranch.toString() and + i = 0 and + branch = operandBranch.booleanNot() + } + } + + abstract private class BinaryLogicalOperationGuard extends LogicalOperationGuard, + BinaryLogicalOperation + { + final override Guard getOperand(int i) { + i = 0 and result = this.getLeftOperand() + or + i = 1 and result = this.getRightOperand() + } + + abstract predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch); + + final override predicate lift(string id, int i, boolean operandBranch, boolean branch) { + exists(Boolean branchLeft, Boolean branchRight | + this.lift(branchLeft, branchRight, branch) and + id = branchLeft + "," + branchRight + | + i = 0 and operandBranch = branchLeft + or + i = 1 and operandBranch = branchRight + ) + } + } + + private class AndGuard extends BinaryLogicalOperationGuard, LogicalAndExpr { + override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) { + branch = branchLeft.booleanOr(branchRight) // yes, should not be `booleanAnd` + } + } + + private class OrGuard extends BinaryLogicalOperationGuard, LogicalOrExpr { + override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) { + branch = branchLeft.booleanAnd(branchRight) // yes, should not be `booleanOr` + } + } + /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ predicate guardControlsBlock(Guard guard, ControlFlow::BasicBlock bb, boolean branch) { exists(ConditionBlock conditionBlock, ControlFlow::SuccessorTypes::ConditionalSuccessor s | diff --git a/csharp/ql/test/library-tests/dataflow/barrier-guards/BarrierFlow.cs b/csharp/ql/test/library-tests/dataflow/barrier-guards/BarrierFlow.cs index 3f3afed0a148..1fdc9b966671 100644 --- a/csharp/ql/test/library-tests/dataflow/barrier-guards/BarrierFlow.cs +++ b/csharp/ql/test/library-tests/dataflow/barrier-guards/BarrierFlow.cs @@ -80,4 +80,104 @@ void M6(bool b) Sink(x); } + + void M7() + { + var x = Source(7); + + if (x == "safe" && x == "safe") + { + Sink(x); + } + else + { + Sink(x); // $ hasValueFlow=7 + } + + if (x != "safe" && x == "safe2") + { + Sink(x); + } + else + { + Sink(x); // $ hasValueFlow=7 + } + + if (x == "safe" && x != "safe2") + { + Sink(x); + } + else + { + Sink(x); // $ hasValueFlow=7 + } + + if (x != "safe1" && x != "safe2") + { + Sink(x); // $ hasValueFlow=7 + } + else + { + Sink(x); + } + + if (!(x == "safe1") && x != "safe2") + { + Sink(x); // $ hasValueFlow=7 + } + else + { + Sink(x); + } + } + + void M8() + { + var x = Source(8); + + if (x == "safe1" || x == "safe2") + { + Sink(x); + } + else + { + Sink(x); // $ hasValueFlow=8 + } + + if (x != "safe1" || x == "safe2") + { + Sink(x); // $ hasValueFlow=8 + } + else + { + Sink(x); + } + + if (x == "safe1" || x != "safe2") + { + Sink(x); // $ hasValueFlow=8 + } + else + { + Sink(x); + } + + if (x != "safe" || x != "safe") + { + Sink(x); // $ hasValueFlow=8 + } + else + { + Sink(x); + } + + if (!(x == "safe") || x != "safe") + { + Sink(x); // $ hasValueFlow=8 + } + else + { + Sink(x); + } + } } diff --git a/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected b/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected index 4fb1c0c3df15..c6f74ce465ef 100644 --- a/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected +++ b/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected @@ -4,6 +4,18 @@ edges | BarrierFlow.cs:10:17:10:25 | call to method Source : Object | BarrierFlow.cs:10:13:10:13 | access to local variable x : Object | provenance | | | BarrierFlow.cs:17:13:17:13 | access to local variable x : Object | BarrierFlow.cs:21:18:21:18 | access to local variable x | provenance | | | BarrierFlow.cs:17:17:17:25 | call to method Source : Object | BarrierFlow.cs:17:13:17:13 | access to local variable x : Object | provenance | | +| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:94:18:94:18 | access to local variable x | provenance | | +| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:103:18:103:18 | access to local variable x | provenance | | +| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:112:18:112:18 | access to local variable x | provenance | | +| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:117:18:117:18 | access to local variable x | provenance | | +| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:126:18:126:18 | access to local variable x | provenance | | +| BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | provenance | | +| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:144:18:144:18 | access to local variable x | provenance | | +| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:149:18:149:18 | access to local variable x | provenance | | +| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:158:18:158:18 | access to local variable x | provenance | | +| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:167:18:167:18 | access to local variable x | provenance | | +| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:176:18:176:18 | access to local variable x | provenance | | +| BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | provenance | | nodes | BarrierFlow.cs:10:13:10:13 | access to local variable x : Object | semmle.label | access to local variable x : Object | | BarrierFlow.cs:10:17:10:25 | call to method Source : Object | semmle.label | call to method Source : Object | @@ -11,8 +23,32 @@ nodes | BarrierFlow.cs:17:13:17:13 | access to local variable x : Object | semmle.label | access to local variable x : Object | | BarrierFlow.cs:17:17:17:25 | call to method Source : Object | semmle.label | call to method Source : Object | | BarrierFlow.cs:21:18:21:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | semmle.label | access to local variable x : Object | +| BarrierFlow.cs:86:17:86:25 | call to method Source : Object | semmle.label | call to method Source : Object | +| BarrierFlow.cs:94:18:94:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:103:18:103:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:112:18:112:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:117:18:117:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:126:18:126:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | semmle.label | access to local variable x : Object | +| BarrierFlow.cs:136:17:136:25 | call to method Source : Object | semmle.label | call to method Source : Object | +| BarrierFlow.cs:144:18:144:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:149:18:149:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:158:18:158:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:167:18:167:18 | access to local variable x | semmle.label | access to local variable x | +| BarrierFlow.cs:176:18:176:18 | access to local variable x | semmle.label | access to local variable x | subpaths testFailures #select | BarrierFlow.cs:12:14:12:14 | access to local variable x | BarrierFlow.cs:10:17:10:25 | call to method Source : Object | BarrierFlow.cs:12:14:12:14 | access to local variable x | $@ | BarrierFlow.cs:10:17:10:25 | call to method Source : Object | call to method Source : Object | | BarrierFlow.cs:21:18:21:18 | access to local variable x | BarrierFlow.cs:17:17:17:25 | call to method Source : Object | BarrierFlow.cs:21:18:21:18 | access to local variable x | $@ | BarrierFlow.cs:17:17:17:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:94:18:94:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:94:18:94:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:103:18:103:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:103:18:103:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:112:18:112:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:112:18:112:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:117:18:117:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:117:18:117:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:126:18:126:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:126:18:126:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:144:18:144:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:144:18:144:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:149:18:149:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:149:18:149:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:158:18:158:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:158:18:158:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:167:18:167:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:167:18:167:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object | +| BarrierFlow.cs:176:18:176:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:176:18:176:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object | diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll index 6b6c8d3b6815..d393e4e994a9 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll @@ -31,30 +31,6 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN ) or stringConstCaseCompare(guard, testedNode, branch) - or - exists(CfgNodes::ExprNodes::BinaryOperationCfgNode g | - g = guard and - stringConstCompareOr(guard, branch) and - stringConstCompare(g.getLeftOperand(), testedNode, _) - ) -} - -/** - * Holds if `guard` is an `or` expression whose operands are string comparison guards. - * For example: - * - * ```rb - * x == "foo" or x == "bar" - * ``` - */ -private predicate stringConstCompareOr( - CfgNodes::ExprNodes::BinaryOperationCfgNode guard, boolean branch -) { - guard.getExpr() instanceof LogicalOrExpr and - branch = true and - forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() | - stringConstCompare(innerGuard, any(Ssa::Definition def).getARead(), branch) - ) } /** diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll index bc7a9cc0eb1b..375131493821 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -541,6 +541,7 @@ class ParameterExt extends TParameterExt { private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig { private import codeql.ruby.controlflow.internal.Guards as Guards + private import codeql.util.Boolean class Parameter = ParameterExt; @@ -560,6 +561,79 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } } + abstract class LogicalOperationGuard extends Guard { + abstract Guard getOperand(int i); + + abstract predicate lift(string id, int i, boolean operandBranch, boolean branch); + } + + private class NotGuard extends LogicalOperationGuard, + Cfg::CfgNodes::ExprNodes::UnaryOperationCfgNode + { + NotGuard() { this.getExpr() instanceof NotExpr } + + override Guard getOperand(int i) { i = 0 and result = this.getOperand() } + + override predicate lift(string id, int i, boolean operandBranch, boolean branch) { + operandBranch instanceof Boolean and + id = operandBranch.toString() and + i = 0 and + branch = operandBranch.booleanNot() + } + } + + private class StmtSequenceGuard extends LogicalOperationGuard, + Cfg::CfgNodes::ExprNodes::StmtSequenceCfgNode + { + override Guard getOperand(int i) { i = 0 and result = this.getLastStmt() } + + override predicate lift(string id, int i, boolean operandBranch, boolean branch) { + operandBranch instanceof Boolean and + id = operandBranch.toString() and + i = 0 and + branch = operandBranch + } + } + + abstract private class BinaryLogicalOperationGuard extends LogicalOperationGuard, + Cfg::CfgNodes::ExprNodes::BinaryOperationCfgNode + { + final override Guard getOperand(int i) { + i = 0 and result = this.getLeftOperand() + or + i = 1 and result = this.getRightOperand() + } + + abstract predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch); + + final override predicate lift(string id, int i, boolean operandBranch, boolean branch) { + exists(Boolean branchLeft, Boolean branchRight | + this.lift(branchLeft, branchRight, branch) and + id = branchLeft + "," + branchRight + | + i = 0 and operandBranch = branchLeft + or + i = 1 and operandBranch = branchRight + ) + } + } + + private class AndGuard extends BinaryLogicalOperationGuard { + AndGuard() { this.getExpr() instanceof LogicalAndExpr } + + override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) { + branch = branchLeft.booleanOr(branchRight) // yes, should not be `booleanAnd` + } + } + + private class OrGuard extends BinaryLogicalOperationGuard { + OrGuard() { this.getExpr() instanceof LogicalOrExpr } + + override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) { + branch = branchLeft.booleanAnd(branchRight) // yes, should not be `booleanOr` + } + } + /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) { Guards::guardControlsBlock(guard, bb, branch) diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected index 1bc62247a174..9cefcc5f7368 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected @@ -6,6 +6,18 @@ edges | barrier_flow.rb:8:9:8:17 | call to source | barrier_flow.rb:8:5:8:5 | x | provenance | | | barrier_flow.rb:24:5:24:5 | x | barrier_flow.rb:26:10:26:10 | x | provenance | | | barrier_flow.rb:24:9:24:17 | call to source | barrier_flow.rb:24:5:24:5 | x | provenance | | +| barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:87:14:87:14 | x | provenance | | +| barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:93:14:93:14 | x | provenance | | +| barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:99:14:99:14 | x | provenance | | +| barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:103:14:103:14 | x | provenance | | +| barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:109:14:109:14 | x | provenance | | +| barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:82:5:82:5 | x | provenance | | +| barrier_flow.rb:116:5:116:5 | x | barrier_flow.rb:121:14:121:14 | x | provenance | | +| barrier_flow.rb:116:5:116:5 | x | barrier_flow.rb:125:14:125:14 | x | provenance | | +| barrier_flow.rb:116:5:116:5 | x | barrier_flow.rb:131:14:131:14 | x | provenance | | +| barrier_flow.rb:116:5:116:5 | x | barrier_flow.rb:137:14:137:14 | x | provenance | | +| barrier_flow.rb:116:5:116:5 | x | barrier_flow.rb:143:14:143:14 | x | provenance | | +| barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:116:5:116:5 | x | provenance | | nodes | barrier_flow.rb:2:5:2:5 | x | semmle.label | x | | barrier_flow.rb:2:9:2:17 | call to source | semmle.label | call to source | @@ -16,9 +28,33 @@ nodes | barrier_flow.rb:24:5:24:5 | x | semmle.label | x | | barrier_flow.rb:24:9:24:17 | call to source | semmle.label | call to source | | barrier_flow.rb:26:10:26:10 | x | semmle.label | x | +| barrier_flow.rb:82:5:82:5 | x | semmle.label | x | +| barrier_flow.rb:82:9:82:18 | call to source | semmle.label | call to source | +| barrier_flow.rb:87:14:87:14 | x | semmle.label | x | +| barrier_flow.rb:93:14:93:14 | x | semmle.label | x | +| barrier_flow.rb:99:14:99:14 | x | semmle.label | x | +| barrier_flow.rb:103:14:103:14 | x | semmle.label | x | +| barrier_flow.rb:109:14:109:14 | x | semmle.label | x | +| barrier_flow.rb:116:5:116:5 | x | semmle.label | x | +| barrier_flow.rb:116:9:116:18 | call to source | semmle.label | call to source | +| barrier_flow.rb:121:14:121:14 | x | semmle.label | x | +| barrier_flow.rb:125:14:125:14 | x | semmle.label | x | +| barrier_flow.rb:131:14:131:14 | x | semmle.label | x | +| barrier_flow.rb:137:14:137:14 | x | semmle.label | x | +| barrier_flow.rb:143:14:143:14 | x | semmle.label | x | subpaths testFailures #select | barrier_flow.rb:4:10:4:10 | x | barrier_flow.rb:2:9:2:17 | call to source | barrier_flow.rb:4:10:4:10 | x | $@ | barrier_flow.rb:2:9:2:17 | call to source | call to source | | barrier_flow.rb:11:14:11:14 | x | barrier_flow.rb:8:9:8:17 | call to source | barrier_flow.rb:11:14:11:14 | x | $@ | barrier_flow.rb:8:9:8:17 | call to source | call to source | | barrier_flow.rb:26:10:26:10 | x | barrier_flow.rb:24:9:24:17 | call to source | barrier_flow.rb:26:10:26:10 | x | $@ | barrier_flow.rb:24:9:24:17 | call to source | call to source | +| barrier_flow.rb:87:14:87:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:87:14:87:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source | +| barrier_flow.rb:93:14:93:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:93:14:93:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source | +| barrier_flow.rb:99:14:99:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:99:14:99:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source | +| barrier_flow.rb:103:14:103:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:103:14:103:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source | +| barrier_flow.rb:109:14:109:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:109:14:109:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source | +| barrier_flow.rb:121:14:121:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:121:14:121:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source | +| barrier_flow.rb:125:14:125:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:125:14:125:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source | +| barrier_flow.rb:131:14:131:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:131:14:131:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source | +| barrier_flow.rb:137:14:137:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:137:14:137:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source | +| barrier_flow.rb:143:14:143:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:143:14:143:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected index 21d697c86e93..4c1234683742 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected @@ -46,6 +46,25 @@ newStyleBarrierGuards | barrier_flow.rb:58:5:59:34 | [input] SSA phi read(x) | | barrier_flow.rb:68:10:71:11 | [input] SSA phi read(x) | | barrier_flow.rb:72:5:75:11 | [input] SSA phi read(x) | +| barrier_flow.rb:84:24:84:24 | x | +| barrier_flow.rb:84:24:84:34 | [input] SSA phi read(x) | +| barrier_flow.rb:84:36:85:26 | [input] SSA phi read(x) | +| barrier_flow.rb:85:14:85:14 | x | +| barrier_flow.rb:91:14:91:14 | x | +| barrier_flow.rb:96:24:96:24 | x | +| barrier_flow.rb:97:14:97:14 | x | +| barrier_flow.rb:105:14:105:14 | x | +| barrier_flow.rb:111:14:111:14 | x | +| barrier_flow.rb:118:8:118:19 | [input] SSA phi read(x) | +| barrier_flow.rb:118:24:118:35 | [input] SSA phi read(x) | +| barrier_flow.rb:119:14:119:14 | x | +| barrier_flow.rb:124:23:124:23 | x | +| barrier_flow.rb:127:14:127:14 | x | +| barrier_flow.rb:133:14:133:14 | x | +| barrier_flow.rb:136:23:136:23 | x | +| barrier_flow.rb:139:14:139:14 | x | +| barrier_flow.rb:142:29:142:29 | x | +| barrier_flow.rb:145:14:145:14 | x | controls | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true | | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false | @@ -367,3 +386,97 @@ controls | barrier_flow.rb:69:12:69:23 | ... != ... | barrier_flow.rb:70:13:70:18 | return | true | | barrier_flow.rb:73:12:73:23 | ... != ... | barrier_flow.rb:73:9:75:11 | if ... | false | | barrier_flow.rb:73:12:73:23 | ... != ... | barrier_flow.rb:74:13:74:18 | return | true | +| barrier_flow.rb:84:8:84:18 | ... == ... | barrier_flow.rb:84:8:84:34 | [true] ... and ... | true | +| barrier_flow.rb:84:8:84:18 | ... == ... | barrier_flow.rb:84:24:84:24 | x | true | +| barrier_flow.rb:84:8:84:18 | ... == ... | barrier_flow.rb:85:9:85:14 | self | true | +| barrier_flow.rb:84:8:84:34 | [false] ... and ... | barrier_flow.rb:87:9:87:14 | self | false | +| barrier_flow.rb:84:8:84:34 | [true] ... and ... | barrier_flow.rb:85:9:85:14 | self | true | +| barrier_flow.rb:84:24:84:34 | ... == ... | barrier_flow.rb:84:8:84:34 | [true] ... and ... | true | +| barrier_flow.rb:84:24:84:34 | ... == ... | barrier_flow.rb:85:9:85:14 | self | true | +| barrier_flow.rb:90:8:90:18 | ... != ... | barrier_flow.rb:90:8:90:35 | [true] ... and ... | true | +| barrier_flow.rb:90:8:90:18 | ... != ... | barrier_flow.rb:90:24:90:24 | x | true | +| barrier_flow.rb:90:8:90:18 | ... != ... | barrier_flow.rb:91:9:91:14 | self | true | +| barrier_flow.rb:90:8:90:35 | [false] ... and ... | barrier_flow.rb:93:9:93:14 | self | false | +| barrier_flow.rb:90:8:90:35 | [true] ... and ... | barrier_flow.rb:91:9:91:14 | self | true | +| barrier_flow.rb:90:24:90:35 | ... == ... | barrier_flow.rb:90:8:90:35 | [true] ... and ... | true | +| barrier_flow.rb:90:24:90:35 | ... == ... | barrier_flow.rb:91:9:91:14 | self | true | +| barrier_flow.rb:96:8:96:18 | ... == ... | barrier_flow.rb:96:8:96:35 | [true] ... and ... | true | +| barrier_flow.rb:96:8:96:18 | ... == ... | barrier_flow.rb:96:24:96:24 | x | true | +| barrier_flow.rb:96:8:96:18 | ... == ... | barrier_flow.rb:97:9:97:14 | self | true | +| barrier_flow.rb:96:8:96:35 | [false] ... and ... | barrier_flow.rb:99:9:99:14 | self | false | +| barrier_flow.rb:96:8:96:35 | [true] ... and ... | barrier_flow.rb:97:9:97:14 | self | true | +| barrier_flow.rb:96:24:96:35 | ... != ... | barrier_flow.rb:96:8:96:35 | [true] ... and ... | true | +| barrier_flow.rb:96:24:96:35 | ... != ... | barrier_flow.rb:97:9:97:14 | self | true | +| barrier_flow.rb:102:8:102:19 | ... != ... | barrier_flow.rb:102:8:102:36 | [true] ... and ... | true | +| barrier_flow.rb:102:8:102:19 | ... != ... | barrier_flow.rb:102:25:102:25 | x | true | +| barrier_flow.rb:102:8:102:19 | ... != ... | barrier_flow.rb:103:9:103:14 | self | true | +| barrier_flow.rb:102:8:102:36 | [false] ... and ... | barrier_flow.rb:105:9:105:14 | self | false | +| barrier_flow.rb:102:8:102:36 | [true] ... and ... | barrier_flow.rb:103:9:103:14 | self | true | +| barrier_flow.rb:102:25:102:36 | ... != ... | barrier_flow.rb:102:8:102:36 | [true] ... and ... | true | +| barrier_flow.rb:102:25:102:36 | ... != ... | barrier_flow.rb:103:9:103:14 | self | true | +| barrier_flow.rb:108:8:108:25 | [true] not ... | barrier_flow.rb:108:8:108:42 | [true] ... and ... | true | +| barrier_flow.rb:108:8:108:25 | [true] not ... | barrier_flow.rb:108:31:108:31 | x | true | +| barrier_flow.rb:108:8:108:25 | [true] not ... | barrier_flow.rb:109:9:109:14 | self | true | +| barrier_flow.rb:108:8:108:42 | [false] ... and ... | barrier_flow.rb:111:9:111:14 | self | false | +| barrier_flow.rb:108:8:108:42 | [true] ... and ... | barrier_flow.rb:109:9:109:14 | self | true | +| barrier_flow.rb:108:12:108:25 | [false] ( ... ) | barrier_flow.rb:108:8:108:25 | [true] not ... | false | +| barrier_flow.rb:108:12:108:25 | [false] ( ... ) | barrier_flow.rb:108:8:108:42 | [true] ... and ... | false | +| barrier_flow.rb:108:12:108:25 | [false] ( ... ) | barrier_flow.rb:108:31:108:31 | x | false | +| barrier_flow.rb:108:12:108:25 | [false] ( ... ) | barrier_flow.rb:109:9:109:14 | self | false | +| barrier_flow.rb:108:12:108:25 | [true] ( ... ) | barrier_flow.rb:108:8:108:25 | [false] not ... | true | +| barrier_flow.rb:108:13:108:24 | ... == ... | barrier_flow.rb:108:8:108:25 | [false] not ... | true | +| barrier_flow.rb:108:13:108:24 | ... == ... | barrier_flow.rb:108:8:108:25 | [true] not ... | false | +| barrier_flow.rb:108:13:108:24 | ... == ... | barrier_flow.rb:108:8:108:42 | [true] ... and ... | false | +| barrier_flow.rb:108:13:108:24 | ... == ... | barrier_flow.rb:108:12:108:25 | [false] ( ... ) | false | +| barrier_flow.rb:108:13:108:24 | ... == ... | barrier_flow.rb:108:12:108:25 | [true] ( ... ) | true | +| barrier_flow.rb:108:13:108:24 | ... == ... | barrier_flow.rb:108:31:108:31 | x | false | +| barrier_flow.rb:108:13:108:24 | ... == ... | barrier_flow.rb:109:9:109:14 | self | false | +| barrier_flow.rb:108:31:108:42 | ... != ... | barrier_flow.rb:108:8:108:42 | [true] ... and ... | true | +| barrier_flow.rb:108:31:108:42 | ... != ... | barrier_flow.rb:109:9:109:14 | self | true | +| barrier_flow.rb:118:8:118:19 | ... == ... | barrier_flow.rb:118:8:118:35 | [false] ... or ... | false | +| barrier_flow.rb:118:8:118:19 | ... == ... | barrier_flow.rb:118:24:118:24 | x | false | +| barrier_flow.rb:118:8:118:19 | ... == ... | barrier_flow.rb:121:9:121:14 | self | false | +| barrier_flow.rb:118:8:118:35 | [false] ... or ... | barrier_flow.rb:121:9:121:14 | self | false | +| barrier_flow.rb:118:8:118:35 | [true] ... or ... | barrier_flow.rb:119:9:119:14 | self | true | +| barrier_flow.rb:118:24:118:35 | ... == ... | barrier_flow.rb:118:8:118:35 | [false] ... or ... | false | +| barrier_flow.rb:118:24:118:35 | ... == ... | barrier_flow.rb:121:9:121:14 | self | false | +| barrier_flow.rb:124:8:124:18 | ... != ... | barrier_flow.rb:124:8:124:34 | [false] ... or ... | false | +| barrier_flow.rb:124:8:124:18 | ... != ... | barrier_flow.rb:124:23:124:23 | x | false | +| barrier_flow.rb:124:8:124:18 | ... != ... | barrier_flow.rb:127:9:127:14 | self | false | +| barrier_flow.rb:124:8:124:34 | [false] ... or ... | barrier_flow.rb:127:9:127:14 | self | false | +| barrier_flow.rb:124:8:124:34 | [true] ... or ... | barrier_flow.rb:125:9:125:14 | self | true | +| barrier_flow.rb:124:23:124:34 | ... == ... | barrier_flow.rb:124:8:124:34 | [false] ... or ... | false | +| barrier_flow.rb:124:23:124:34 | ... == ... | barrier_flow.rb:127:9:127:14 | self | false | +| barrier_flow.rb:130:8:130:18 | ... == ... | barrier_flow.rb:130:8:130:34 | [false] ... or ... | false | +| barrier_flow.rb:130:8:130:18 | ... == ... | barrier_flow.rb:130:23:130:23 | x | false | +| barrier_flow.rb:130:8:130:18 | ... == ... | barrier_flow.rb:133:9:133:14 | self | false | +| barrier_flow.rb:130:8:130:34 | [false] ... or ... | barrier_flow.rb:133:9:133:14 | self | false | +| barrier_flow.rb:130:8:130:34 | [true] ... or ... | barrier_flow.rb:131:9:131:14 | self | true | +| barrier_flow.rb:130:23:130:34 | ... != ... | barrier_flow.rb:130:8:130:34 | [false] ... or ... | false | +| barrier_flow.rb:130:23:130:34 | ... != ... | barrier_flow.rb:133:9:133:14 | self | false | +| barrier_flow.rb:136:8:136:18 | ... != ... | barrier_flow.rb:136:8:136:33 | [false] ... or ... | false | +| barrier_flow.rb:136:8:136:18 | ... != ... | barrier_flow.rb:136:23:136:23 | x | false | +| barrier_flow.rb:136:8:136:18 | ... != ... | barrier_flow.rb:139:9:139:14 | self | false | +| barrier_flow.rb:136:8:136:33 | [false] ... or ... | barrier_flow.rb:139:9:139:14 | self | false | +| barrier_flow.rb:136:8:136:33 | [true] ... or ... | barrier_flow.rb:137:9:137:14 | self | true | +| barrier_flow.rb:136:23:136:33 | ... != ... | barrier_flow.rb:136:8:136:33 | [false] ... or ... | false | +| barrier_flow.rb:136:23:136:33 | ... != ... | barrier_flow.rb:139:9:139:14 | self | false | +| barrier_flow.rb:142:8:142:24 | [false] not ... | barrier_flow.rb:142:8:142:39 | [false] ... or ... | false | +| barrier_flow.rb:142:8:142:24 | [false] not ... | barrier_flow.rb:142:29:142:29 | x | false | +| barrier_flow.rb:142:8:142:24 | [false] not ... | barrier_flow.rb:145:9:145:14 | self | false | +| barrier_flow.rb:142:8:142:39 | [false] ... or ... | barrier_flow.rb:145:9:145:14 | self | false | +| barrier_flow.rb:142:8:142:39 | [true] ... or ... | barrier_flow.rb:143:9:143:14 | self | true | +| barrier_flow.rb:142:12:142:24 | [false] ( ... ) | barrier_flow.rb:142:8:142:24 | [true] not ... | false | +| barrier_flow.rb:142:12:142:24 | [true] ( ... ) | barrier_flow.rb:142:8:142:24 | [false] not ... | true | +| barrier_flow.rb:142:12:142:24 | [true] ( ... ) | barrier_flow.rb:142:8:142:39 | [false] ... or ... | true | +| barrier_flow.rb:142:12:142:24 | [true] ( ... ) | barrier_flow.rb:142:29:142:29 | x | true | +| barrier_flow.rb:142:12:142:24 | [true] ( ... ) | barrier_flow.rb:145:9:145:14 | self | true | +| barrier_flow.rb:142:13:142:23 | ... == ... | barrier_flow.rb:142:8:142:24 | [false] not ... | true | +| barrier_flow.rb:142:13:142:23 | ... == ... | barrier_flow.rb:142:8:142:24 | [true] not ... | false | +| barrier_flow.rb:142:13:142:23 | ... == ... | barrier_flow.rb:142:8:142:39 | [false] ... or ... | true | +| barrier_flow.rb:142:13:142:23 | ... == ... | barrier_flow.rb:142:12:142:24 | [false] ( ... ) | false | +| barrier_flow.rb:142:13:142:23 | ... == ... | barrier_flow.rb:142:12:142:24 | [true] ( ... ) | true | +| barrier_flow.rb:142:13:142:23 | ... == ... | barrier_flow.rb:142:29:142:29 | x | true | +| barrier_flow.rb:142:13:142:23 | ... == ... | barrier_flow.rb:145:9:145:14 | self | true | +| barrier_flow.rb:142:29:142:39 | ... != ... | barrier_flow.rb:142:8:142:39 | [false] ... or ... | false | +| barrier_flow.rb:142:29:142:39 | ... != ... | barrier_flow.rb:145:9:145:14 | self | false | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier_flow.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier_flow.rb index d4c2d49eb362..567ceee07e40 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier_flow.rb +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier_flow.rb @@ -77,3 +77,71 @@ def m9(b) sink x end + +def m10 + x = source(10) + + if x == "safe" and x == "safe" then # $ guarded + sink x # $ guarded + else + sink x # $ hasValueFlow=10 + end + + if x != "safe" and x == "safe2" then + sink x # $ guarded + else + sink x # $ hasValueFlow=10 + end + + if x == "safe" and x != "safe2" then # $ guarded + sink x # $ guarded + else + sink x # $ hasValueFlow=10 + end + + if x != "safe1" and x != "safe2" then + sink x # $ hasValueFlow=10 + else + sink x # $ guarded + end + + if not (x == "safe1") and x != "safe2" then + sink x # $ hasValueFlow=10 + else + sink x # $ guarded + end +end + +def m11 + x = source(11) + + if x == "safe1" or x == "safe2" then + sink x # $ guarded + else + sink x # $ hasValueFlow=11 + end + + if x != "safe" or x == "safe2" then # $ guarded + sink x # $ hasValueFlow=11 + else + sink x # $ guarded + end + + if x == "safe" or x != "safe2" then + sink x # $ hasValueFlow=11 + else + sink x # $ guarded + end + + if x != "safe" or x != "safe" then # $ guarded + sink x # $ hasValueFlow=11 + else + sink x # $ guarded + end + + if not (x == "safe") or x != "safe" then # $ guarded + sink x # $ hasValueFlow=11 + else + sink x # $ guarded + end +end \ No newline at end of file diff --git a/shared/ssa/codeql/ssa/Ssa.qll b/shared/ssa/codeql/ssa/Ssa.qll index d3b1414dfcfc..ed278cae75d4 100644 --- a/shared/ssa/codeql/ssa/Ssa.qll +++ b/shared/ssa/codeql/ssa/Ssa.qll @@ -1220,6 +1220,139 @@ module Make Input> { /** Holds if the `i`th node of basic block `bb` evaluates this guard. */ predicate hasCfgNode(BasicBlock bb, int i); + + /** Gets the location of this guard. */ + Location getLocation(); + } + + /** + * A logical operation guard. + * + * Guards are automatically lifted to logical operations, as dictacted + * by the `lift` predicate. For example, logical conjunctions can be + * lifted as follows: + * + * ```csharp + * // Example 1 + * if (x == "safe" && x == "safe") + * { + * sink(x); // guarded + * } + * else + * { + * sink(x); // unguarded + * } + * + * // Example 2 + * if (x == "safe" && x != "safe2") + * { + * sink(x); // guarded + * } + * else + * { + * sink(x); // unguarded + * } + * + * // Example 3 + * if (x != "safe" && x == "safe2") + * { + * sink(x); // guarded + * } + * else + * { + * sink(x); // unguarded + * } + * + * // Example 4 + * if (x != "safe1" && x != "safe2") + * { + * sink(x); // unguarded + * } + * else + * { + * sink(x); // guarded + * } + * ``` + * + * Examples 1--3 should normally be handled via the control-flow graph (CFG) + * implementation, but they are included for completeness. + * + * Example 4, which does not come for free just from the CFG, requires that + * logical conjunctions be modeled post-order in the CFG, as they will + * otherwise not dominate their `else` branch. + * + * Similarly, logical disjunctions can be lifted as follows: + * + * ```csharp + * // Example 1 + * if (x == "safe1" || x == "safe2") + * { + * sink(x); // guarded + * } + * else + * { + * sink(x); // unguarded + * } + * + * // Example 2 + * if (x == "safe" || x != "safe2") + * { + * sink(x); // unguarded + * } + * else + * { + * sink(x); // guarded + * } + * + * // Example 3 + * if (x != "safe" || x == "safe2") + * { + * sink(x); // unguarded + * } + * else + * { + * sink(x); // guarded + * } + * + * // Example 4 + * if (x != "safe" || x != "safe") + * { + * sink(x); // unguarded + * } + * else + * { + * sink(x); // guarded + * } + * ``` + * + * Again, Examples 1--3 should normally be handled via the control-flow graph + * (CFG) implementation, while Example 4 requires that logical disjunctions be + * modeled post-order in the CFG. + */ + class LogicalOperationGuard extends Guard { + /** Gets the `i`th operand of this logical operation. */ + Guard getOperand(int i); + + /** + * Holds if this logical operation guards `branch`, provided that the `i`th + * operand guards its `operandBranch`. The string `id` is used to encode + * the bit-pattern of the operands. + * + * Example: If this operand is a Boolean conjunction, then the following lift + * table should be used: + * + * | id | i | operandBranch | branch | + * |---------------|---|---------------|--------| + * | "false,false" | 0 | false | false | + * | "false,false" | 1 | false | false | + * | "false,true" | 0 | false | true | + * | "false,true" | 1 | true | true | + * | "true,false" | 0 | true | true | + * | "true,false" | 1 | false | true | + * | "true,true" | 0 | true | true | + * | "true,true" | 1 | true | true | + */ + predicate lift(string id, int i, boolean operandBranch, boolean branch); } /** Holds if `guard` controls block `bb` upon evaluating to `branch`. */ @@ -1681,6 +1814,26 @@ module Make Input> { DfInput::Guard g, Definition def, boolean branch, State state ) { guardChecks(g, DfInput::getARead(def), branch, state) + or + exists(int last | + logicalOperationGuardChecksSsaDef(g, _, last, def, branch, state) and + last = strictcount(int i | exists(g.(DfInput::LogicalOperationGuard).getOperand(i))) - 1 + ) + } + + pragma[nomagic] + private predicate logicalOperationGuardChecksSsaDef( + DfInput::LogicalOperationGuard op, string id, int i, Definition def, boolean branch, + State state + ) { + exists(boolean operandBranch | + op.lift(id, i, operandBranch, branch) and + guardChecksSsaDef(op.getOperand(i), def, operandBranch, state) + | + i = 0 + or + logicalOperationGuardChecksSsaDef(op, id, i - 1, def, branch, state) + ) } /** Gets a node that is safely guarded by the given guard check. */