Merge pull request #335 from github/hmac/self-flow

Author: hvitved

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

@@ -20,3 +20,5 @@ class Scope extends AstNode, TScopeType {
     result.getName() = name
   }
 }
+
+class SelfScope extends Scope, TSelfScopeType { }

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

@@ -14,6 +14,9 @@ class Stmt extends AstNode, TStmt {
   /** Gets a control-flow node for this statement, if any. */
   CfgNodes::AstCfgNode getAControlFlowNode() { result.getNode() = this }
 
+  /** Gets a control-flow entry node for this statement, if any */
+  AstNode getAControlFlowEntryNode() { result = getAControlFlowEntryNode(this) }
+
   /** Gets the control-flow scope of this statement, if any. */
   CfgScope getCfgScope() { result = getCfgScope(this) }
 

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

@@ -71,6 +71,9 @@ class ClassVariable extends Variable instanceof ClassVariableImpl {
   final override ClassVariableAccess getAnAccess() { result.getVariable() = this }
 }
 
+/** A `self` variable. */
+class SelfVariable extends LocalVariable instanceof SelfVariableImpl { }
+
 /** An access to a variable. */
 class VariableAccess extends Expr instanceof VariableAccessImpl {
   /** Gets the variable this identifier refers to. */
@@ -127,7 +130,7 @@ class VariableReadAccess extends VariableAccess {
 
 /** An access to a local variable. */
 class LocalVariableAccess extends VariableAccess instanceof LocalVariableAccessImpl {
-  final override string getAPrimaryQlClass() { result = "LocalVariableAccess" }
+  override string getAPrimaryQlClass() { result = "LocalVariableAccess" }
 
   /**
    * Holds if this access is a captured variable access. For example in
@@ -144,7 +147,7 @@ class LocalVariableAccess extends VariableAccess instanceof LocalVariableAccessI
    * the access to `x` in the first `puts x` is a captured access, while
    * the access to `x` in the second `puts x` is not.
    */
-  final predicate isCapturedAccess() { isCapturedAccess(this) }
+  predicate isCapturedAccess() { isCapturedAccess(this) }
 }
 
 /** An access to a local variable where the value is updated. */
@@ -185,3 +188,17 @@ class ClassVariableWriteAccess extends ClassVariableAccess, VariableWriteAccess
 
 /** An access to a class variable where the value is read. */
 class ClassVariableReadAccess extends ClassVariableAccess, VariableReadAccess { }
+
+/** An access to the `self` variable */
+class SelfVariableAccess extends LocalVariableAccess instanceof SelfVariableAccessImpl {
+  final override string getAPrimaryQlClass() { result = "SelfVariableAccess" }
+}
+
+/** An access to the `self` variable where the value is read. */
+class SelfVariableReadAccess extends SelfVariableAccess, VariableReadAccess {
+  // We override the definition in `LocalVariableAccess` because it gives the
+  // wrong result for synthesised `self` variables.
+  override predicate isCapturedAccess() {
+    this.getVariable().getDeclaringScope() != this.getCfgScope()
+  }
+}

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

@@ -235,7 +235,9 @@ private module Cached {
       isScopeResolutionMethodCall(g, i)
     } or
     TSelfReal(Ruby::Self g) or
-    TSelfSynth(AST::AstNode parent, int i) { mkSynthChild(SelfKind(), parent, i) } or
+    TSelfSynth(AST::AstNode parent, int i, AST::SelfVariable v) {
+      mkSynthChild(SelfKind(v), parent, i)
+    } or
     TSimpleParameter(Ruby::Identifier g) { g instanceof Parameter::Range } or
     TSimpleSymbolLiteral(Ruby::SimpleSymbol g) or
     TSingletonClass(Ruby::SingletonClass g) or
@@ -476,7 +478,7 @@ private module Cached {
     or
     result = TRShiftExprSynth(parent, i)
     or
-    result = TSelfSynth(parent, i)
+    result = TSelfSynth(parent, i, _)
     or
     result = TSplatExprSynth(parent, i)
     or
@@ -693,12 +695,16 @@ class TNamedParameter =
 class TTuplePattern = TTuplePatternParameter or TDestructuredLeftAssignment or TLeftAssignmentList;
 
 class TVariableAccess =
-  TLocalVariableAccess or TGlobalVariableAccess or TInstanceVariableAccess or TClassVariableAccess;
+  TLocalVariableAccess or TGlobalVariableAccess or TInstanceVariableAccess or
+      TClassVariableAccess or TSelfVariableAccess;
 
-class TLocalVariableAccess = TLocalVariableAccessReal or TLocalVariableAccessSynth;
+class TLocalVariableAccess =
+  TLocalVariableAccessReal or TLocalVariableAccessSynth or TSelfVariableAccess;
 
 class TGlobalVariableAccess = TGlobalVariableAccessReal or TGlobalVariableAccessSynth;
 
 class TInstanceVariableAccess = TInstanceVariableAccessReal or TInstanceVariableAccessSynth;
 
 class TClassVariableAccess = TClassVariableAccessReal or TClassVariableAccessSynth;
+
+class TSelfVariableAccess = TSelfReal or TSelfSynth;

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

@@ -60,7 +60,7 @@ class IdentifierMethodCall extends MethodCallImpl, TIdentifierMethodCall {
 
   final override string getMethodNameImpl() { result = g.getValue() }
 
-  final override AstNode getReceiverImpl() { result = TSelfSynth(this, 0) }
+  final override AstNode getReceiverImpl() { result = TSelfSynth(this, 0, _) }
 
   final override Expr getArgumentImpl(int n) { none() }
 
@@ -97,7 +97,7 @@ class RegularMethodCall extends MethodCallImpl, TRegularMethodCall {
     not exists(g.getReceiver()) and
     toGenerated(result) = g.getMethod().(Ruby::ScopeResolution).getScope()
     or
-    result = TSelfSynth(this, 0)
+    result = TSelfSynth(this, 0, _)
   }
 
   final override string getMethodNameImpl() {

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

@@ -5,6 +5,12 @@ private import codeql.ruby.ast.internal.Parameter
 
 class TScopeType = TMethodBase or TModuleLike or TBlockLike;
 
+/**
+ * The scope of a `self` variable.
+ * This differs from a normal scope because it is not affected by blocks or lambdas.
+ */
+class TSelfScopeType = TMethodBase or TModuleBase;
+
 private class TBlockLike = TDoBlock or TLambda or TBlock or TEndBlock;
 
 private class TModuleLike = TToplevel or TModuleDeclaration or TClassDeclaration or TSingletonClass;
@@ -29,6 +35,12 @@ module Scope {
       result = this.getOuterScope().getEnclosingMethod()
     }
 
+    SelfBase::Range getEnclosingSelfScope() {
+      this instanceof SelfBase::Range and result = this
+      or
+      not this instanceof SelfBase::Range and result = this.getOuterScope().getEnclosingSelfScope()
+    }
+
     Range getOuterScope() { result = scopeOf(this) }
   }
 }
@@ -59,6 +71,15 @@ module ModuleBase {
   class Range extends Scope::Range, TypeRange { }
 }
 
+module SelfBase {
+  class TypeRange = MethodBase::TypeRange or ModuleBase::TypeRange;
+
+  /**
+   * A `self` variable can appear in a class, module, method or at the top level.
+   */
+  class Range extends Scope::Range, TypeRange { }
+}
+
 pragma[noinline]
 private predicate rankHeredocBody(File f, Ruby::HeredocBody b, int i) {
   b =

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

@@ -5,6 +5,7 @@ private import TreeSitter
 private import codeql.ruby.ast.internal.Call
 private import codeql.ruby.ast.internal.Variable
 private import codeql.ruby.ast.internal.Pattern
+private import codeql.ruby.ast.internal.Scope
 private import codeql.ruby.AST
 
 /** A synthesized AST node kind. */
@@ -34,7 +35,7 @@ newtype SynthKind =
   RShiftExprKind() or
   SplatExprKind() or
   StmtSequenceKind() or
-  SelfKind() or
+  SelfKind(SelfVariable v) or
   SubExprKind() or
   ConstantReadAccessKind(string value) { any(Synthesis s).constantReadAccess(value) }
 
@@ -142,7 +143,7 @@ private predicate hasLocation(AstNode n, Location l) {
 private module ImplicitSelfSynthesis {
   pragma[nomagic]
   private predicate identifierMethodCallSelfSynthesis(AstNode mc, int i, Child child) {
-    child = SynthChild(SelfKind()) and
+    child = SynthChild(SelfKind(TSelfVariable(scopeOf(toGenerated(mc)).getEnclosingSelfScope()))) and
     mc = TIdentifierMethodCall(_) and
     i = 0
   }
@@ -163,7 +164,7 @@ private module ImplicitSelfSynthesis {
       not exists(g.(Ruby::Call).getReceiver()) and
       not exists(g.(Ruby::Call).getMethod().(Ruby::ScopeResolution).getScope())
     ) and
-    child = SynthChild(SelfKind()) and
+    child = SynthChild(SelfKind(TSelfVariable(scopeOf(toGenerated(mc)).getEnclosingSelfScope()))) and
     i = 0
   }
 

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

@@ -133,6 +133,7 @@ private module Cached {
       not scopeDefinesParameterVariable(scope, name, _) and
       not inherits(scope, name, _)
     } or
+    TSelfVariable(SelfBase::Range scope) or
     TLocalVariableSynth(AstNode n, int i) { any(Synthesis s).localVariable(n, i) }
 
   // Db types that can be vcalls
@@ -307,7 +308,8 @@ private module Cached {
       access(this, _) or
       this instanceof Ruby::GlobalVariable or
       this instanceof Ruby::InstanceVariable or
-      this instanceof Ruby::ClassVariable
+      this instanceof Ruby::ClassVariable or
+      this instanceof Ruby::Self
     }
   }
 
@@ -374,9 +376,10 @@ abstract class VariableImpl extends TVariable {
   abstract Location getLocationImpl();
 }
 
-class TVariableReal = TGlobalVariable or TClassVariable or TInstanceVariable or TLocalVariableReal;
+class TVariableReal =
+  TGlobalVariable or TClassVariable or TInstanceVariable or TLocalVariableReal or TSelfVariable;
 
-class TLocalVariable = TLocalVariableReal or TLocalVariableSynth;
+class TLocalVariable = TLocalVariableReal or TLocalVariableSynth or TSelfVariable;
 
 /**
  * This class only exists to avoid negative recursion warnings. Ideally,
@@ -475,6 +478,18 @@ class ClassVariableImpl extends VariableReal, TClassVariable {
   final override Scope::Range getDeclaringScopeImpl() { result = scope }
 }
 
+class SelfVariableImpl extends VariableReal, TSelfVariable {
+  private SelfBase::Range scope;
+
+  SelfVariableImpl() { this = TSelfVariable(scope) }
+
+  final override string getNameImpl() { result = "self" }
+
+  final override Location getLocationImpl() { result = scope.getLocation() }
+
+  final override Scope::Range getDeclaringScopeImpl() { result = scope }
+}
+
 abstract class VariableAccessImpl extends Expr, TVariableAccess {
   abstract VariableImpl getVariableImpl();
 }
@@ -602,3 +617,26 @@ private class ClassVariableAccessSynth extends ClassVariableAccessRealImpl,
 
   final override string toString() { result = v.getName() }
 }
+
+abstract class SelfVariableAccessImpl extends LocalVariableAccessImpl, TSelfVariableAccess { }
+
+private class SelfVariableAccessReal extends SelfVariableAccessImpl, TSelfReal {
+  private Ruby::Self self;
+  private SelfVariable var;
+
+  SelfVariableAccessReal() { this = TSelfReal(self) and var = TSelfVariable(scopeOf(self)) }
+
+  final override SelfVariable getVariableImpl() { result = var }
+
+  final override string toString() { result = var.toString() }
+}
+
+private class SelfVariableAccessSynth extends SelfVariableAccessImpl, TSelfSynth {
+  private SelfVariable v;
+
+  SelfVariableAccessSynth() { this = TSelfSynth(_, _, v) }
+
+  final override LocalVariable getVariableImpl() { result = v }
+
+  final override string toString() { result = v.getName() }
+}

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

@@ -59,7 +59,7 @@ class ExitNode extends CfgNode, TExitNode {
 /**
  * A node for an AST node.
  *
- * Each AST node maps to zero or more `AstCfgNode`s: zero when the node in unreachable
+ * Each AST node maps to zero or more `AstCfgNode`s: zero when the node is unreachable
  * (dead) code or not important for control flow, and multiple when there are different
  * splits for the AST node.
  */

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

@@ -808,19 +808,28 @@ module Trees {
     }
   }
 
+  /**
+   * Namespaces (i.e. modules or classes) behave like other `BodyStmt`s except they are
+   * executed in pre-order rather than post-order. We do this in order to insert a write for
+   * `self` before any subsequent reads in the namespace body.
+   */
   private class NamespaceTree extends BodyStmtTree, Namespace {
-    final override predicate first(AstNode first) {
-      this.firstInner(first)
-      or
-      not exists(this.getAChild(_)) and
-      first = this
-    }
+    final override predicate first(AstNode first) { first = this }
 
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {
       BodyStmtTree.super.succ(pred, succ, c)
       or
-      succ = this and
-      this.lastInner(pred, c)
+      pred = this and
+      this.firstInner(succ) and
+      c instanceof SimpleCompletion
+    }
+
+    final override predicate last(AstNode last, Completion c) {
+      this.lastInner(last, c)
+      or
+      not exists(this.getAChild(_)) and
+      last = this and
+      c.isValidFor(this)
     }
   }
 
@@ -956,8 +965,6 @@ module Trees {
     final override ControlFlowTree getChildElement(int i) { result = this.getValue() and i = 0 }
   }
 
-  private class SelfTree extends LeafTree, Self { }
-
   private class SimpleParameterTree extends NonDefaultValueParameterTree, SimpleParameter { }
 
   // Corner case: For duplicated '_' parameters, only the first occurence has a defining