From 1ada9feda7f1291182bfebaaf1561a0b469f267e Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Fri, 18 Dec 2020 13:00:28 +0100 Subject: [PATCH 1/4] Make VariableAccess "abstract" --- ql/src/codeql_ruby/ast/Variable.qll | 4 +--- ql/src/codeql_ruby/ast/internal/Variable.qll | 18 ++++++++---------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll index 73b4e2a0593..275a699db63 100644 --- a/ql/src/codeql_ruby/ast/Variable.qll +++ b/ql/src/codeql_ruby/ast/Variable.qll @@ -60,15 +60,13 @@ class LocalVariable extends Variable { } /** An access to a variable. */ -class VariableAccess extends Expr, @token_identifier { +class VariableAccess extends Expr { override VariableAccess::Range range; /** Gets the variable this identifier refers to. */ Variable getVariable() { result = range.getVariable() } final override string toString() { result = this.getVariable().getName() } - // TODO uncomment this and fix the params test - //override string getAPrimaryQlClass() { result = "VariableAccess" } } /** An access to a local variable. */ diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll index a1fd920231a..f827924d883 100644 --- a/ql/src/codeql_ruby/ast/internal/Variable.qll +++ b/ql/src/codeql_ruby/ast/internal/Variable.qll @@ -227,20 +227,18 @@ module LocalVariable { } module VariableAccess { - class Range extends Expr::Range, @token_identifier { - override Generated::Identifier generated; - Variable variable; - - Range() { access(this, variable) } - - Variable getVariable() { result = variable } + abstract class Range extends Expr::Range { + abstract Variable getVariable(); } } module LocalVariableAccess { - class Range extends VariableAccess::Range { - override LocalVariable variable; + class Range extends VariableAccess::Range, @token_identifier { + override Generated::Identifier generated; + LocalVariable variable; - override LocalVariable getVariable() { result = variable } + Range() { access(this, variable) } + + final override LocalVariable getVariable() { result = variable } } } From dc0de9132e501c0da09f5c6984c268c1a37b98d4 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Fri, 18 Dec 2020 13:40:53 +0100 Subject: [PATCH 2/4] Add GlobalVariable --- ql/src/codeql_ruby/ast/Variable.qll | 19 +++++++++- ql/src/codeql_ruby/ast/internal/Variable.qll | 37 +++++++++++++++++++ ql/test/library-tests/variables/scopes.rb | 8 +++- .../variables/varaccess.expected | 17 +++++---- .../library-tests/variables/variable.expected | 3 ++ .../variables/varscopes.expected | 3 +- 6 files changed, 77 insertions(+), 10 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll index 275a699db63..5ec6ac76bf1 100644 --- a/ql/src/codeql_ruby/ast/Variable.qll +++ b/ql/src/codeql_ruby/ast/Variable.qll @@ -59,6 +59,13 @@ class LocalVariable extends Variable { final override LocalVariableAccess getAnAccess() { result.getVariable() = this } } +/** A global variable. */ +class GlobalVariable extends Variable { + override GlobalVariable::Range range; + + final override GlobalVariableAccess getAnAccess() { result.getVariable() = this } +} + /** An access to a variable. */ class VariableAccess extends Expr { override VariableAccess::Range range; @@ -70,7 +77,7 @@ class VariableAccess extends Expr { } /** An access to a local variable. */ -class LocalVariableAccess extends VariableAccess { +class LocalVariableAccess extends VariableAccess, @token_identifier { final override LocalVariableAccess::Range range; /** Gets the variable this identifier refers to. */ @@ -78,3 +85,13 @@ class LocalVariableAccess extends VariableAccess { // TODO uncomment this and fix the params test //final override string getAPrimaryQlClass() { result = "LocalVariableAccess" } } + +/** An access to a local variable. */ +class GlobalVariableAccess extends VariableAccess, @token_global_variable { + final override GlobalVariableAccess::Range range; + + /** Gets the variable this identifier refers to. */ + final override GlobalVariable getVariable() { result = range.getVariable() } + + final override string getAPrimaryQlClass() { result = "GlobalVariableAccess" } +} diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll index f827924d883..d7119e5dd01 100644 --- a/ql/src/codeql_ruby/ast/internal/Variable.qll +++ b/ql/src/codeql_ruby/ast/internal/Variable.qll @@ -101,6 +101,7 @@ cached private module Cached { cached newtype TScope = + TGlobalScope() or TTopLevelScope(Generated::Program node) or TModuleScope(Generated::Module node) or TClassScope(AstNode cls) { @@ -110,6 +111,9 @@ private module Cached { cached newtype TVariable = + TGlobalVariable(TGlobalScope scope, string name) { + name = any(Generated::GlobalVariable var).getValue() + } or TLocalVariable(VariableScope scope, string name, Generated::Identifier i) { scopeDefinesParameterVariable(scope, name, i) or @@ -153,6 +157,14 @@ module VariableScope { } } +module GlobalScope { + class Range extends VariableScope::Range, TGlobalScope { + override string toString() { result = "global scope" } + + override AstNode getScopeElement() { none() } + } +} + module TopLevelScope { class Range extends VariableScope::Range, TTopLevelScope { override string toString() { result = "top-level scope" } @@ -226,6 +238,21 @@ module LocalVariable { } } +module GlobalVariable { + class Range extends Variable::Range { + private VariableScope scope; + private string name; + + Range() { this = TGlobalVariable(scope, name) } + + final override string getName() { result = name } + + final override Location getLocation() { none() } + + final override VariableScope getDeclaringScope() { result = scope } + } +} + module VariableAccess { abstract class Range extends Expr::Range { abstract Variable getVariable(); @@ -242,3 +269,13 @@ module LocalVariableAccess { final override LocalVariable getVariable() { result = variable } } } + +module GlobalVariableAccess { + class Range extends VariableAccess::Range, @token_global_variable { + GlobalVariable variable; + + Range() { this.(Generated::GlobalVariable).getValue() = variable.getName() } + + final override GlobalVariable getVariable() { result = variable } + } +} diff --git a/ql/test/library-tests/variables/scopes.rb b/ql/test/library-tests/variables/scopes.rb index 1219acef8cc..65816cc31b4 100644 --- a/ql/test/library-tests/variables/scopes.rb +++ b/ql/test/library-tests/variables/scopes.rb @@ -15,4 +15,10 @@ puts a puts b # new local variable puts c # new local variable puts d # new local variable -end \ No newline at end of file +end + +# new global variable +$global = 42 + +# use of a pre-defined global variable +script = $0 diff --git a/ql/test/library-tests/variables/varaccess.expected b/ql/test/library-tests/variables/varaccess.expected index 3c97574dacf..43683ab605d 100644 --- a/ql/test/library-tests/variables/varaccess.expected +++ b/ql/test/library-tests/variables/varaccess.expected @@ -76,17 +76,20 @@ | 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 | -| scopes.rb:7:1:7:1 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:18:3 | top-level scope | -| scopes.rb:8:6:8:6 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:18:3 | top-level scope | +| scopes.rb:7:1:7:1 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:24:12 | top-level scope | +| scopes.rb:8:6:8:6 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:24:12 | top-level scope | | scopes.rb:9:14:9:14 | x | scopes.rb:9:14:9:14 | x | scopes.rb:9:9:18:3 | block scope | -| scopes.rb:10:9:10:9 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:18:3 | top-level scope | -| scopes.rb:11:4:11:4 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:18:3 | top-level scope | -| scopes.rb:12:9:12:9 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:18:3 | top-level scope | -| scopes.rb:13:4:13:4 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:18:3 | top-level scope | +| scopes.rb:10:9:10:9 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:24:12 | top-level scope | +| scopes.rb:11:4:11:4 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:24:12 | top-level scope | +| scopes.rb:12:9:12:9 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:24:12 | top-level scope | +| scopes.rb:13:4:13:4 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:24:12 | top-level scope | | scopes.rb:13:7:13:7 | b | scopes.rb:13:7:13:7 | b | scopes.rb:9:9:18:3 | block scope | | scopes.rb:13:11:13:11 | c | scopes.rb:13:11:13:11 | c | scopes.rb:9:9:18:3 | block scope | | scopes.rb:13:14:13:14 | d | scopes.rb:13:14:13:14 | d | scopes.rb:9:9:18:3 | block scope | -| scopes.rb:14:9:14:9 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:18:3 | top-level scope | +| scopes.rb:14:9:14:9 | a | scopes.rb:7:1:7:1 | a | scopes.rb:1:1:24:12 | top-level scope | | scopes.rb:15:9:15:9 | b | scopes.rb:13:7:13:7 | b | scopes.rb:9:9:18:3 | block scope | | scopes.rb:16:9:16:9 | c | scopes.rb:13:11:13:11 | c | scopes.rb:9:9:18:3 | block scope | | scopes.rb:17:9:17:9 | d | scopes.rb:13:14:13:14 | d | scopes.rb:9:9:18:3 | block scope | +| scopes.rb:21:1:21:7 | $global | file://:0:0:0:0 | $global | file://:0:0:0:0 | global scope | +| scopes.rb:24:1:24:6 | script | scopes.rb:24:1:24:6 | script | scopes.rb:1:1:24:12 | top-level scope | +| scopes.rb:24:10:24:11 | $0 | file://:0:0:0:0 | $0 | file://:0:0:0:0 | global scope | diff --git a/ql/test/library-tests/variables/variable.expected b/ql/test/library-tests/variables/variable.expected index 143f5d2639a..749df84cb0d 100644 --- a/ql/test/library-tests/variables/variable.expected +++ b/ql/test/library-tests/variables/variable.expected @@ -1,3 +1,5 @@ +| file://:0:0:0:0 | $0 | +| file://:0:0:0:0 | $global | | nested_scopes.rb:5:3:5:3 | a | | nested_scopes.rb:7:5:7:5 | a | | nested_scopes.rb:9:7:9:7 | a | @@ -40,3 +42,4 @@ | scopes.rb:13:7:13:7 | b | | scopes.rb:13:11:13:11 | c | | scopes.rb:13:14:13:14 | d | +| scopes.rb:24:1:24:6 | script | diff --git a/ql/test/library-tests/variables/varscopes.expected b/ql/test/library-tests/variables/varscopes.expected index 70c2d9283cf..03b586db730 100644 --- a/ql/test/library-tests/variables/varscopes.expected +++ b/ql/test/library-tests/variables/varscopes.expected @@ -1,3 +1,4 @@ +| file://:0:0:0:0 | global scope | | nested_scopes.rb:1:1:3:3 | method scope | | nested_scopes.rb:1:1:42:1 | top-level scope | | nested_scopes.rb:4:1:39:3 | class scope | @@ -25,6 +26,6 @@ | 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:18:3 | top-level scope | +| scopes.rb:1:1:24:12 | top-level scope | | scopes.rb:2:9:6:3 | block scope | | scopes.rb:9:9:18:3 | block scope | From 8469bd3688855227c3c9ce6f20fd2b38969f020c Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Fri, 18 Dec 2020 15:02:02 +0100 Subject: [PATCH 3/4] Uncomment `getAPrimaryQlClass()` --- ql/src/codeql_ruby/ast/Variable.qll | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll index 5ec6ac76bf1..3ab213f54de 100644 --- a/ql/src/codeql_ruby/ast/Variable.qll +++ b/ql/src/codeql_ruby/ast/Variable.qll @@ -82,8 +82,10 @@ class LocalVariableAccess extends VariableAccess, @token_identifier { /** Gets the variable this identifier refers to. */ final override LocalVariable getVariable() { result = range.getVariable() } - // TODO uncomment this and fix the params test - //final override string getAPrimaryQlClass() { result = "LocalVariableAccess" } + + final override string getAPrimaryQlClass() { + not this instanceof SimpleParameter and result = "LocalVariableAccess" + } } /** An access to a local variable. */ From ad1782b62043f9a6702a255b5987747618fb8988 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Mon, 21 Dec 2020 10:52:42 +0100 Subject: [PATCH 4/4] Address comments --- ql/src/codeql_ruby/ast/Variable.qll | 4 ++-- ql/src/codeql_ruby/ast/internal/Variable.qll | 13 +++++-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll index 3ab213f54de..06dee5ef89b 100644 --- a/ql/src/codeql_ruby/ast/Variable.qll +++ b/ql/src/codeql_ruby/ast/Variable.qll @@ -53,14 +53,14 @@ class Variable extends TVariable { } /** A local variable. */ -class LocalVariable extends Variable { +class LocalVariable extends Variable, TLocalVariable { override LocalVariable::Range range; final override LocalVariableAccess getAnAccess() { result.getVariable() = this } } /** A global variable. */ -class GlobalVariable extends Variable { +class GlobalVariable extends Variable, TGlobalVariable { override GlobalVariable::Range range; final override GlobalVariableAccess getAnAccess() { result.getVariable() = this } diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll index d7119e5dd01..8ef650ff1cf 100644 --- a/ql/src/codeql_ruby/ast/internal/Variable.qll +++ b/ql/src/codeql_ruby/ast/internal/Variable.qll @@ -111,9 +111,7 @@ private module Cached { cached newtype TVariable = - TGlobalVariable(TGlobalScope scope, string name) { - name = any(Generated::GlobalVariable var).getValue() - } or + TGlobalVariable(string name) { name = any(Generated::GlobalVariable var).getValue() } or TLocalVariable(VariableScope scope, string name, Generated::Identifier i) { scopeDefinesParameterVariable(scope, name, i) or @@ -223,7 +221,7 @@ module Variable { } module LocalVariable { - class Range extends Variable::Range { + class Range extends Variable::Range, TLocalVariable { private VariableScope scope; private string name; private Generated::Identifier i; @@ -239,17 +237,16 @@ module LocalVariable { } module GlobalVariable { - class Range extends Variable::Range { - private VariableScope scope; + class Range extends Variable::Range, TGlobalVariable { private string name; - Range() { this = TGlobalVariable(scope, name) } + Range() { this = TGlobalVariable(name) } final override string getName() { result = name } final override Location getLocation() { none() } - final override VariableScope getDeclaringScope() { result = scope } + final override VariableScope getDeclaringScope() { result = TGlobalScope() } } }