From b14a889f5f849d28c8494cb15e172363d10060a5 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 8 Dec 2020 11:05:04 +0100 Subject: [PATCH] CFG: Use `MatchingCompletion` for parameters with default values --- .../controlflow/internal/Completion.qll | 2 ++ .../internal/ControlFlowGraphImpl.qll | 35 ++++++++++++++++--- .../controlflow/graph/Cfg.expected | 22 +++++++++--- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/ql/src/codeql_ruby/controlflow/internal/Completion.qll b/ql/src/codeql_ruby/controlflow/internal/Completion.qll index 0bb18ac5bbb..3b86799b8c9 100644 --- a/ql/src/codeql_ruby/controlflow/internal/Completion.qll +++ b/ql/src/codeql_ruby/controlflow/internal/Completion.qll @@ -197,6 +197,8 @@ private predicate inMatchingContext(AstNode n) { c.getChild(_) = w and w.getPattern(_).getChild() = n ) + or + n.(Trees::DefaultValueParameterTree).hasDefaultValue() } /** diff --git a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll index 3a4cf09ca69..37658c3d64b 100644 --- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll +++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll @@ -392,6 +392,33 @@ module Trees { private class ConstantTree extends LeafTree, Constant { } + /** A parameter that may have a default value. */ + abstract class DefaultValueParameterTree extends PreOrderTree { + abstract AstNode getDefaultValue(); + + predicate hasDefaultValue() { exists(this.getDefaultValue()) } + + final override predicate propagatesAbnormal(AstNode child) { child = this.getDefaultValue() } + + final override predicate last(AstNode last, Completion c) { + last = this and + exists(this.getDefaultValue()) and + c.(MatchingCompletion).getValue() = true + or + last(this.getDefaultValue(), last, c) + or + last = this and + not exists(this.getDefaultValue()) and + c instanceof SimpleCompletion + } + + final override predicate succ(AstNode pred, AstNode succ, Completion c) { + pred = this and + first(this.getDefaultValue(), succ) and + c.(MatchingCompletion).getValue() = false + } + } + private class DestructuredLeftAssignmentTree extends StandardPostOrderTree, DestructuredLeftAssignment { final override AstNode getChildNode(int i) { result = this.getChild(i) } @@ -688,8 +715,8 @@ module Trees { final override AstNode getChildNode(int i) { result = this.getChild() and i = 0 } } - private class KeywordParameterTree extends StandardPostOrderTree, KeywordParameter { - final override AstNode getChildNode(int i) { result = this.getValue() and i = 0 } + private class KeywordParameterTree extends DefaultValueParameterTree, KeywordParameter { + final override AstNode getDefaultValue() { result = this.getValue() } } private class LambdaTree extends StandardPreOrderTree, Lambda { @@ -820,8 +847,8 @@ module Trees { } } - private class OptionalParameterTree extends StandardPostOrderTree, OptionalParameter { - final override AstNode getChildNode(int i) { result = this.getValue() and i = 0 } + private class OptionalParameterTree extends DefaultValueParameterTree, OptionalParameter { + final override AstNode getDefaultValue() { result = this.getValue() } } private class PairTree extends StandardPostOrderTree, Pair { diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected index 063725dd7db..13d02efa769 100644 --- a/ql/test/library-tests/controlflow/graph/Cfg.expected +++ b/ql/test/library-tests/controlflow/graph/Cfg.expected @@ -1072,9 +1072,15 @@ nodes | raise.rb:121:1:124:3 | enter m10 | | raise.rb:121:1:124:3 | exit m10 | | raise.rb:121:1:124:3 | exit m10 (abnormal) | +| raise.rb:121:1:124:3 | exit m10 (normal) | +| raise.rb:121:9:121:31 | OptionalParameter | | raise.rb:121:14:121:18 | raise | | raise.rb:121:14:121:30 | MethodCall | | raise.rb:121:20:121:30 | String | +| raise.rb:122:1:123:51 | Ensure | +| raise.rb:123:3:123:6 | puts | +| raise.rb:123:3:123:51 | MethodCall | +| raise.rb:123:8:123:51 | String | edges | break_ensure.rb:1:1:11:3 | enter m1 | break_ensure.rb:1:8:1:15 | elements | semmle.label | successor | | break_ensure.rb:1:1:11:3 | exit m1 (normal) | break_ensure.rb:1:1:11:3 | exit m1 | semmle.label | successor | @@ -1430,10 +1436,11 @@ edges | cfg.rb:98:25:98:27 | String | cfg.rb:98:18:98:27 | Pair | semmle.label | successor | | cfg.rb:98:30:98:35 | HashSplatArgument | cfg.rb:98:8:98:36 | Hash | semmle.label | successor | | cfg.rb:98:32:98:35 | map1 | cfg.rb:98:30:98:35 | HashSplatArgument | semmle.label | successor | -| cfg.rb:101:1:104:3 | enter parameters | cfg.rb:101:24:101:25 | 42 | semmle.label | successor | +| cfg.rb:101:1:104:3 | enter parameters | cfg.rb:101:16:101:25 | OptionalParameter | semmle.label | successor | | cfg.rb:101:1:104:3 | exit parameters (normal) | cfg.rb:101:1:104:3 | exit parameters | semmle.label | successor | -| cfg.rb:101:16:101:25 | OptionalParameter | cfg.rb:101:28:101:31 | KeywordParameter | semmle.label | successor | -| cfg.rb:101:24:101:25 | 42 | cfg.rb:101:16:101:25 | OptionalParameter | semmle.label | successor | +| cfg.rb:101:16:101:25 | OptionalParameter | cfg.rb:101:24:101:25 | 42 | semmle.label | no-match | +| cfg.rb:101:16:101:25 | OptionalParameter | cfg.rb:101:28:101:31 | KeywordParameter | semmle.label | match | +| cfg.rb:101:24:101:25 | 42 | cfg.rb:101:28:101:31 | KeywordParameter | semmle.label | successor | | cfg.rb:101:28:101:31 | KeywordParameter | cfg.rb:101:34:101:41 | HashSplatParameter | semmle.label | successor | | cfg.rb:101:34:101:41 | HashSplatParameter | cfg.rb:102:8:102:12 | value | semmle.label | successor | | cfg.rb:102:3:102:6 | puts | cfg.rb:102:3:102:12 | MethodCall | semmle.label | successor | @@ -2180,8 +2187,15 @@ edges | raise.rb:117:11:117:22 | String | raise.rb:117:5:117:9 | raise | semmle.label | successor | | raise.rb:117:11:117:22 | [ensure: raise] String | raise.rb:117:5:117:9 | [ensure: raise] raise | semmle.label | successor | | raise.rb:117:11:117:22 | [ensure: return] String | raise.rb:117:5:117:9 | [ensure: return] raise | semmle.label | successor | -| raise.rb:121:1:124:3 | enter m10 | raise.rb:121:20:121:30 | String | semmle.label | successor | +| raise.rb:121:1:124:3 | enter m10 | raise.rb:121:9:121:31 | OptionalParameter | semmle.label | successor | | raise.rb:121:1:124:3 | exit m10 (abnormal) | raise.rb:121:1:124:3 | exit m10 | semmle.label | successor | +| raise.rb:121:1:124:3 | exit m10 (normal) | raise.rb:121:1:124:3 | exit m10 | semmle.label | successor | +| raise.rb:121:9:121:31 | OptionalParameter | raise.rb:121:20:121:30 | String | semmle.label | no-match | +| raise.rb:121:9:121:31 | OptionalParameter | raise.rb:122:1:123:51 | Ensure | semmle.label | match | | raise.rb:121:14:121:18 | raise | raise.rb:121:14:121:30 | MethodCall | semmle.label | successor | | raise.rb:121:14:121:30 | MethodCall | raise.rb:121:1:124:3 | exit m10 (abnormal) | semmle.label | raise | | raise.rb:121:20:121:30 | String | raise.rb:121:14:121:18 | raise | semmle.label | successor | +| raise.rb:122:1:123:51 | Ensure | raise.rb:123:8:123:51 | String | semmle.label | successor | +| raise.rb:123:3:123:6 | puts | raise.rb:123:3:123:51 | MethodCall | semmle.label | successor | +| raise.rb:123:3:123:51 | MethodCall | raise.rb:121:1:124:3 | exit m10 (normal) | semmle.label | successor | +| raise.rb:123:8:123:51 | String | raise.rb:123:3:123:6 | puts | semmle.label | successor |