From 290d3decc8c9e3d5c1641e5e2be13e262d6aee89 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Mon, 23 Nov 2020 14:25:30 +0100 Subject: [PATCH 1/4] Add consistency query for Variables Test that VariableAccess.getVariable returns a unique Variable --- ql/consistency-queries/VariablesConsistency.ql | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ql/consistency-queries/VariablesConsistency.ql diff --git a/ql/consistency-queries/VariablesConsistency.ql b/ql/consistency-queries/VariablesConsistency.ql new file mode 100644 index 00000000000..6ef6598f89b --- /dev/null +++ b/ql/consistency-queries/VariablesConsistency.ql @@ -0,0 +1,6 @@ +import codeql_ruby.Variables + +query predicate ambiguousVariable(VariableAccess access, Variable variable) { + access.getVariable() = variable and + count(access.getVariable()) > 1 +} From c745978ebbac8c603a49c729e0052e0b07e7693b Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Mon, 23 Nov 2020 14:27:40 +0100 Subject: [PATCH 2/4] Fix inconsistent variable references --- ql/src/codeql_ruby/Variables.qll | 44 +++++--- ql/test/library-tests/variables/parameters.rb | 58 ++++++++++ .../variables/varaccess.expected | 102 ++++++++++++++++++ .../library-tests/variables/variable.expected | 46 ++++++++ .../variables/varscopes.expected | 13 +++ 5 files changed, 247 insertions(+), 16 deletions(-) create mode 100644 ql/test/library-tests/variables/parameters.rb diff --git a/ql/src/codeql_ruby/Variables.qll b/ql/src/codeql_ruby/Variables.qll index 0cb83aaee41..f605e0c272e 100644 --- a/ql/src/codeql_ruby/Variables.qll +++ b/ql/src/codeql_ruby/Variables.qll @@ -13,24 +13,36 @@ private VariableScope enclosingScope(AstNode node) { result.getScopeElement() = parent*(node.getParent()) } +private string parameterName(AstNode node) { + result = node.(Identifier).getValue() or + result = node.(SplatParameter).getName().getValue() or + result = node.(HashSplatParameter).getName().getValue() or + result = node.(BlockParameter).getName().getValue() or + result = node.(OptionalParameter).getName().getValue() or + result = node.(KeywordParameter).getName().getValue() or + result = parameterName(node.(DestructuredParameter).getAFieldOrChild()) +} + /** Holds if `scope` defines `name` as a parameter. */ private predicate scopeDefinesParameter(VariableScope scope, string name, Location location) { - exists(Identifier var | - name = var.getValue() and - location = var.getLocation() and - var in [scope - .(BlockScope) - .getScopeElement() - .getAFieldOrChild() - .(BlockParameters) - .getAFieldOrChild+(), - scope - .(MethodScope) - .getScopeElement() - .getAFieldOrChild() - .(MethodParameters) - .getAFieldOrChild+()] - ) + location = + min(AstNode var | + name = parameterName(var) and + var in [scope + .(BlockScope) + .getScopeElement() + .getAFieldOrChild() + .(BlockParameters) + .getAFieldOrChild(), + scope + .(MethodScope) + .getScopeElement() + .getAFieldOrChild() + .(MethodParameters) + .getAFieldOrChild()] + | + var.getLocation() as loc order by loc.getStartLine(), loc.getStartColumn() + ) } /** Holds if `var` is assigned in `scope`. */ diff --git a/ql/test/library-tests/variables/parameters.rb b/ql/test/library-tests/variables/parameters.rb new file mode 100644 index 00000000000..99aff4df5ae --- /dev/null +++ b/ql/test/library-tests/variables/parameters.rb @@ -0,0 +1,58 @@ +1.times do | x ; y| + y = 5 + puts x + puts y +end + +def order_pizza(client, *pizzas) + if pizzas.count == 1 + puts "1 pizza for #{client}!" + else + puts "#{ pizzas.count} pizzas for #{client}!" + end +end + +def print_map(**map) + map.each do |key, value| + puts "#{key} = #{value}" + end +end + +def call_block(&block) + block.call +end + +def opt_param(name = 'unknown', size = name.length) + puts name + puts size +end + +def key_param(first: , middle: '', last:) + puts "#{first} #{middle} #{last}" +end + +b = 2 +def multi(a = (b = 5)) + # `a` is a parameter and `b` is a new variable + puts "#{a} #{b}" +end + +def multi2(d: e = 4) + # `d` is a parameter and `e` is a local variable + puts "#{d} #{e}" +end + +def dup_underscore(_,_) + puts _ # binds to the first _ +end + +def tuples((a,b)) + puts "#{a} #{b}" +end + +x = 10 +1.times do | y = (x = 1)| + puts x + puts y +end + diff --git a/ql/test/library-tests/variables/varaccess.expected b/ql/test/library-tests/variables/varaccess.expected index 899db88f637..44ae78509d7 100644 --- a/ql/test/library-tests/variables/varaccess.expected +++ b/ql/test/library-tests/variables/varaccess.expected @@ -24,6 +24,57 @@ variableAccess | nested_scopes.rb:38:8:38:8 | a | nested_scopes.rb:5:3:5:3 | a | nested_scopes.rb:4:1:39:3 | class scope | | nested_scopes.rb:40:1:40:1 | d | nested_scopes.rb:40:1:40:1 | d | nested_scopes.rb:1:1:42:1 | top-level scope | | nested_scopes.rb:41:1:41:1 | d | nested_scopes.rb:40:1:40:1 | d | nested_scopes.rb:1:1:42:1 | top-level scope | +| parameters.rb:1:14:1:14 | x | parameters.rb:1:14:1:14 | x | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:1:18:1:18 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:2:4:2:4 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:3:9:3:9 | x | parameters.rb:1:14:1:14 | x | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:4:9:4:9 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:7:17:7:22 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:8:6:8:11 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:9:25:9:30 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:11:14:11:19 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:11:41:11:46 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:15:17:15:19 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:16:3:16:5 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:16:16:16:18 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:16:21:16:25 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:17:13:17:15 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:17:22:17:26 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:21:17:21:21 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:22:3:22:7 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:25:15:25:18 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:33:25:36 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:40:25:43 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:26:8:26:11 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:27:8:27:11 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:30:15:30:19 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:24:30:29 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:36:30:39 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:11:31:15 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:20:31:25 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:30:31:33 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:34:1:34:1 | b | parameters.rb:34:1:34:1 | b | parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:35:11:35:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:35:16:35:16 | b | parameters.rb:35:16:35:16 | b | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:37:11:37:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:37:16:37:16 | b | parameters.rb:35:16:35:16 | b | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:40:12:40:12 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:40:15:40:15 | e | parameters.rb:40:15:40:15 | e | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:42:11:42:11 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:42:16:42:16 | e | parameters.rb:40:15:40:15 | e | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:45:20:45:20 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | +| parameters.rb:45:22:45:22 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | +| parameters.rb:46:8:46:8 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | +| parameters.rb:49:13:49:13 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:49:15:49:15 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:11:50:11 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:16:50:16 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:53:1:53:1 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:54:14:54:14 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | +| parameters.rb:54:19:54:19 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:55:9:55:9 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:56:9:56:9 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | | scopes.rb:2:14:2:14 | x | scopes.rb:2:14:2:14 | x | scopes.rb:2:9:6:3 | block scope | | scopes.rb:4:4:4:4 | a | scopes.rb:4:4:4:4 | a | scopes.rb:2:9:6:3 | block scope | | scopes.rb:5:9:5:9 | a | scopes.rb:4:4:4:4 | a | scopes.rb:2:9:6:3 | block scope | @@ -44,6 +95,49 @@ parameterAccess | nested_scopes.rb:18:34:18:34 | a | nested_scopes.rb:16:29:16:29 | a | nested_scopes.rb:16:21:19:15 | block scope | | nested_scopes.rb:22:21:22:21 | a | nested_scopes.rb:22:21:22:21 | a | nested_scopes.rb:22:9:24:11 | method scope | | nested_scopes.rb:23:16:23:16 | a | nested_scopes.rb:22:21:22:21 | a | nested_scopes.rb:22:9:24:11 | method scope | +| parameters.rb:1:14:1:14 | x | parameters.rb:1:14:1:14 | x | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:1:18:1:18 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:2:4:2:4 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:3:9:3:9 | x | parameters.rb:1:14:1:14 | x | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:4:9:4:9 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | +| parameters.rb:7:17:7:22 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:8:6:8:11 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:9:25:9:30 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:11:14:11:19 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:11:41:11:46 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:15:17:15:19 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:16:3:16:5 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:16:16:16:18 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:16:21:16:25 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:17:13:17:15 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:17:22:17:26 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | +| parameters.rb:21:17:21:21 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:22:3:22:7 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:25:15:25:18 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:33:25:36 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:40:25:43 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:26:8:26:11 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:27:8:27:11 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:30:15:30:19 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:24:30:29 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:36:30:39 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:11:31:15 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:20:31:25 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:30:31:33 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:35:11:35:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:37:11:37:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:40:12:40:12 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:42:11:42:11 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:45:20:45:20 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | +| parameters.rb:45:22:45:22 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | +| parameters.rb:46:8:46:8 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | +| parameters.rb:49:13:49:13 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:49:15:49:15 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:11:50:11 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:16:50:16 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:54:14:54:14 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | +| parameters.rb:56:9:56:9 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | | scopes.rb:2:14:2:14 | x | scopes.rb:2:14:2:14 | x | scopes.rb:2:9:6:3 | block scope | | scopes.rb:9:14:9:14 | x | scopes.rb:9:14:9:14 | x | scopes.rb:9:9:13:3 | block scope | localVariableAccess @@ -62,6 +156,14 @@ localVariableAccess | nested_scopes.rb:38:8:38:8 | a | nested_scopes.rb:5:3:5:3 | a | nested_scopes.rb:4:1:39:3 | class scope | | nested_scopes.rb:40:1:40:1 | d | nested_scopes.rb:40:1:40:1 | d | nested_scopes.rb:1:1:42:1 | top-level scope | | nested_scopes.rb:41:1:41:1 | d | nested_scopes.rb:40:1:40:1 | d | nested_scopes.rb:1:1:42:1 | top-level scope | +| parameters.rb:34:1:34:1 | b | parameters.rb:34:1:34:1 | b | parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:35:16:35:16 | b | parameters.rb:35:16:35:16 | b | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:37:16:37:16 | b | parameters.rb:35:16:35:16 | b | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:40:15:40:15 | e | parameters.rb:40:15:40:15 | e | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:42:16:42:16 | e | parameters.rb:40:15:40:15 | e | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:53:1:53:1 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:54:19:54:19 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:55:9:55:9 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | | scopes.rb:4:4:4:4 | a | scopes.rb:4:4:4:4 | a | scopes.rb:2:9:6:3 | block scope | | scopes.rb:5:9:5:9 | a | scopes.rb:4:4:4:4 | a | scopes.rb:2:9:6:3 | block scope | | scopes.rb:7:1:7:1 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:13:3 | top-level scope | diff --git a/ql/test/library-tests/variables/variable.expected b/ql/test/library-tests/variables/variable.expected index 6d2277e0b14..4010f77dd66 100644 --- a/ql/test/library-tests/variables/variable.expected +++ b/ql/test/library-tests/variables/variable.expected @@ -11,6 +11,29 @@ variable | nested_scopes.rb:22:21:22:21 | a | | nested_scopes.rb:31:11:31:11 | a | | nested_scopes.rb:40:1:40:1 | d | +| parameters.rb:1:14:1:14 | x | +| parameters.rb:1:18:1:18 | y | +| parameters.rb:7:17:7:22 | client | +| parameters.rb:7:25:7:31 | pizzas | +| parameters.rb:15:15:15:19 | map | +| parameters.rb:16:16:16:18 | key | +| parameters.rb:16:21:16:25 | value | +| parameters.rb:21:16:21:21 | block | +| parameters.rb:25:15:25:30 | name | +| parameters.rb:25:33:25:50 | size | +| parameters.rb:30:15:30:20 | first | +| parameters.rb:30:24:30:33 | middle | +| parameters.rb:30:36:30:40 | last | +| parameters.rb:34:1:34:1 | b | +| parameters.rb:35:11:35:21 | a | +| parameters.rb:35:16:35:16 | b | +| parameters.rb:40:12:40:19 | d | +| parameters.rb:40:15:40:15 | e | +| parameters.rb:45:20:45:20 | _ | +| parameters.rb:49:12:49:16 | a | +| parameters.rb:49:12:49:16 | b | +| parameters.rb:53:1:53:1 | x | +| parameters.rb:54:14:54:24 | y | | scopes.rb:2:14:2:14 | x | | scopes.rb:4:4:4:4 | a | | scopes.rb:7:1:7:1 | a | @@ -21,6 +44,25 @@ parameter | nested_scopes.rb:16:29:16:29 | a | | nested_scopes.rb:18:26:18:26 | x | | nested_scopes.rb:22:21:22:21 | a | +| parameters.rb:1:14:1:14 | x | +| parameters.rb:1:18:1:18 | y | +| parameters.rb:7:17:7:22 | client | +| parameters.rb:7:25:7:31 | pizzas | +| parameters.rb:15:15:15:19 | map | +| parameters.rb:16:16:16:18 | key | +| parameters.rb:16:21:16:25 | value | +| parameters.rb:21:16:21:21 | block | +| parameters.rb:25:15:25:30 | name | +| parameters.rb:25:33:25:50 | size | +| parameters.rb:30:15:30:20 | first | +| parameters.rb:30:24:30:33 | middle | +| parameters.rb:30:36:30:40 | last | +| parameters.rb:35:11:35:21 | a | +| parameters.rb:40:12:40:19 | d | +| parameters.rb:45:20:45:20 | _ | +| parameters.rb:49:12:49:16 | a | +| parameters.rb:49:12:49:16 | b | +| parameters.rb:54:14:54:24 | y | | scopes.rb:2:14:2:14 | x | | scopes.rb:9:14:9:14 | x | localVariable @@ -31,5 +73,9 @@ localVariable | nested_scopes.rb:13:11:13:11 | a | | nested_scopes.rb:31:11:31:11 | a | | nested_scopes.rb:40:1:40:1 | d | +| parameters.rb:34:1:34:1 | b | +| parameters.rb:35:16:35:16 | b | +| parameters.rb:40:15:40:15 | e | +| parameters.rb:53:1:53:1 | x | | scopes.rb:4:4:4:4 | a | | scopes.rb:7:1:7:1 | a | diff --git a/ql/test/library-tests/variables/varscopes.expected b/ql/test/library-tests/variables/varscopes.expected index 88a58f78a71..7a09f598183 100644 --- a/ql/test/library-tests/variables/varscopes.expected +++ b/ql/test/library-tests/variables/varscopes.expected @@ -11,6 +11,19 @@ | nested_scopes.rb:22:9:24:11 | method scope | | nested_scopes.rb:27:7:29:9 | method scope | | nested_scopes.rb:30:7:33:9 | class scope | +| parameters.rb:1:1:58:1 | top-level scope | +| parameters.rb:1:9:5:3 | block scope | +| parameters.rb:7:1:13:3 | method scope | +| parameters.rb:15:1:19:3 | method scope | +| parameters.rb:16:12:18:5 | block scope | +| parameters.rb:21:1:23:3 | method scope | +| parameters.rb:25:1:28:3 | method scope | +| parameters.rb:30:1:32:3 | method scope | +| parameters.rb:35:1:38:3 | method scope | +| parameters.rb:40:1:43:3 | method scope | +| parameters.rb:45:1:47:3 | method scope | +| parameters.rb:49:1:51:3 | method scope | +| parameters.rb:54:9:57:3 | block scope | | scopes.rb:1:1:1:15 | method scope | | scopes.rb:1:1:13:3 | top-level scope | | scopes.rb:2:9:6:3 | block scope | From bc5d7a3b745f339fb1d086a423cf6a79291e88ee Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Tue, 24 Nov 2020 18:27:03 +0100 Subject: [PATCH 3/4] Change modelling of Parameters --- ql/src/codeql_ruby/Variables.qll | 104 ++++++++-------- .../variables/parameter.expected | 29 +++++ ql/test/library-tests/variables/parameter.ql | 5 + .../variables/varaccess.expected | 112 +++++++++--------- ql/test/library-tests/variables/varaccess.ql | 5 +- .../library-tests/variables/variable.expected | 52 ++++---- ql/test/library-tests/variables/variable.ql | 4 +- 7 files changed, 171 insertions(+), 140 deletions(-) create mode 100644 ql/test/library-tests/variables/parameter.expected create mode 100644 ql/test/library-tests/variables/parameter.ql diff --git a/ql/src/codeql_ruby/Variables.qll b/ql/src/codeql_ruby/Variables.qll index f605e0c272e..d7171c78b53 100644 --- a/ql/src/codeql_ruby/Variables.qll +++ b/ql/src/codeql_ruby/Variables.qll @@ -13,35 +13,50 @@ private VariableScope enclosingScope(AstNode node) { result.getScopeElement() = parent*(node.getParent()) } -private string parameterName(AstNode node) { - result = node.(Identifier).getValue() or - result = node.(SplatParameter).getName().getValue() or - result = node.(HashSplatParameter).getName().getValue() or - result = node.(BlockParameter).getName().getValue() or - result = node.(OptionalParameter).getName().getValue() or - result = node.(KeywordParameter).getName().getValue() or - result = parameterName(node.(DestructuredParameter).getAFieldOrChild()) +/** A parameter. */ +class Parameter extends AstNode { + int position; + VariableScope scope; + + Parameter() { + this = + scope.(BlockScope).getScopeElement().getAFieldOrChild().(BlockParameters).getChild(position) + or + this = + scope.(MethodScope).getScopeElement().getAFieldOrChild().(MethodParameters).getChild(position) + } + + final int getPosition() { result = position } + + final VariableScope getDeclaringScope() { result = scope } + + final ParameterAccess getAnAccess() { result.getParameter() = this } } -/** Holds if `scope` defines `name` as a parameter. */ +private Identifier parameterIdentifier(Parameter p) { + result = p or + result = p.(SplatParameter).getName() or + result = p.(HashSplatParameter).getName() or + result = p.(BlockParameter).getName() or + result = p.(OptionalParameter).getName() or + result = p.(KeywordParameter).getName() or + result = destructuredIdentifier(p.(DestructuredParameter)) +} + +private Identifier destructuredIdentifier(AstNode node) { + result = node or + result = destructuredIdentifier(node.(DestructuredParameter).getAFieldOrChild()) +} + +/** Holds if `scope` defines `name` in its parameter declaration. */ private predicate scopeDefinesParameter(VariableScope scope, string name, Location location) { location = - min(AstNode var | - name = parameterName(var) and - var in [scope - .(BlockScope) - .getScopeElement() - .getAFieldOrChild() - .(BlockParameters) - .getAFieldOrChild(), - scope - .(MethodScope) - .getScopeElement() - .getAFieldOrChild() - .(MethodParameters) - .getAFieldOrChild()] + min(Parameter p, Identifier i | + scope = p.getDeclaringScope() and + i = parameterIdentifier(p) and + name = i.getValue() | - var.getLocation() as loc order by loc.getStartLine(), loc.getStartColumn() + i.getLocation() as loc order by loc.getStartLine(), loc.getStartColumn() ) } @@ -89,10 +104,9 @@ private module Cached { cached newtype TVariable = - TParameter(VariableScope scope, string name, Location location) { - scopeDefinesParameter(scope, name, location) - } or TLocalVariable(VariableScope scope, string name, Location location) { + scopeDefinesParameter(scope, name, location) + or not scopeDefinesParameter(scope, name, _) and not blockScopeInherits(scope, name, _) and location = @@ -158,23 +172,6 @@ class Variable extends TVariable { VariableAccess getAnAccess() { result.getVariable() = this } } -/** A parameter. */ -class Parameter extends Variable { - private VariableScope scope; - private string name; - private Location location; - - Parameter() { this = TParameter(scope, name, location) } - - final override string getName() { result = name } - - final override Location getLocation() { result = location } - - final override VariableScope getDeclaringScope() { result = scope } - - final override ParameterAccess getAnAccess() { result = super.getAnAccess() } -} - /** A local variable. */ class LocalVariable extends Variable { private VariableScope scope; @@ -188,8 +185,6 @@ class LocalVariable extends Variable { final override Location getLocation() { result = location } final override VariableScope getDeclaringScope() { result = scope } - - final override LocalVariableAccess getAnAccess() { result = super.getAnAccess() } } /** An identifier that refers to a variable. */ @@ -206,16 +201,17 @@ class VariableAccess extends Identifier { /** An identifier that refers to a parameter. */ class ParameterAccess extends VariableAccess { - override Parameter variable; + Parameter parameter; - final override Parameter getVariable() { result = variable } -} + ParameterAccess() { + exists(Identifier i | + i = parameterIdentifier(parameter) and + variable.getDeclaringScope() = parameter.getDeclaringScope() and + variable.getLocation() = i.getLocation() + ) + } -/** An identifier that refers to a local variable. */ -class LocalVariableAccess extends VariableAccess { - override LocalVariable variable; - - final override LocalVariable getVariable() { result = super.getVariable() } + final Parameter getParameter() { result = parameter } } /** A top-level scope. */ diff --git a/ql/test/library-tests/variables/parameter.expected b/ql/test/library-tests/variables/parameter.expected new file mode 100644 index 00000000000..1e3167d187d --- /dev/null +++ b/ql/test/library-tests/variables/parameter.expected @@ -0,0 +1,29 @@ +parameter +| nested_scopes.rb:15:23:15:23 | a | nested_scopes.rb:15:23:15:23 | a | +| nested_scopes.rb:16:26:16:26 | x | nested_scopes.rb:16:26:16:26 | x | +| nested_scopes.rb:16:29:16:29 | a | nested_scopes.rb:16:29:16:29 | a | +| nested_scopes.rb:18:26:18:26 | x | nested_scopes.rb:18:26:18:26 | x | +| nested_scopes.rb:22:21:22:21 | a | nested_scopes.rb:22:21:22:21 | a | +| parameters.rb:1:14:1:14 | x | parameters.rb:1:14:1:14 | x | +| parameters.rb:1:18:1:18 | y | parameters.rb:1:18:1:18 | y | +| parameters.rb:7:17:7:22 | client | parameters.rb:7:17:7:22 | client | +| parameters.rb:7:25:7:31 | SplatParameter | parameters.rb:7:26:7:31 | pizzas | +| parameters.rb:15:15:15:19 | HashSplatParameter | parameters.rb:15:17:15:19 | map | +| parameters.rb:16:16:16:18 | key | parameters.rb:16:16:16:18 | key | +| parameters.rb:16:21:16:25 | value | parameters.rb:16:21:16:25 | value | +| parameters.rb:21:16:21:21 | BlockParameter | parameters.rb:21:17:21:21 | block | +| parameters.rb:25:15:25:30 | OptionalParameter | parameters.rb:25:15:25:18 | name | +| parameters.rb:25:33:25:50 | OptionalParameter | parameters.rb:25:33:25:36 | size | +| parameters.rb:30:15:30:20 | KeywordParameter | parameters.rb:30:15:30:19 | first | +| parameters.rb:30:24:30:33 | KeywordParameter | parameters.rb:30:24:30:29 | middle | +| parameters.rb:30:36:30:40 | KeywordParameter | parameters.rb:30:36:30:39 | last | +| parameters.rb:35:11:35:21 | OptionalParameter | parameters.rb:35:11:35:11 | a | +| parameters.rb:40:12:40:19 | KeywordParameter | parameters.rb:40:12:40:12 | d | +| parameters.rb:45:20:45:20 | _ | parameters.rb:45:20:45:20 | _ | +| parameters.rb:49:12:49:16 | DestructuredParameter | parameters.rb:49:13:49:13 | a | +| parameters.rb:49:12:49:16 | DestructuredParameter | parameters.rb:49:15:49:15 | b | +| parameters.rb:54:14:54:24 | OptionalParameter | parameters.rb:54:14:54:14 | y | +| scopes.rb:2:14:2:14 | x | scopes.rb:2:14:2:14 | x | +| scopes.rb:9:14:9:14 | x | scopes.rb:9:14:9:14 | x | +parameterNoAcess +| parameters.rb:45:22:45:22 | _ | diff --git a/ql/test/library-tests/variables/parameter.ql b/ql/test/library-tests/variables/parameter.ql new file mode 100644 index 00000000000..0c7eec56208 --- /dev/null +++ b/ql/test/library-tests/variables/parameter.ql @@ -0,0 +1,5 @@ +import codeql_ruby.Variables + +query predicate parameter(Parameter p, Variable v) { p.getAnAccess().getVariable() = v } + +query predicate parameterNoAcess(Parameter p) { not exists(p.getAnAccess()) } diff --git a/ql/test/library-tests/variables/varaccess.expected b/ql/test/library-tests/variables/varaccess.expected index 44ae78509d7..afb0bdab854 100644 --- a/ql/test/library-tests/variables/varaccess.expected +++ b/ql/test/library-tests/variables/varaccess.expected @@ -30,51 +30,51 @@ variableAccess | parameters.rb:3:9:3:9 | x | parameters.rb:1:14:1:14 | x | parameters.rb:1:9:5:3 | block scope | | parameters.rb:4:9:4:9 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | | parameters.rb:7:17:7:22 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:8:6:8:11 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:8:6:8:11 | pizzas | parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | | parameters.rb:9:25:9:30 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:11:14:11:19 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:11:14:11:19 | pizzas | parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | | parameters.rb:11:41:11:46 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:15:17:15:19 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | -| parameters.rb:16:3:16:5 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:15:17:15:19 | map | parameters.rb:15:17:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:16:3:16:5 | map | parameters.rb:15:17:15:19 | map | parameters.rb:15:1:19:3 | method scope | | parameters.rb:16:16:16:18 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | | parameters.rb:16:21:16:25 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | | parameters.rb:17:13:17:15 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | | parameters.rb:17:22:17:26 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | -| parameters.rb:21:17:21:21 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | -| parameters.rb:22:3:22:7 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | -| parameters.rb:25:15:25:18 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:25:33:25:36 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:25:40:25:43 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:26:8:26:11 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:27:8:27:11 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:30:15:30:19 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:30:24:30:29 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:30:36:30:39 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:31:11:31:15 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:31:20:31:25 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:31:30:31:33 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:21:17:21:21 | block | parameters.rb:21:17:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:22:3:22:7 | block | parameters.rb:21:17:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:25:15:25:18 | name | parameters.rb:25:15:25:18 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:33:25:36 | size | parameters.rb:25:33:25:36 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:40:25:43 | name | parameters.rb:25:15:25:18 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:26:8:26:11 | name | parameters.rb:25:15:25:18 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:27:8:27:11 | size | parameters.rb:25:33:25:36 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:30:15:30:19 | first | parameters.rb:30:15:30:19 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:24:30:29 | middle | parameters.rb:30:24:30:29 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:36:30:39 | last | parameters.rb:30:36:30:39 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:11:31:15 | first | parameters.rb:30:15:30:19 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:20:31:25 | middle | parameters.rb:30:24:30:29 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:30:31:33 | last | parameters.rb:30:36:30:39 | last | parameters.rb:30:1:32:3 | method scope | | parameters.rb:34:1:34:1 | b | parameters.rb:34:1:34:1 | b | parameters.rb:1:1:58:1 | top-level scope | -| parameters.rb:35:11:35:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:35:11:35:11 | a | parameters.rb:35:11:35:11 | a | parameters.rb:35:1:38:3 | method scope | | parameters.rb:35:16:35:16 | b | parameters.rb:35:16:35:16 | b | parameters.rb:35:1:38:3 | method scope | -| parameters.rb:37:11:37:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:37:11:37:11 | a | parameters.rb:35:11:35:11 | a | parameters.rb:35:1:38:3 | method scope | | parameters.rb:37:16:37:16 | b | parameters.rb:35:16:35:16 | b | parameters.rb:35:1:38:3 | method scope | -| parameters.rb:40:12:40:12 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:40:12:40:12 | d | parameters.rb:40:12:40:12 | d | parameters.rb:40:1:43:3 | method scope | | parameters.rb:40:15:40:15 | e | parameters.rb:40:15:40:15 | e | parameters.rb:40:1:43:3 | method scope | -| parameters.rb:42:11:42:11 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:42:11:42:11 | d | parameters.rb:40:12:40:12 | d | parameters.rb:40:1:43:3 | method scope | | parameters.rb:42:16:42:16 | e | parameters.rb:40:15:40:15 | e | parameters.rb:40:1:43:3 | method scope | | parameters.rb:45:20:45:20 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | | parameters.rb:45:22:45:22 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | | parameters.rb:46:8:46:8 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | -| parameters.rb:49:13:49:13 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | -| parameters.rb:49:15:49:15 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | -| parameters.rb:50:11:50:11 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | -| parameters.rb:50:16:50:16 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:49:13:49:13 | a | parameters.rb:49:13:49:13 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:49:15:49:15 | b | parameters.rb:49:15:49:15 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:11:50:11 | a | parameters.rb:49:13:49:13 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:16:50:16 | b | parameters.rb:49:15:49:15 | b | parameters.rb:49:1:51:3 | method scope | | parameters.rb:53:1:53:1 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | -| parameters.rb:54:14:54:14 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | +| parameters.rb:54:14:54:14 | y | parameters.rb:54:14:54:14 | y | parameters.rb:54:9:57:3 | block scope | | parameters.rb:54:19:54:19 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | | parameters.rb:55:9:55:9 | x | parameters.rb:53:1:53:1 | x | parameters.rb:1:1:58:1 | top-level scope | -| parameters.rb:56:9:56:9 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | +| parameters.rb:56:9:56:9 | y | parameters.rb:54:14:54:14 | y | parameters.rb:54:9:57:3 | block scope | | scopes.rb:2:14:2:14 | x | scopes.rb:2:14:2:14 | x | scopes.rb:2:9:6:3 | block scope | | scopes.rb:4:4:4:4 | a | scopes.rb:4:4:4:4 | a | scopes.rb:2:9:6:3 | block scope | | scopes.rb:5:9:5:9 | a | scopes.rb:4:4:4:4 | a | scopes.rb:2:9:6:3 | block scope | @@ -101,43 +101,43 @@ parameterAccess | parameters.rb:3:9:3:9 | x | parameters.rb:1:14:1:14 | x | parameters.rb:1:9:5:3 | block scope | | parameters.rb:4:9:4:9 | y | parameters.rb:1:18:1:18 | y | parameters.rb:1:9:5:3 | block scope | | parameters.rb:7:17:7:22 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:8:6:8:11 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:8:6:8:11 | pizzas | parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | | parameters.rb:9:25:9:30 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:11:14:11:19 | pizzas | parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | +| parameters.rb:11:14:11:19 | pizzas | parameters.rb:7:26:7:31 | pizzas | parameters.rb:7:1:13:3 | method scope | | parameters.rb:11:41:11:46 | client | parameters.rb:7:17:7:22 | client | parameters.rb:7:1:13:3 | method scope | -| parameters.rb:15:17:15:19 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | -| parameters.rb:16:3:16:5 | map | parameters.rb:15:15:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:15:17:15:19 | map | parameters.rb:15:17:15:19 | map | parameters.rb:15:1:19:3 | method scope | +| parameters.rb:16:3:16:5 | map | parameters.rb:15:17:15:19 | map | parameters.rb:15:1:19:3 | method scope | | parameters.rb:16:16:16:18 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | | parameters.rb:16:21:16:25 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | | parameters.rb:17:13:17:15 | key | parameters.rb:16:16:16:18 | key | parameters.rb:16:12:18:5 | block scope | | parameters.rb:17:22:17:26 | value | parameters.rb:16:21:16:25 | value | parameters.rb:16:12:18:5 | block scope | -| parameters.rb:21:17:21:21 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | -| parameters.rb:22:3:22:7 | block | parameters.rb:21:16:21:21 | block | parameters.rb:21:1:23:3 | method scope | -| parameters.rb:25:15:25:18 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:25:33:25:36 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:25:40:25:43 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:26:8:26:11 | name | parameters.rb:25:15:25:30 | name | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:27:8:27:11 | size | parameters.rb:25:33:25:50 | size | parameters.rb:25:1:28:3 | method scope | -| parameters.rb:30:15:30:19 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:30:24:30:29 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:30:36:30:39 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:31:11:31:15 | first | parameters.rb:30:15:30:20 | first | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:31:20:31:25 | middle | parameters.rb:30:24:30:33 | middle | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:31:30:31:33 | last | parameters.rb:30:36:30:40 | last | parameters.rb:30:1:32:3 | method scope | -| parameters.rb:35:11:35:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | -| parameters.rb:37:11:37:11 | a | parameters.rb:35:11:35:21 | a | parameters.rb:35:1:38:3 | method scope | -| parameters.rb:40:12:40:12 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | -| parameters.rb:42:11:42:11 | d | parameters.rb:40:12:40:19 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:21:17:21:21 | block | parameters.rb:21:17:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:22:3:22:7 | block | parameters.rb:21:17:21:21 | block | parameters.rb:21:1:23:3 | method scope | +| parameters.rb:25:15:25:18 | name | parameters.rb:25:15:25:18 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:33:25:36 | size | parameters.rb:25:33:25:36 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:25:40:25:43 | name | parameters.rb:25:15:25:18 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:26:8:26:11 | name | parameters.rb:25:15:25:18 | name | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:27:8:27:11 | size | parameters.rb:25:33:25:36 | size | parameters.rb:25:1:28:3 | method scope | +| parameters.rb:30:15:30:19 | first | parameters.rb:30:15:30:19 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:24:30:29 | middle | parameters.rb:30:24:30:29 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:30:36:30:39 | last | parameters.rb:30:36:30:39 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:11:31:15 | first | parameters.rb:30:15:30:19 | first | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:20:31:25 | middle | parameters.rb:30:24:30:29 | middle | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:31:30:31:33 | last | parameters.rb:30:36:30:39 | last | parameters.rb:30:1:32:3 | method scope | +| parameters.rb:35:11:35:11 | a | parameters.rb:35:11:35:11 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:37:11:37:11 | a | parameters.rb:35:11:35:11 | a | parameters.rb:35:1:38:3 | method scope | +| parameters.rb:40:12:40:12 | d | parameters.rb:40:12:40:12 | d | parameters.rb:40:1:43:3 | method scope | +| parameters.rb:42:11:42:11 | d | parameters.rb:40:12:40:12 | d | parameters.rb:40:1:43:3 | method scope | | parameters.rb:45:20:45:20 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | | parameters.rb:45:22:45:22 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | | parameters.rb:46:8:46:8 | _ | parameters.rb:45:20:45:20 | _ | parameters.rb:45:1:47:3 | method scope | -| parameters.rb:49:13:49:13 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | -| parameters.rb:49:15:49:15 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | -| parameters.rb:50:11:50:11 | a | parameters.rb:49:12:49:16 | a | parameters.rb:49:1:51:3 | method scope | -| parameters.rb:50:16:50:16 | b | parameters.rb:49:12:49:16 | b | parameters.rb:49:1:51:3 | method scope | -| parameters.rb:54:14:54:14 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | -| parameters.rb:56:9:56:9 | y | parameters.rb:54:14:54:24 | y | parameters.rb:54:9:57:3 | block scope | +| parameters.rb:49:13:49:13 | a | parameters.rb:49:13:49:13 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:49:15:49:15 | b | parameters.rb:49:15:49:15 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:11:50:11 | a | parameters.rb:49:13:49:13 | a | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:50:16:50:16 | b | parameters.rb:49:15:49:15 | b | parameters.rb:49:1:51:3 | method scope | +| parameters.rb:54:14:54:14 | y | parameters.rb:54:14:54:14 | y | parameters.rb:54:9:57:3 | block scope | +| parameters.rb:56:9:56:9 | y | parameters.rb:54:14:54:14 | y | parameters.rb:54:9:57:3 | block scope | | scopes.rb:2:14:2:14 | x | scopes.rb:2:14:2:14 | x | scopes.rb:2:9:6:3 | block scope | | scopes.rb:9:14:9:14 | x | scopes.rb:9:14:9:14 | x | scopes.rb:9:9:13:3 | block scope | localVariableAccess diff --git a/ql/test/library-tests/variables/varaccess.ql b/ql/test/library-tests/variables/varaccess.ql index 911b81f147e..6d428bdf4e6 100644 --- a/ql/test/library-tests/variables/varaccess.ql +++ b/ql/test/library-tests/variables/varaccess.ql @@ -5,14 +5,15 @@ query predicate variableAccess(VariableAccess access, Variable variable, Variabl scope = variable.getDeclaringScope() } -query predicate parameterAccess(ParameterAccess access, Parameter variable, VariableScope scope) { +query predicate parameterAccess(ParameterAccess access, LocalVariable variable, VariableScope scope) { variable = access.getVariable() and scope = variable.getDeclaringScope() } query predicate localVariableAccess( - LocalVariableAccess access, LocalVariable variable, VariableScope scope + VariableAccess access, LocalVariable variable, VariableScope scope ) { + not access instanceof ParameterAccess and variable = access.getVariable() and scope = variable.getDeclaringScope() } diff --git a/ql/test/library-tests/variables/variable.expected b/ql/test/library-tests/variables/variable.expected index 4010f77dd66..a9b283555f7 100644 --- a/ql/test/library-tests/variables/variable.expected +++ b/ql/test/library-tests/variables/variable.expected @@ -14,26 +14,26 @@ variable | parameters.rb:1:14:1:14 | x | | parameters.rb:1:18:1:18 | y | | parameters.rb:7:17:7:22 | client | -| parameters.rb:7:25:7:31 | pizzas | -| parameters.rb:15:15:15:19 | map | +| parameters.rb:7:26:7:31 | pizzas | +| parameters.rb:15:17:15:19 | map | | parameters.rb:16:16:16:18 | key | | parameters.rb:16:21:16:25 | value | -| parameters.rb:21:16:21:21 | block | -| parameters.rb:25:15:25:30 | name | -| parameters.rb:25:33:25:50 | size | -| parameters.rb:30:15:30:20 | first | -| parameters.rb:30:24:30:33 | middle | -| parameters.rb:30:36:30:40 | last | +| parameters.rb:21:17:21:21 | block | +| parameters.rb:25:15:25:18 | name | +| parameters.rb:25:33:25:36 | size | +| parameters.rb:30:15:30:19 | first | +| parameters.rb:30:24:30:29 | middle | +| parameters.rb:30:36:30:39 | last | | parameters.rb:34:1:34:1 | b | -| parameters.rb:35:11:35:21 | a | +| parameters.rb:35:11:35:11 | a | | parameters.rb:35:16:35:16 | b | -| parameters.rb:40:12:40:19 | d | +| parameters.rb:40:12:40:12 | d | | parameters.rb:40:15:40:15 | e | | parameters.rb:45:20:45:20 | _ | -| parameters.rb:49:12:49:16 | a | -| parameters.rb:49:12:49:16 | b | +| parameters.rb:49:13:49:13 | a | +| parameters.rb:49:15:49:15 | b | | parameters.rb:53:1:53:1 | x | -| parameters.rb:54:14:54:24 | y | +| parameters.rb:54:14:54:14 | y | | scopes.rb:2:14:2:14 | x | | scopes.rb:4:4:4:4 | a | | scopes.rb:7:1:7:1 | a | @@ -47,22 +47,22 @@ parameter | parameters.rb:1:14:1:14 | x | | parameters.rb:1:18:1:18 | y | | parameters.rb:7:17:7:22 | client | -| parameters.rb:7:25:7:31 | pizzas | -| parameters.rb:15:15:15:19 | map | +| parameters.rb:7:26:7:31 | pizzas | +| parameters.rb:15:17:15:19 | map | | parameters.rb:16:16:16:18 | key | | parameters.rb:16:21:16:25 | value | -| parameters.rb:21:16:21:21 | block | -| parameters.rb:25:15:25:30 | name | -| parameters.rb:25:33:25:50 | size | -| parameters.rb:30:15:30:20 | first | -| parameters.rb:30:24:30:33 | middle | -| parameters.rb:30:36:30:40 | last | -| parameters.rb:35:11:35:21 | a | -| parameters.rb:40:12:40:19 | d | +| parameters.rb:21:17:21:21 | block | +| parameters.rb:25:15:25:18 | name | +| parameters.rb:25:33:25:36 | size | +| parameters.rb:30:15:30:19 | first | +| parameters.rb:30:24:30:29 | middle | +| parameters.rb:30:36:30:39 | last | +| parameters.rb:35:11:35:11 | a | +| parameters.rb:40:12:40:12 | d | | parameters.rb:45:20:45:20 | _ | -| parameters.rb:49:12:49:16 | a | -| parameters.rb:49:12:49:16 | b | -| parameters.rb:54:14:54:24 | y | +| parameters.rb:49:13:49:13 | a | +| parameters.rb:49:15:49:15 | b | +| parameters.rb:54:14:54:14 | y | | scopes.rb:2:14:2:14 | x | | scopes.rb:9:14:9:14 | x | localVariable diff --git a/ql/test/library-tests/variables/variable.ql b/ql/test/library-tests/variables/variable.ql index 54300c416b4..13f7a0f0efa 100644 --- a/ql/test/library-tests/variables/variable.ql +++ b/ql/test/library-tests/variables/variable.ql @@ -2,6 +2,6 @@ import codeql_ruby.Variables query predicate variable(Variable v) { any() } -query predicate parameter(Parameter p) { any() } +query predicate parameter(LocalVariable v) { v.getAnAccess() instanceof ParameterAccess } -query predicate localVariable(LocalVariable v) { any() } +query predicate localVariable(LocalVariable v) { not v.getAnAccess() instanceof ParameterAccess } From 64ebf5b90962d5b717d16e6aba70de80fceb97d2 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Wed, 25 Nov 2020 12:55:53 +0100 Subject: [PATCH 4/4] Address comments --- ql/src/codeql_ruby/Variables.qll | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ql/src/codeql_ruby/Variables.qll b/ql/src/codeql_ruby/Variables.qll index d7171c78b53..ee1d6aa9751 100644 --- a/ql/src/codeql_ruby/Variables.qll +++ b/ql/src/codeql_ruby/Variables.qll @@ -15,8 +15,8 @@ private VariableScope enclosingScope(AstNode node) { /** A parameter. */ class Parameter extends AstNode { - int position; - VariableScope scope; + private int position; + private VariableScope scope; Parameter() { this = @@ -26,10 +26,13 @@ class Parameter extends AstNode { scope.(MethodScope).getScopeElement().getAFieldOrChild().(MethodParameters).getChild(position) } + /** Gets the (zero-based) position of this parameter. */ final int getPosition() { result = position } + /** Gets the scope this parameter is declared in. */ final VariableScope getDeclaringScope() { result = scope } + /** Gets an access to this parameter. */ final ParameterAccess getAnAccess() { result.getParameter() = this } }