From 7ac46bf8f882fba8d8f3bae07bdd1485fc4f1ed1 Mon Sep 17 00:00:00 2001 From: Nick Rolfe Date: Tue, 26 Jan 2021 18:03:13 +0000 Subject: [PATCH 1/5] Add `SuperCall` class for calls to `super` --- ql/src/codeql_ruby/ast/Call.qll | 16 ++++++++ ql/src/codeql_ruby/ast/internal/Call.qll | 41 +++++++++++++++++++ ql/src/codeql_ruby/ast/internal/Variable.qll | 9 +++- .../library-tests/ast/calls/calls.expected | 37 +++++++++++++++++ ql/test/library-tests/ast/calls/calls.ql | 6 +++ ql/test/library-tests/ast/calls/calls.rb | 18 +++++++- .../controlflow/graph/Cfg.expected | 4 +- 7 files changed, 126 insertions(+), 5 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Call.qll b/ql/src/codeql_ruby/ast/Call.qll index 20f30c0e299..4f9248a0ecb 100644 --- a/ql/src/codeql_ruby/ast/Call.qll +++ b/ql/src/codeql_ruby/ast/Call.qll @@ -108,6 +108,22 @@ class YieldCall extends Call, @yield { final override string getAPrimaryQlClass() { result = "YieldCall" } } +/** + * A call to `super`. + * ```rb + * class Foo < Bar + * def baz + * super + * end + * end + * ``` + */ +class SuperCall extends Call { + final override SuperCall::Range range; + + final override string getAPrimaryQlClass() { result = "SuperCall" } +} + /** * A block argument in a method call. * ```rb diff --git a/ql/src/codeql_ruby/ast/internal/Call.qll b/ql/src/codeql_ruby/ast/internal/Call.qll index d15f6dbe83c..b6d47ac9be8 100644 --- a/ql/src/codeql_ruby/ast/internal/Call.qll +++ b/ql/src/codeql_ruby/ast/internal/Call.qll @@ -66,6 +66,47 @@ module YieldCall { } } +module SuperCall { + abstract class Range extends Call::Range { } + + private class SuperTokenCallRange extends SuperCall::Range, @token_super { + final override Generated::Super generated; + + SuperTokenCallRange() { vcall(this) and not access(this, _) } + + final override Expr getReceiver() { none() } + + final override string getMethodName() { result = generated.getValue() } + + final override ScopeResolution getMethodScopeResolution() { none() } + + final override Expr getArgument(int n) { none() } + + final override Block getBlock() { none() } + } + + private class RegularSuperCallRange extends SuperCall::Range, @call { + final override Generated::Call generated; + + RegularSuperCallRange() { + generated = this and + generated.getMethod() instanceof Generated::Super + } + + final override Expr getReceiver() { result = generated.getReceiver() } + + final override string getMethodName() { + result = generated.getMethod().(Generated::Super).getValue() + } + + final override ScopeResolution getMethodScopeResolution() { none() } + + final override Expr getArgument(int n) { result = generated.getArguments().getChild(n) } + + final override Block getBlock() { result = generated.getBlock() } + } +} + module BlockArgument { class Range extends Expr::Range, @block_argument { final override Generated::BlockArgument generated; diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll index 15f6086fb08..68f85b890c0 100644 --- a/ql/src/codeql_ruby/ast/internal/Variable.qll +++ b/ql/src/codeql_ruby/ast/internal/Variable.qll @@ -125,6 +125,11 @@ private module Cached { not scope.(CapturingScope).inherits(name, _) } + // Token types that can be accesses/vcalls + private class AccessTokenUnion = @token_identifier or @token_super; + + private class AccessToken extends Generated::Token, AccessTokenUnion { } + /** * Holds if `i` is an `identifier` node occurring in the context where it * should be considered a VCALL. VCALL is the term that MRI/Ripper uses @@ -139,7 +144,7 @@ private module Cached { * ``` */ cached - predicate vcall(Generated::Identifier i) { + predicate vcall(AccessToken i) { i = any(Generated::ArgumentList x).getChild(_) or i = any(Generated::Array x).getChild(_) @@ -266,7 +271,7 @@ private module Cached { } cached - predicate access(Generated::Identifier access, Variable variable) { + predicate access(AccessToken access, Variable variable) { exists(string name | name = access.getValue() | variable = enclosingScope(access).getVariable(name) and not strictlyBefore(access.getLocation(), variable.getLocation()) and diff --git a/ql/test/library-tests/ast/calls/calls.expected b/ql/test/library-tests/ast/calls/calls.expected index 70b0832b26c..68a0637c206 100644 --- a/ql/test/library-tests/ast/calls/calls.expected +++ b/ql/test/library-tests/ast/calls/calls.expected @@ -72,6 +72,8 @@ callsWithNoReceiverArgumentsOrBlock | calls.rb:204:6:204:8 | call to bar | bar | | calls.rb:207:7:207:9 | call to bar | bar | | calls.rb:210:11:210:13 | call to bar | bar | +| calls.rb:217:5:217:9 | call to super | super | +| calls.rb:218:5:218:11 | call to super | super | callsWithScopeResolutionName | calls.rb:5:1:5:10 | call to bar | calls.rb:5:1:5:8 | ...::bar | callsWithArguments @@ -86,6 +88,14 @@ callsWithArguments | calls.rb:204:1:204:9 | call to foo | foo | 0 | calls.rb:204:5:204:8 | *... | | calls.rb:207:1:207:10 | call to foo | foo | 0 | calls.rb:207:5:207:9 | **... | | calls.rb:210:1:210:14 | call to foo | foo | 0 | calls.rb:210:5:210:13 | Pair | +| calls.rb:219:5:219:16 | call to super | super | 0 | calls.rb:219:11:219:16 | blah | +| calls.rb:220:5:220:17 | call to super | super | 0 | calls.rb:220:11:220:11 | 1 | +| calls.rb:220:5:220:17 | call to super | super | 1 | calls.rb:220:14:220:14 | 2 | +| calls.rb:220:5:220:17 | call to super | super | 2 | calls.rb:220:17:220:17 | 3 | +| calls.rb:223:5:223:30 | call to super | super | 0 | calls.rb:223:11:223:11 | 4 | +| calls.rb:223:5:223:30 | call to super | super | 1 | calls.rb:223:14:223:14 | 5 | +| calls.rb:224:5:224:33 | call to super | super | 0 | calls.rb:224:11:224:11 | 6 | +| calls.rb:224:5:224:33 | call to super | super | 1 | calls.rb:224:14:224:14 | 7 | callsWithReceiver | calls.rb:8:1:8:7 | call to bar | calls.rb:8:1:8:3 | 123 | | calls.rb:22:1:24:3 | call to bar | calls.rb:22:1:22:3 | 123 | @@ -96,6 +106,33 @@ callsWithBlock | calls.rb:22:1:24:3 | call to bar | calls.rb:22:16:24:3 | do ... end | | calls.rb:80:1:80:13 | call to foo | calls.rb:80:7:80:13 | { ... } | | calls.rb:83:1:83:16 | call to foo | calls.rb:83:7:83:16 | do ... end | +| calls.rb:221:5:221:23 | call to super | calls.rb:221:11:221:23 | { ... } | +| calls.rb:222:5:222:26 | call to super | calls.rb:222:11:222:26 | do ... end | +| calls.rb:223:5:223:30 | call to super | calls.rb:223:16:223:30 | { ... } | +| calls.rb:224:5:224:33 | call to super | calls.rb:224:16:224:33 | do ... end | yieldCalls | calls.rb:28:3:28:7 | call to yield | | calls.rb:33:3:33:16 | call to yield | +superCalls +| calls.rb:217:5:217:9 | call to super | +| calls.rb:218:5:218:11 | call to super | +| calls.rb:219:5:219:16 | call to super | +| calls.rb:220:5:220:17 | call to super | +| calls.rb:221:5:221:23 | call to super | +| calls.rb:222:5:222:26 | call to super | +| calls.rb:223:5:223:30 | call to super | +| calls.rb:224:5:224:33 | call to super | +superCallsWithArguments +| calls.rb:219:5:219:16 | call to super | 0 | calls.rb:219:11:219:16 | blah | +| calls.rb:220:5:220:17 | call to super | 0 | calls.rb:220:11:220:11 | 1 | +| calls.rb:220:5:220:17 | call to super | 1 | calls.rb:220:14:220:14 | 2 | +| calls.rb:220:5:220:17 | call to super | 2 | calls.rb:220:17:220:17 | 3 | +| calls.rb:223:5:223:30 | call to super | 0 | calls.rb:223:11:223:11 | 4 | +| calls.rb:223:5:223:30 | call to super | 1 | calls.rb:223:14:223:14 | 5 | +| calls.rb:224:5:224:33 | call to super | 0 | calls.rb:224:11:224:11 | 6 | +| calls.rb:224:5:224:33 | call to super | 1 | calls.rb:224:14:224:14 | 7 | +superCallsWithBlock +| calls.rb:221:5:221:23 | call to super | calls.rb:221:11:221:23 | { ... } | +| calls.rb:222:5:222:26 | call to super | calls.rb:222:11:222:26 | do ... end | +| calls.rb:223:5:223:30 | call to super | calls.rb:223:16:223:30 | { ... } | +| calls.rb:224:5:224:33 | call to super | calls.rb:224:16:224:33 | do ... end | diff --git a/ql/test/library-tests/ast/calls/calls.ql b/ql/test/library-tests/ast/calls/calls.ql index 7e29d5f7dd6..bc6c244356e 100644 --- a/ql/test/library-tests/ast/calls/calls.ql +++ b/ql/test/library-tests/ast/calls/calls.ql @@ -22,3 +22,9 @@ query predicate callsWithReceiver(Call c, Expr rcv) { rcv = c.getReceiver() } query predicate callsWithBlock(Call c, Block b) { b = c.getBlock() } query predicate yieldCalls(YieldCall c) { any() } + +query predicate superCalls(SuperCall c) { any() } + +query predicate superCallsWithArguments(SuperCall c, int n, Expr argN) { argN = c.getArgument(n) } + +query predicate superCallsWithBlock(SuperCall c, Block b) { b = c.getBlock() } diff --git a/ql/test/library-tests/ast/calls/calls.rb b/ql/test/library-tests/ast/calls/calls.rb index 7133ee98ff4..e7c1e426435 100644 --- a/ql/test/library-tests/ast/calls/calls.rb +++ b/ql/test/library-tests/ast/calls/calls.rb @@ -207,4 +207,20 @@ foo(*bar) foo(**bar) # the value in a keyword argument -foo(blah: bar) \ No newline at end of file +foo(blah: bar) + +# ------------------------------------------------------------------------------ +# calls to `super` + +class MyClass + def my_method + super + super() + super 'blah' + super 1, 2, 3 + super { |x| x + 1 } + super do |x| x * 2 end + super 4, 5 { |x| x + 100 } + super 6, 7 do |x| x + 200 end + end +end \ No newline at end of file diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected index d3a1e0e8a41..630d18bcf13 100644 --- a/ql/test/library-tests/controlflow/graph/Cfg.expected +++ b/ql/test/library-tests/controlflow/graph/Cfg.expected @@ -1631,12 +1631,12 @@ cfg.rb: #-----| -> exit print (normal) # 144| puts -#-----| -> super +#-----| -> call to super # 144| call to print #-----| -> call to puts -# 144| super +# 144| call to super #-----| -> print # 144| print From 70bbeaac3ba4a0f885d935e24b3b5c32108675a3 Mon Sep 17 00:00:00 2001 From: Nick Rolfe Date: Wed, 27 Jan 2021 18:28:01 +0000 Subject: [PATCH 2/5] Simplify, since `super` tokens are never variable accesses --- ql/src/codeql_ruby/ast/internal/Call.qll | 4 +++- ql/src/codeql_ruby/ast/internal/Variable.qll | 10 ++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ql/src/codeql_ruby/ast/internal/Call.qll b/ql/src/codeql_ruby/ast/internal/Call.qll index b6d47ac9be8..d818d44f0b2 100644 --- a/ql/src/codeql_ruby/ast/internal/Call.qll +++ b/ql/src/codeql_ruby/ast/internal/Call.qll @@ -72,7 +72,9 @@ module SuperCall { private class SuperTokenCallRange extends SuperCall::Range, @token_super { final override Generated::Super generated; - SuperTokenCallRange() { vcall(this) and not access(this, _) } + // N.B. `super` tokens can never be accesses, so any vcall with `super` must + // be a call. + SuperTokenCallRange() { vcall(this) } final override Expr getReceiver() { none() } diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll index 68f85b890c0..914291dee64 100644 --- a/ql/src/codeql_ruby/ast/internal/Variable.qll +++ b/ql/src/codeql_ruby/ast/internal/Variable.qll @@ -125,10 +125,8 @@ private module Cached { not scope.(CapturingScope).inherits(name, _) } - // Token types that can be accesses/vcalls - private class AccessTokenUnion = @token_identifier or @token_super; - - private class AccessToken extends Generated::Token, AccessTokenUnion { } + // Token types that can be vcalls + private class VcallToken = @token_identifier or @token_super; /** * Holds if `i` is an `identifier` node occurring in the context where it @@ -144,7 +142,7 @@ private module Cached { * ``` */ cached - predicate vcall(AccessToken i) { + predicate vcall(VcallToken i) { i = any(Generated::ArgumentList x).getChild(_) or i = any(Generated::Array x).getChild(_) @@ -271,7 +269,7 @@ private module Cached { } cached - predicate access(AccessToken access, Variable variable) { + predicate access(Generated::Identifier access, Variable variable) { exists(string name | name = access.getValue() | variable = enclosingScope(access).getVariable(name) and not strictlyBefore(access.getLocation(), variable.getLocation()) and From 743e627a8df3cc0b86e3cf3105833cd7a31913b7 Mon Sep 17 00:00:00 2001 From: Nick Rolfe Date: Wed, 27 Jan 2021 18:45:08 +0000 Subject: [PATCH 3/5] Test calls to methods named 'super' --- ql/test/library-tests/ast/calls/calls.expected | 6 ++++++ ql/test/library-tests/ast/calls/calls.rb | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/ql/test/library-tests/ast/calls/calls.expected b/ql/test/library-tests/ast/calls/calls.expected index 68a0637c206..72591f575a8 100644 --- a/ql/test/library-tests/ast/calls/calls.expected +++ b/ql/test/library-tests/ast/calls/calls.expected @@ -74,6 +74,9 @@ callsWithNoReceiverArgumentsOrBlock | calls.rb:210:11:210:13 | call to bar | bar | | calls.rb:217:5:217:9 | call to super | super | | calls.rb:218:5:218:11 | call to super | super | +| calls.rb:234:5:234:7 | call to foo | foo | +| calls.rb:235:5:235:14 | call to super | super | +| calls.rb:236:5:236:9 | call to super | super | callsWithScopeResolutionName | calls.rb:5:1:5:10 | call to bar | calls.rb:5:1:5:8 | ...::bar | callsWithArguments @@ -100,6 +103,8 @@ callsWithReceiver | calls.rb:8:1:8:7 | call to bar | calls.rb:8:1:8:3 | 123 | | calls.rb:22:1:24:3 | call to bar | calls.rb:22:1:22:3 | 123 | | calls.rb:86:1:86:9 | call to bar | calls.rb:86:1:86:3 | call to foo | +| calls.rb:234:5:234:13 | call to super | calls.rb:234:5:234:7 | call to foo | +| calls.rb:236:5:236:15 | call to super | calls.rb:236:5:236:9 | call to super | callsWithBlock | calls.rb:14:1:14:17 | call to foo | calls.rb:14:5:14:17 | { ... } | | calls.rb:17:1:19:3 | call to foo | calls.rb:17:5:19:3 | do ... end | @@ -122,6 +127,7 @@ superCalls | calls.rb:222:5:222:26 | call to super | | calls.rb:223:5:223:30 | call to super | | calls.rb:224:5:224:33 | call to super | +| calls.rb:236:5:236:9 | call to super | superCallsWithArguments | calls.rb:219:5:219:16 | call to super | 0 | calls.rb:219:11:219:16 | blah | | calls.rb:220:5:220:17 | call to super | 0 | calls.rb:220:11:220:11 | 1 | diff --git a/ql/test/library-tests/ast/calls/calls.rb b/ql/test/library-tests/ast/calls/calls.rb index e7c1e426435..c60cf682001 100644 --- a/ql/test/library-tests/ast/calls/calls.rb +++ b/ql/test/library-tests/ast/calls/calls.rb @@ -223,4 +223,16 @@ class MyClass super 4, 5 { |x| x + 100 } super 6, 7 do |x| x + 200 end end +end + +# ------------------------------------------------------------------------------ +# calls to methods simply named `super`, i.e. *not* calls to the same method in +# a parent classs, so these should be Call but not SuperCall + +class AnotherClass + def another_method + foo.super + self.super # TODO: this shows up as a call without a receiver, but that should be fixed once we handle `self` expressions + super.super # we expect the receiver to be a SuperCall, while the outer call should not (it's just a regular Call) + end end \ No newline at end of file From 640092352b0157794dcc8ae429f2674ff6c8718e Mon Sep 17 00:00:00 2001 From: Nick Rolfe Date: Wed, 27 Jan 2021 18:49:37 +0000 Subject: [PATCH 4/5] RegularSuperCallRange::getReceiver() never holds --- ql/src/codeql_ruby/ast/internal/Call.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/codeql_ruby/ast/internal/Call.qll b/ql/src/codeql_ruby/ast/internal/Call.qll index d818d44f0b2..cecf5ff0c34 100644 --- a/ql/src/codeql_ruby/ast/internal/Call.qll +++ b/ql/src/codeql_ruby/ast/internal/Call.qll @@ -95,7 +95,7 @@ module SuperCall { generated.getMethod() instanceof Generated::Super } - final override Expr getReceiver() { result = generated.getReceiver() } + final override Expr getReceiver() { none() } final override string getMethodName() { result = generated.getMethod().(Generated::Super).getValue() From 30804f74e21e0a074b8f245fa7869615d59ae536 Mon Sep 17 00:00:00 2001 From: Nick Rolfe Date: Thu, 28 Jan 2021 17:48:16 +0000 Subject: [PATCH 5/5] Remove redundant `instanceof` expression --- ql/src/codeql_ruby/ast/internal/Call.qll | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ql/src/codeql_ruby/ast/internal/Call.qll b/ql/src/codeql_ruby/ast/internal/Call.qll index cecf5ff0c34..407e6e938c2 100644 --- a/ql/src/codeql_ruby/ast/internal/Call.qll +++ b/ql/src/codeql_ruby/ast/internal/Call.qll @@ -90,10 +90,7 @@ module SuperCall { private class RegularSuperCallRange extends SuperCall::Range, @call { final override Generated::Call generated; - RegularSuperCallRange() { - generated = this and - generated.getMethod() instanceof Generated::Super - } + RegularSuperCallRange() { generated.getMethod() instanceof Generated::Super } final override Expr getReceiver() { none() }