Categorize variable accesses into reads and (implicit or explicit) writes

2026-04-26 09:15:12 +02:00 · 2021-01-20 13:32:45 +01:00
parent 78771ba4c2
commit 7e374c416a
7 changed files with 226 additions and 36 deletions
--- 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 { }
--- 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();
  }
--- 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
--- 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
--- 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 |
--- 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() }
--- a/ql/test/qlpack.yml
+++ b/ql/test/qlpack.yml
@@ -4,3 +4,4 @@ libraryPathDependencies:
  - codeql-ruby
  - codeql-ruby-examples
 extractor: ruby
+tests: .