diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll index 06dee5ef89b..a4c01ec7c97 100644 --- a/ql/src/codeql_ruby/ast/Variable.qll +++ b/ql/src/codeql_ruby/ast/Variable.qll @@ -73,9 +73,56 @@ class VariableAccess extends Expr { /** Gets the variable this identifier refers to. */ Variable getVariable() { result = range.getVariable() } + /** + * Holds if this access is a write access belonging to the explicit + * assignment `assignment`. For example, in + * + * ```rb + * a, b = foo + * ``` + * + * both `a` and `b` are write accesses belonging to the same assignment. + */ + predicate isExplicitWrite(AstNode assignment) { explicitWriteAccess(this, assignment) } + + /** + * Holds if this access is a write access belonging to an implicit assignment. + * For example, in + * + * ```rb + * def m elements + * for e in elements do + * puts e + * end + * end + * ``` + * + * the access to `elements` in the parameter list is an implicit assignment, + * as is the first access to `e`. + */ + predicate isImplicitWrite() { implicitWriteAccess(this) } + final override string toString() { result = this.getVariable().getName() } } +/** An access to a variable where the value is updated. */ +class VariableWriteAccess extends VariableAccess { + VariableWriteAccess() { + this.isExplicitWrite(_) or + this.isImplicitWrite() + } +} + +/** An access to a variable where the value is read. */ +class VariableReadAccess extends VariableAccess { + VariableReadAccess() { + not this instanceof VariableWriteAccess + or + // `x` in `x += y` is considered both a read and a write + this = any(AssignOperation a).getLhs() + } +} + /** An access to a local variable. */ class LocalVariableAccess extends VariableAccess, @token_identifier { final override LocalVariableAccess::Range range; @@ -88,6 +135,12 @@ class LocalVariableAccess extends VariableAccess, @token_identifier { } } +/** An access to a local variable where the value is updated. */ +class LocalVariableWriteAccess extends LocalVariableAccess, VariableWriteAccess { } + +/** An access to a local variable where the value is read. */ +class LocalVariableReadAccess extends LocalVariableAccess, VariableReadAccess { } + /** An access to a local variable. */ class GlobalVariableAccess extends VariableAccess, @token_global_variable { final override GlobalVariableAccess::Range range; @@ -97,3 +150,9 @@ class GlobalVariableAccess extends VariableAccess, @token_global_variable { final override string getAPrimaryQlClass() { result = "GlobalVariableAccess" } } + +/** An access to a global variable where the value is updated. */ +class GlobalVariableWriteAccess extends GlobalVariableAccess, VariableWriteAccess { } + +/** An access to a global variable where the value is read. */ +class GlobalVariableReadAccess extends GlobalVariableAccess, VariableReadAccess { } diff --git a/ql/src/codeql_ruby/ast/internal/Pattern.qll b/ql/src/codeql_ruby/ast/internal/Pattern.qll index 75c348e1a58..b2ecd5a6963 100644 --- a/ql/src/codeql_ruby/ast/internal/Pattern.qll +++ b/ql/src/codeql_ruby/ast/internal/Pattern.qll @@ -3,43 +3,49 @@ private import TreeSitter private import codeql_ruby.ast.internal.Variable private import codeql.Locations -private predicate tuplePatternNode(Generated::AstNode n, boolean parameter) { - n instanceof Generated::DestructuredParameter and - parameter = true - or - n instanceof Generated::DestructuredLeftAssignment and - parameter = false - or - n instanceof Generated::LeftAssignmentList and - parameter = false - or - tuplePatternNode(n.getParent(), parameter) -} - -private predicate patternNode(Generated::AstNode n, boolean parameter) { - tuplePatternNode(n, parameter) - or - parameter = true and - n = any(Callable c).getAParameter() - or - parameter = false and - n in [ - any(Generated::Assignment assign).getLeft(), - any(Generated::OperatorAssignment assign).getLeft(), - any(Generated::ExceptionVariable exceptionVariable).getChild(), - any(Generated::For for).getPattern() - ] -} - /** - * Holds if a variable is assigned at `i`. `parameter` indicates whether it is - * an implicit parameter assignment. + * Holds if `n` is in the left-hand-side of an explicit assignment `assignment`. */ -predicate assignment(Generated::Identifier i, boolean parameter) { patternNode(i, parameter) } +predicate explicitAssignmentNode(Generated::AstNode n, Generated::AstNode assignment) { + n = assignment.(Generated::Assignment).getLeft() + or + n = assignment.(Generated::OperatorAssignment).getLeft() + or + exists(Generated::AstNode parent | + parent = n.getParent() and + explicitAssignmentNode(parent, assignment) + | + parent instanceof Generated::DestructuredLeftAssignment + or + parent instanceof Generated::LeftAssignmentList + ) +} + +/** Holds if `n` is inside an implicit assignment. */ +predicate implicitAssignmentNode(Generated::AstNode n) { + n = any(Generated::ExceptionVariable ev).getChild() + or + n = any(Generated::For for).getPattern() + or + implicitAssignmentNode(n.getParent()) +} + +/** Holds if `n` is inside a parameter. */ +predicate implicitParameterAssignmentNode(Generated::AstNode n, Callable c) { + n = c.getAParameter() + or + implicitParameterAssignmentNode(n.getParent().(Generated::DestructuredParameter), c) +} module Pattern { abstract class Range extends AstNode { - Range() { patternNode(this, _) } + Range() { + explicitAssignmentNode(this, _) + or + implicitAssignmentNode(this) + or + implicitParameterAssignmentNode(this, _) + } abstract Variable getAVariable(); } diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll index 8ef650ff1cf..b8849043f9b 100644 --- a/ql/src/codeql_ruby/ast/internal/Variable.qll +++ b/ql/src/codeql_ruby/ast/internal/Variable.qll @@ -17,8 +17,7 @@ private VariableScope enclosingScope(Generated::AstNode node) { private predicate parameterAssignment( CallableScope::Range scope, string name, Generated::Identifier i ) { - assignment(i, true) and - scope = enclosingScope(i) and + implicitParameterAssignmentNode(i, scope.getScopeElement()) and name = i.getValue() } @@ -50,7 +49,7 @@ private predicate scopeDefinesParameterVariable( /** Holds if `name` is assigned in `scope` at `i`. */ private predicate scopeAssigns(VariableScope scope, string name, Generated::Identifier i) { - assignment(i, false) and + (explicitAssignmentNode(i, _) or implicitAssignmentNode(i)) and name = i.getValue() and scope = enclosingScope(i) } @@ -143,6 +142,22 @@ private module Cached { ) ) } + + private class Access extends Generated::Token { + Access() { access(this, _) or this instanceof Generated::GlobalVariable } + } + + cached + predicate explicitWriteAccess(Access access, Generated::AstNode assignment) { + explicitAssignmentNode(access, assignment) + } + + cached + predicate implicitWriteAccess(Access access) { + implicitAssignmentNode(access) + or + scopeDefinesParameterVariable(_, _, access) + } } import Cached diff --git a/ql/test/library-tests/variables/scopes.rb b/ql/test/library-tests/variables/scopes.rb index 65816cc31b4..9f2a97cc0a4 100644 --- a/ql/test/library-tests/variables/scopes.rb +++ b/ql/test/library-tests/variables/scopes.rb @@ -8,7 +8,7 @@ a = 6 puts a 1.times do | x | puts a # local variable from top-level - a = 3 + a += 3 puts a # local variable from top-level a, b, (c, d) = [4, 5, [6, 7]] puts a # local variable from top-level diff --git a/ql/test/library-tests/variables/varaccess.expected b/ql/test/library-tests/variables/varaccess.expected index 43683ab605d..538fca09713 100644 --- a/ql/test/library-tests/variables/varaccess.expected +++ b/ql/test/library-tests/variables/varaccess.expected @@ -1,3 +1,4 @@ +variableAccess | nested_scopes.rb:5:3:5:3 | a | nested_scopes.rb:5:3:5:3 | a | nested_scopes.rb:4:1:39:3 | class scope | | nested_scopes.rb:7:5:7:5 | a | nested_scopes.rb:7:5:7:5 | a | nested_scopes.rb:6:3:37:5 | module scope | | nested_scopes.rb:9:7:9:7 | a | nested_scopes.rb:9:7:9:7 | a | nested_scopes.rb:8:5:35:7 | module scope | @@ -93,3 +94,102 @@ | 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 | +explicitWrite +| nested_scopes.rb:5:3:5:3 | a | nested_scopes.rb:5:3:5:7 | ... = ... | +| nested_scopes.rb:7:5:7:5 | a | nested_scopes.rb:7:5:7:9 | ... = ... | +| nested_scopes.rb:9:7:9:7 | a | nested_scopes.rb:9:7:9:11 | ... = ... | +| nested_scopes.rb:11:9:11:9 | a | nested_scopes.rb:11:9:11:13 | ... = ... | +| nested_scopes.rb:13:11:13:11 | a | nested_scopes.rb:13:11:13:15 | ... = ... | +| nested_scopes.rb:17:15:17:15 | a | nested_scopes.rb:17:15:17:19 | ... = ... | +| nested_scopes.rb:31:11:31:11 | a | nested_scopes.rb:31:11:31:16 | ... = ... | +| nested_scopes.rb:40:1:40:1 | d | nested_scopes.rb:40:1:40:18 | ... = ... | +| parameters.rb:2:4:2:4 | y | parameters.rb:2:4:2:8 | ... = ... | +| parameters.rb:34:1:34:1 | b | parameters.rb:34:1:34:5 | ... = ... | +| parameters.rb:35:16:35:16 | b | parameters.rb:35:16:35:20 | ... = ... | +| parameters.rb:40:15:40:15 | e | parameters.rb:40:15:40:19 | ... = ... | +| parameters.rb:53:1:53:1 | x | parameters.rb:53:1:53:6 | ... = ... | +| parameters.rb:54:19:54:19 | x | parameters.rb:54:19:54:23 | ... = ... | +| scopes.rb:4:4:4:4 | a | scopes.rb:4:4:4:8 | ... = ... | +| scopes.rb:7:1:7:1 | a | scopes.rb:7:1:7:5 | ... = ... | +| scopes.rb:11:4:11:4 | a | scopes.rb:11:4:11:9 | ... += ... | +| scopes.rb:13:4:13:4 | a | scopes.rb:13:4:13:32 | ... = ... | +| scopes.rb:13:7:13:7 | b | scopes.rb:13:4:13:32 | ... = ... | +| scopes.rb:13:11:13:11 | c | scopes.rb:13:4:13:32 | ... = ... | +| scopes.rb:13:14:13:14 | d | scopes.rb:13:4:13:32 | ... = ... | +| scopes.rb:21:1:21:7 | $global | scopes.rb:21:1:21:12 | ... = ... | +| scopes.rb:24:1:24:6 | script | scopes.rb:24:1:24:11 | ... = ... | +implicitWrite +| nested_scopes.rb:15:23:15:23 | a | +| nested_scopes.rb:16:26:16:26 | x | +| 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: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: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: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 | +readAccess +| nested_scopes.rb:14:16:14:16 | a | +| nested_scopes.rb:15:11:15:11 | a | +| nested_scopes.rb:16:13:16:13 | a | +| nested_scopes.rb:18:15:18:15 | a | +| nested_scopes.rb:18:34:18:34 | a | +| nested_scopes.rb:23:16:23:16 | a | +| nested_scopes.rb:25:14:25:14 | a | +| nested_scopes.rb:32:16:32:16 | a | +| nested_scopes.rb:34:12:34:12 | a | +| nested_scopes.rb:36:10:36:10 | a | +| nested_scopes.rb:38:8:38:8 | a | +| nested_scopes.rb:41:1:41:1 | d | +| parameters.rb:3:9:3:9 | x | +| parameters.rb:4:9:4:9 | y | +| parameters.rb:8:6:8:11 | pizzas | +| parameters.rb:9:25:9:30 | client | +| parameters.rb:11:14:11:19 | pizzas | +| parameters.rb:11:41:11:46 | client | +| parameters.rb:16:3:16:5 | map | +| parameters.rb:17:13:17:15 | key | +| parameters.rb:17:22:17:26 | value | +| parameters.rb:22:3:22:7 | block | +| parameters.rb:25:40:25:43 | name | +| parameters.rb:26:8:26:11 | name | +| parameters.rb:27:8:27:11 | size | +| parameters.rb:31:11:31:15 | first | +| parameters.rb:31:20:31:25 | middle | +| parameters.rb:31:30:31:33 | last | +| parameters.rb:37:11:37:11 | a | +| parameters.rb:37:16:37:16 | b | +| parameters.rb:42:11:42:11 | d | +| parameters.rb:42:16:42:16 | e | +| parameters.rb:46:8:46:8 | _ | +| parameters.rb:50:11:50:11 | a | +| parameters.rb:50:16:50:16 | b | +| parameters.rb:55:9:55:9 | x | +| parameters.rb:56:9:56:9 | y | +| scopes.rb:5:9:5:9 | a | +| scopes.rb:8:6:8:6 | a | +| scopes.rb:10:9:10:9 | a | +| scopes.rb:11:4:11:4 | a | +| scopes.rb:12:9:12:9 | a | +| scopes.rb:14:9:14:9 | a | +| scopes.rb:15:9:15:9 | b | +| scopes.rb:16:9:16:9 | c | +| scopes.rb:17:9:17:9 | d | +| scopes.rb:24:10:24:11 | $0 | diff --git a/ql/test/library-tests/variables/varaccess.ql b/ql/test/library-tests/variables/varaccess.ql index 11282910461..df7f516efb4 100644 --- a/ql/test/library-tests/variables/varaccess.ql +++ b/ql/test/library-tests/variables/varaccess.ql @@ -1,6 +1,15 @@ +import codeql_ruby.AST import codeql_ruby.ast.Variable query predicate variableAccess(VariableAccess access, Variable variable, VariableScope scope) { variable = access.getVariable() and scope = variable.getDeclaringScope() } + +query predicate explicitWrite(VariableWriteAccess write, AstNode assignment) { + write.isExplicitWrite(assignment) +} + +query predicate implicitWrite(VariableWriteAccess write) { write.isImplicitWrite() } + +query predicate readAccess(VariableReadAccess read) { any() } diff --git a/ql/test/qlpack.yml b/ql/test/qlpack.yml index 70b527caa7f..575fc93ddc1 100644 --- a/ql/test/qlpack.yml +++ b/ql/test/qlpack.yml @@ -4,3 +4,4 @@ libraryPathDependencies: - codeql-ruby - codeql-ruby-examples extractor: ruby +tests: .