Merge pull request #9206 from aibaars/instance-variable-flow

Ruby: flow through instance variables

Author: aibaars

ruby/ql/lib/change-notes/2022-05-23-flow-through-instance-variables.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+Support for data flow through instance variables has been added.

ruby/ql/lib/codeql/ruby/ast/Variable.qll

@@ -181,6 +181,17 @@ class GlobalVariableReadAccess extends GlobalVariableAccess, VariableReadAccess
 /** An access to an instance variable. */
 class InstanceVariableAccess extends VariableAccess instanceof InstanceVariableAccessImpl {
   final override string getAPrimaryQlClass() { result = "InstanceVariableAccess" }
+
+  /**
+   * Gets the synthetic receiver (`self`) of this instance variable access.
+   */
+  final SelfVariableAccess getReceiver() { synthChild(this, 0, result) }
+
+  final override AstNode getAChild(string pred) {
+    result = VariableAccess.super.getAChild(pred)
+    or
+    pred = "getReceiver" and result = this.getReceiver()
+  }
 }
 
 /** An access to an instance variable where the value is updated. */

ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll

@@ -210,6 +210,38 @@ private module ImplicitSelfSynthesis {
       regularMethodCallSelfSynthesis(parent, i, child)
     }
   }
+
+  pragma[nomagic]
+  private AstNode instanceVarAccessSynthParentStar(InstanceVariableAccess var) {
+    result = var
+    or
+    instanceVarAccessSynthParentStar(var) = getSynthChild(result, _)
+  }
+
+  /**
+   * Gets the `SelfKind` for instance variable access `var`. This is based on the
+   * "owner" of `var`; for real nodes this is the node itself, for synthetic nodes
+   * this is the closest parent that is a real node.
+   */
+  pragma[nomagic]
+  private SelfKind getSelfKind(InstanceVariableAccess var) {
+    exists(Ruby::AstNode owner |
+      owner = toGenerated(instanceVarAccessSynthParentStar(var)) and
+      result = SelfKind(TSelfVariable(scopeOf(owner).getEnclosingSelfScope()))
+    )
+  }
+
+  pragma[nomagic]
+  private predicate instanceVariableSelfSynthesis(InstanceVariableAccess var, int i, Child child) {
+    child = SynthChild(getSelfKind(var)) and
+    i = 0
+  }
+
+  private class InstanceVariableSelfSynthesis extends Synthesis {
+    final override predicate child(AstNode parent, int i, Child child) {
+      instanceVariableSelfSynthesis(parent, i, child)
+    }
+  }
 }
 
 private module SetterDesugar {

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

@@ -682,6 +682,24 @@ module ExprNodes {
     final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
   }
 
+  private class InstanceVariableAccessMapping extends ExprChildMapping, InstanceVariableAccess {
+    override predicate relevantChild(AstNode n) { n = this.getReceiver() }
+  }
+
+  /** A control-flow node that wraps an `InstanceVariableAccess` AST expression. */
+  class InstanceVariableAccessCfgNode extends ExprCfgNode {
+    override string getAPrimaryQlClass() { result = "InstanceVariableAccessCfgNode" }
+
+    override InstanceVariableAccessMapping e;
+
+    override InstanceVariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+
+    /**
+     * Gets the synthetic receiver (`self`) of this instance variable access.
+     */
+    final CfgNode getReceiver() { e.hasCfgChild(e.getReceiver(), this, result) }
+  }
+
   /** A control-flow node that wraps a `VariableWriteAccess` AST expression. */
   class VariableWriteAccessCfgNode extends ExprCfgNode {
     override string getAPrimaryQlClass() { result = "VariableWriteAccessCfgNode" }
@@ -709,13 +727,26 @@ module ExprNodes {
     final override ConstantWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
   }
 
-  /** A control-flow node that wraps a `InstanceVariableWriteAccess` AST expression. */
-  class InstanceVariableWriteAccessCfgNode extends ExprCfgNode {
-    override string getAPrimaryQlClass() { result = "InstanceVariableWriteAccessCfgNode" }
+  /** A control-flow node that wraps an `InstanceVariableReadAccess` AST expression. */
+  class InstanceVariableReadAccessCfgNode extends InstanceVariableAccessCfgNode {
+    InstanceVariableReadAccessCfgNode() { this.getNode() instanceof InstanceVariableReadAccess }
+
+    override string getAPrimaryQlClass() { result = "InstanceVariableReadAccessCfgNode" }
+
+    final override InstanceVariableReadAccess getExpr() {
+      result = InstanceVariableAccessCfgNode.super.getExpr()
+    }
+  }
+
+  /** A control-flow node that wraps an `InstanceVariableWriteAccess` AST expression. */
+  class InstanceVariableWriteAccessCfgNode extends InstanceVariableAccessCfgNode {
+    InstanceVariableWriteAccessCfgNode() { this.getNode() instanceof InstanceVariableWriteAccess }
 
-    override InstanceVariableWriteAccess e;
+    override string getAPrimaryQlClass() { result = "InstanceVariableWriteAccessCfgNode" }
 
-    final override InstanceVariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+    final override InstanceVariableWriteAccess getExpr() {
+      result = InstanceVariableAccessCfgNode.super.getExpr()
+    }
   }
 
   /** A control-flow node that wraps a `StringInterpolationComponent` AST expression. */

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

@@ -1006,7 +1006,9 @@ module Trees {
     final override ControlFlowTree getChildElement(int i) { result = this.getComponent(i) }
   }
 
-  private class InstanceVariableTree extends LeafTree, InstanceVariableAccess { }
+  private class InstanceVariableTree extends StandardPostOrderTree, InstanceVariableAccess {
+    final override ControlFlowTree getChildElement(int i) { result = this.getReceiver() and i = 0 }
+  }
 
   private class KeywordParameterTree extends DefaultValueParameterTree, KeywordParameter {
     final override Expr getDefaultValueExpr() { result = this.getDefaultValue() }

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

@@ -219,7 +219,10 @@ private module Cached {
     } or
     TSelfParameterNode(MethodBase m) or
     TBlockParameterNode(MethodBase m) or
-    TExprPostUpdateNode(CfgNodes::ExprCfgNode n) { n instanceof Argument } or
+    TExprPostUpdateNode(CfgNodes::ExprCfgNode n) {
+      n instanceof Argument or
+      n = any(CfgNodes::ExprNodes::InstanceVariableAccessCfgNode v).getReceiver()
+    } or
     TSummaryNode(
       FlowSummaryImpl::Public::SummarizedCallable c,
       FlowSummaryImpl::Private::SummaryNodeState state
@@ -339,6 +342,7 @@ private module Cached {
       not cv.isInt(_) or
       cv.getInt() in [0 .. 10]
     } or
+    TFieldContent(string name) { name = any(InstanceVariable v).getName() } or
     TUnknownElementContent()
 
   /**
@@ -795,11 +799,40 @@ predicate jumpStep(Node pred, Node succ) {
   succ.asExpr().getExpr().(ConstantReadAccess).getValue() = pred.asExpr().getExpr()
 }
 
+/**
+ * Holds if data can flow from `node1` to `node2` via an assignment to
+ * content `c`.
+ */
 predicate storeStep(Node node1, ContentSet c, Node node2) {
+  // Instance variable assignment, `@var = src`
+  node2.(PostUpdateNode).getPreUpdateNode().asExpr() =
+    any(CfgNodes::ExprNodes::InstanceVariableWriteAccessCfgNode var |
+      exists(CfgNodes::ExprNodes::AssignExprCfgNode assign |
+        var = assign.getLhs() and
+        node1.asExpr() = assign.getRhs()
+      |
+        c.isSingleton(any(Content::FieldContent ct |
+            ct.getName() = var.getExpr().getVariable().getName()
+          ))
+      )
+    ).getReceiver()
+  or
   FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, c, node2)
 }
 
+/**
+ * Holds if there is a read step of content `c` from `node1` to `node2`.
+ */
 predicate readStep(Node node1, ContentSet c, Node node2) {
+  // Instance variable read access, `@var`
+  node2.asExpr() =
+    any(CfgNodes::ExprNodes::InstanceVariableReadAccessCfgNode var |
+      node1.asExpr() = var.getReceiver() and
+      c.isSingleton(any(Content::FieldContent ct |
+          ct.getName() = var.getExpr().getVariable().getName()
+        ))
+    )
+  or
   FlowSummaryImpl::Private::Steps::summaryReadStep(node1, c, node2)
 }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

@@ -223,6 +223,18 @@ module Content {
     override string toString() { result = "element" }
   }
 
+  /** A field of an object, for example an instance variable. */
+  class FieldContent extends Content, TFieldContent {
+    private string name;
+
+    FieldContent() { this = TFieldContent(name) }
+
+    /** Gets the name of the field. */
+    string getName() { result = name }
+
+    override string toString() { result = name }
+  }
+
   /** Gets the element content corresponding to constant value `cv`. */
   ElementContent getElementContent(ConstantValue cv) {
     result = TKnownElementContent(cv)

ruby/ql/test/library-tests/ast/Ast.expected

@@ -1234,6 +1234,7 @@ control/cases.rb:
 #  150|     getBranch: [InClause] in ... then ...
 #  150|       getPattern: [ReferencePattern] ^...
 #  150|         getExpr: [InstanceVariableAccess] @foo
+#  150|           getReceiver: [SelfVariableAccess] self
 #  151|     getBranch: [InClause] in ... then ...
 #  151|       getPattern: [ReferencePattern] ^...
 #  151|         getExpr: [ClassVariableAccess] @@foo
@@ -1246,6 +1247,7 @@ control/cases.rb:
 #  156|     getBranch: [InClause] in ... then ...
 #  156|       getPattern: [ReferencePattern] ^...
 #  156|         getExpr: [InstanceVariableAccess] @foo
+#  156|           getReceiver: [SelfVariableAccess] self
 #  157|     getBranch: [InClause] in ... then ...
 #  157|       getPattern: [ReferencePattern] ^...
 #  157|         getExpr: [AddExpr] ... + ...
@@ -2771,9 +2773,11 @@ operations/operations.rb:
 #   87|   getStmt: [ClassDeclaration] X
 #   88|     getStmt: [AssignExpr] ... = ...
 #   88|       getAnOperand/getLeftOperand: [InstanceVariableAccess] @x
+#   88|         getReceiver: [SelfVariableAccess] self
 #   88|       getAnOperand/getRightOperand: [IntegerLiteral] 1
 #   89|     getStmt: [AssignAddExpr] ... += ...
 #   89|       getAnOperand/getLeftOperand: [InstanceVariableAccess] @x
+#   89|         getReceiver: [SelfVariableAccess] self
 #   89|       getAnOperand/getRightOperand: [IntegerLiteral] 2
 #   91|     getStmt: [AssignExpr] ... = ...
 #   91|       getAnOperand/getLeftOperand: [ClassVariableAccess] @@y

ruby/ql/test/library-tests/ast/AstDesugar.expected

@@ -836,8 +836,10 @@ operations/operations.rb:
 #   89| [AssignAddExpr] ... += ...
 #   89|   getDesugared: [AssignExpr] ... = ...
 #   89|     getAnOperand/getLeftOperand: [InstanceVariableAccess] @x
+#   89|       getReceiver: [SelfVariableAccess] self
 #   89|     getAnOperand/getRightOperand: [AddExpr] ... + ...
 #   89|       getAnOperand/getLeftOperand/getReceiver: [InstanceVariableAccess] @x
+#   89|         getReceiver: [SelfVariableAccess] self
 #   89|       getAnOperand/getArgument/getRightOperand: [IntegerLiteral] 2
 #   92| [AssignDivExpr] ... /= ...
 #   92|   getDesugared: [AssignExpr] ... = ...

ruby/ql/test/library-tests/controlflow/graph/Cfg.expected

@@ -1423,9 +1423,12 @@ cfg.html.erb:
 #    5| @title
 #-----|  -> self
 
-#    5| enter cfg.html.erb
+#    5| self
 #-----|  -> @title
 
+#    5| enter cfg.html.erb
+#-----|  -> self
+
 #    5| exit cfg.html.erb
 
 #    5| exit cfg.html.erb (normal)
@@ -2690,11 +2693,14 @@ cfg.rb:
 #-----|  -> ... > ...
 
 #  115| C
-#-----|  -> @field
+#-----|  -> self
 
 #  116| @field
 #-----|  -> 42
 
+#  116| self
+#-----|  -> @field
+
 #  116| ... = ...
 #-----|  -> @@static_field
 
@@ -4194,23 +4200,32 @@ desugar.rb:
 #-----|  -> call to []
 
 #   29| X
-#-----|  -> @x
+#-----|  -> self
 
 #   30| @x
 #-----|  -> 1
 
-#   30| ... = ...
+#   30| self
 #-----|  -> @x
 
+#   30| ... = ...
+#-----|  -> self
+
 #   30| 1
 #-----|  -> ... = ...
 
 #   31| @x
-#-----|  -> @x
+#-----|  -> self
 
 #   31| @x
 #-----|  -> 2
 
+#   31| self
+#-----|  -> @x
+
+#   31| self
+#-----|  -> @x
+
 #   31| ... = ...
 #-----|  -> @@y