From c86f59715381ee9064f51abba1a8b431a8bc08f1 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Nov 2022 08:25:08 +0100 Subject: [PATCH 1/3] Ruby: Add test for disjunctive guard --- .../barrier-guards/barrier-guards.expected | 32 ++++++++++++++++++- .../dataflow/barrier-guards/barrier-guards.ql | 8 +++++ .../dataflow/barrier-guards/barrier-guards.rb | 6 ++++ 3 files changed, 45 insertions(+), 1 deletion(-) 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 783d414dad6b..393b2beb304e 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 @@ -1,4 +1,4 @@ -WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:8,3-15) +WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:9,3-15) oldStyleBarrierGuards | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true | | barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true | @@ -20,3 +20,33 @@ newStyleBarrierGuards | barrier-guards.rb:71:5:71:7 | foo | | barrier-guards.rb:83:5:83:7 | foo | | barrier-guards.rb:91:5:91:7 | foo | +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 | +| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | true | +| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:12:5:12:7 | foo | false | +| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:16:5:16:7 | foo | true | +| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | false | +| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:22:5:22:7 | foo | false | +| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | true | +| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | false | +| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:30:5:30:7 | foo | true | +| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:38:5:38:7 | foo | true | +| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:40:5:40:7 | foo | false | +| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:44:5:46:5 | self | true | +| barrier-guards.rb:49:4:49:15 | ... == ... | barrier-guards.rb:50:5:53:5 | self | true | +| barrier-guards.rb:56:4:56:15 | ... == ... | barrier-guards.rb:57:5:57:13 | my_lambda | true | +| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:71:5:71:7 | foo | true | +| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:73:5:73:7 | foo | false | +| barrier-guards.rb:76:4:76:21 | call to include? | barrier-guards.rb:77:5:77:7 | foo | true | +| barrier-guards.rb:76:4:76:21 | call to include? | barrier-guards.rb:79:5:79:7 | foo | false | +| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | true | +| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:85:5:85:7 | foo | false | +| barrier-guards.rb:88:4:88:25 | ... == ... | barrier-guards.rb:89:5:89:7 | foo | true | +| barrier-guards.rb:88:4:88:25 | ... == ... | barrier-guards.rb:91:5:91:7 | foo | false | +| barrier-guards.rb:96:4:96:12 | call to condition | barrier-guards.rb:97:5:97:8 | bars | true | +| barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:101:5:101:7 | foo | true | +| barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:103:5:103:7 | foo | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:9:106:9 | self | false | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql index 84a962ade358..7998164b6da1 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql @@ -2,6 +2,7 @@ import codeql.ruby.dataflow.internal.DataFlowPublic import codeql.ruby.dataflow.BarrierGuards import codeql.ruby.controlflow.CfgNodes import codeql.ruby.controlflow.ControlFlowGraph +import codeql.ruby.controlflow.BasicBlocks import codeql.ruby.DataFlow query predicate oldStyleBarrierGuards( @@ -14,3 +15,10 @@ query predicate newStyleBarrierGuards(DataFlow::Node n) { n instanceof StringConstCompareBarrier or n instanceof StringConstArrayInclusionCallBarrier } + +query predicate controls(CfgNode condition, BasicBlock bb, SuccessorTypes::ConditionalSuccessor s) { + exists(ConditionBlock cb | + cb.controls(bb, s) and + condition = cb.getLastNode() + ) +} diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb index bc9599fd9269..21a878e938c4 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb @@ -102,3 +102,9 @@ else foo end + +if x or y then + foo +else + foo +end From 7ba068229734f68165abf0b082831a1b1d999522 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Nov 2022 08:59:59 +0100 Subject: [PATCH 2/3] Ruby: Split basic blocks around constant conditionals --- .../codeql/ruby/controlflow/BasicBlocks.qll | 24 +++++++++++++++++++ .../barrier-guards/barrier-guards.expected | 4 ++++ 2 files changed, 28 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll index 9ce0bf32fd71..300450545039 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll @@ -252,6 +252,30 @@ private module Cached { cfn.isJoin() or cfn.getAPredecessor().isBranch() + or + /* + * In cases such as + * + * ```rb + * if x or y + * foo + * else + * bar + * ``` + * + * we have a CFG that looks like + * + * x --false--> [false] x or y --false--> bar + * \ | + * --true--> y --false-- + * \ + * --true--> [true] x or y --true--> foo + * + * and we want to ensure that both `foo` and `bar` start a new basic block, + * in order to get a `ConditionalBlock` out of the disjunction. + */ + + exists(cfn.getAPredecessor(any(SuccessorTypes::ConditionalSuccessor s))) } /** 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 393b2beb304e..d2f5df8fcd95 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 @@ -49,4 +49,8 @@ controls | barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:103:5:103:7 | foo | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:9:106:9 | self | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:4:106:9 | [false] ... or ... | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:4:106:9 | [true] ... or ... | barrier-guards.rb:107:5:107:7 | foo | true | | barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:109:5:109:7 | foo | false | From f0b9ca4bf9dfd54b3d469c2ac6da77b4f3e1a6cc Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Nov 2022 11:09:54 +0100 Subject: [PATCH 3/3] Ruby: Add more guards tests --- .../barrier-guards/barrier-guards.expected | 19 ++++++++++++++++--- .../dataflow/barrier-guards/barrier-guards.rb | 12 ++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) 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 d2f5df8fcd95..a50542e8df36 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 @@ -49,8 +49,21 @@ controls | barrier-guards.rb:100:4:100:21 | call to include? | barrier-guards.rb:103:5:103:7 | foo | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | | barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:106:9:106:9 | self | false | -| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:109:5:109:7 | foo | false | -| barrier-guards.rb:106:4:106:9 | [false] ... or ... | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:4:106:4 | call to x | barrier-guards.rb:109:5:109:8 | bars | false | +| barrier-guards.rb:106:4:106:9 | [false] ... or ... | barrier-guards.rb:109:5:109:8 | bars | false | | barrier-guards.rb:106:4:106:9 | [true] ... or ... | barrier-guards.rb:107:5:107:7 | foo | true | | barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:106:4:106:9 | [false] ... or ... | false | -| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:109:5:109:7 | foo | false | +| barrier-guards.rb:106:9:106:9 | call to y | barrier-guards.rb:109:5:109:8 | bars | false | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:112:4:112:10 | [true] ... and ... | true | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:112:10:112:10 | self | true | +| barrier-guards.rb:112:4:112:4 | call to x | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:112:4:112:10 | [false] ... and ... | barrier-guards.rb:115:5:115:8 | bars | false | +| barrier-guards.rb:112:4:112:10 | [true] ... and ... | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:112:10:112:10 | call to y | barrier-guards.rb:112:4:112:10 | [true] ... and ... | true | +| barrier-guards.rb:112:10:112:10 | call to y | barrier-guards.rb:113:5:113:7 | foo | true | +| barrier-guards.rb:118:4:118:8 | [false] not ... | barrier-guards.rb:121:5:121:8 | bars | false | +| barrier-guards.rb:118:4:118:8 | [true] not ... | barrier-guards.rb:119:5:119:7 | foo | true | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [false] not ... | true | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [true] not ... | false | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:119:5:119:7 | foo | false | +| barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:121:5:121:8 | bars | true | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb index 21a878e938c4..5ead7bc04cf2 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb @@ -106,5 +106,17 @@ if x or y then foo else + bars +end + +if x and y then foo +else + bars +end + +if not x then + foo +else + bars end