Merge pull request #118 from github/aibaars/dataflow

More dataflow steps

ql/src/codeql_ruby/AST.qll

@@ -7,6 +7,7 @@ import ast.Module
 import ast.Parameter
 import ast.Operation
 import ast.Pattern
+import ast.Statement
 import ast.Variable
 private import ast.internal.AST
 

ql/src/codeql_ruby/ast/Control.qll

@@ -17,7 +17,7 @@ class ControlExpr extends Expr {
  * A conditional expression: `if`/`unless` (including expression-modifier
  * variants), and ternary-if (`?:`) expressions.
  */
-class ConditionalExpr extends Expr {
+class ConditionalExpr extends ControlExpr {
   override ConditionalExpr::Range range;
 
   /**

ql/src/codeql_ruby/ast/Statement.qll

@@ -0,0 +1,80 @@
+private import codeql_ruby.AST
+private import codeql_ruby.CFG
+private import internal.Statement
+private import codeql_ruby.controlflow.internal.ControlFlowGraphImpl
+
+/**
+ * A statement.
+ *
+ * This is the root QL class for all statements.
+ */
+class Stmt extends AstNode {
+  override Stmt::Range range;
+
+  /** Gets a control-flow node for this statement, if any. */
+  CfgNodes::AstCfgNode getAControlFlowNode() { result.getNode() = this }
+
+  /** Gets the control-flow scope of this statement, if any. */
+  CfgScope getCfgScope() { result = getCfgScope(this) }
+
+  /** Gets the enclosing callable, if any. */
+  Callable getEnclosingCallable() { result = this.getCfgScope() }
+}
+
+/**
+ * A statement that may return a value: `return`, `break` and `next`.
+ *
+ * ```rb
+ * return
+ * return value
+ * break
+ * break value
+ * next
+ * next value
+ * ```
+ */
+class ReturningStmt extends Stmt {
+  override ReturningStmt::Range range;
+
+  /** Gets the returned value, if any. */
+  final Expr getValue() { result = range.getValue() }
+}
+
+/**
+ * A `return` statement.
+ * ```rb
+ * return
+ * return value
+ * ```
+ */
+class ReturnStmt extends ReturningStmt, @return {
+  final override ReturnStmt::Range range;
+
+  final override string getAPrimaryQlClass() { result = "ReturnStmt" }
+}
+
+/**
+ * A `break` statement.
+ * ```rb
+ * break
+ * break value
+ * ```
+ */
+class BreakStmt extends ReturningStmt, @break {
+  final override BreakStmt::Range range;
+
+  final override string getAPrimaryQlClass() { result = "BreakStmt" }
+}
+
+/**
+ * A `next` statement.
+ * ```rb
+ * next
+ * next value
+ * ```
+ */
+class NextStmt extends ReturningStmt, @next {
+  final override NextStmt::Range range;
+
+  final override string getAPrimaryQlClass() { result = "NextStmt" }
+}

ql/src/codeql_ruby/ast/internal/AST.qll

@@ -43,10 +43,6 @@ module AstNode {
       or
       this = any(Generated::RestAssignment ra).getChild()
       or
-      this instanceof Generated::Return
-      or
-      this instanceof Generated::Break
-      or
       this instanceof Generated::Alias
       or
       this instanceof Generated::SymbolArray
@@ -65,8 +61,6 @@ module AstNode {
       or
       this instanceof Generated::EmptyStatement
       or
-      this instanceof Generated::Next
-      or
       this instanceof Generated::Redo
       or
       this instanceof Generated::Hash

ql/src/codeql_ruby/ast/internal/Statement.qll

@@ -0,0 +1,53 @@
+private import codeql_ruby.AST
+private import codeql_ruby.ast.internal.AST
+private import codeql_ruby.ast.internal.TreeSitter
+
+module Stmt {
+  abstract class Range extends AstNode::Range { }
+}
+
+module ReturningStmt {
+  abstract class Range extends Stmt::Range {
+    abstract Generated::ArgumentList getArgumentList();
+
+    final Expr getValue() {
+      exists(Generated::ArgumentList a, int c |
+        a = this.getArgumentList() and c = count(a.getChild(_))
+      |
+        result = a.getChild(0) and c = 1
+        or
+        result = a and c > 1
+      )
+    }
+  }
+}
+
+module ReturnStmt {
+  class Range extends ReturningStmt::Range, @return {
+    final override Generated::Return generated;
+
+    final override string toString() { result = "return" }
+
+    final override Generated::ArgumentList getArgumentList() { result = generated.getChild() }
+  }
+}
+
+module BreakStmt {
+  class Range extends ReturningStmt::Range, @break {
+    final override Generated::Break generated;
+
+    final override string toString() { result = "break" }
+
+    final override Generated::ArgumentList getArgumentList() { result = generated.getChild() }
+  }
+}
+
+module NextStmt {
+  class Range extends ReturningStmt::Range, @next {
+    final override Generated::Next generated;
+
+    final override string toString() { result = "next" }
+
+    final override Generated::ArgumentList getArgumentList() { result = generated.getChild() }
+  }
+}

ql/src/codeql_ruby/controlflow/CfgNodes.qll

@@ -112,6 +112,19 @@ class ExprCfgNode extends AstCfgNode {
   Expr getExpr() { result = e }
 }
 
+/** A control-flow node that wraps a return-like statement. */
+class ReturningCfgNode extends AstCfgNode {
+  ReturningStmt s;
+
+  ReturningCfgNode() { s = this.getNode() }
+
+  /** Gets the node of the returned value, if any. */
+  ExprCfgNode getReturnedValueNode() {
+    result = this.getAPredecessor() and
+    result.getNode() = s.getValue()
+  }
+}
+
 /**
  * A class for mapping parent-child AST nodes to parent-child CFG nodes.
  */
@@ -122,14 +135,16 @@ abstract private class ExprChildMapping extends Expr {
    */
   abstract predicate relevantChild(Expr child);
 
-  private AstNode getAChildStar() {
+  private Generated::AstNode getAChildStar() {
     result = this
     or
     result.(Generated::AstNode).getParent() = this.getAChildStar()
   }
 
   pragma[noinline]
-  private BasicBlock getABasicBlockInScope() { result.getANode().getNode() = this.getAChildStar() }
+  private BasicBlock getABasicBlockInScope() {
+    result.getANode() = TAstNode(this.getAChildStar(), _)
+  }
 
   pragma[nomagic]
   private predicate reachesBasicBlockBase(Expr child, CfgNode cfn, BasicBlock bb) {
@@ -238,6 +253,45 @@ module ExprNodes {
     final ExprCfgNode getReceiver() { e.hasCfgChild(e.getReceiver(), this, result) }
   }
 
+  private class CaseExprChildMapping extends ExprChildMapping, CaseExpr {
+    override predicate relevantChild(Expr e) { e = this.getValue() or e = this.getBranch(_) }
+  }
+
+  /** A control-flow node that wraps a `CaseExpr` AST expression. */
+  class CaseExprCfgNode extends ExprCfgNode {
+    override CaseExprChildMapping e;
+
+    final override CaseExpr getExpr() { result = ExprCfgNode.super.getExpr() }
+
+    /** Gets the expression being compared, if any. */
+    final ExprCfgNode getValue() { e.hasCfgChild(e.getValue(), this, result) }
+
+    /**
+     * Gets the `n`th branch of this case expression.
+     */
+    final ExprCfgNode getBranch(int n) { e.hasCfgChild(e.getBranch(n), this, result) }
+  }
+
+  private class ConditionalExprChildMapping extends ExprChildMapping, ConditionalExpr {
+    override predicate relevantChild(Expr e) { e = this.getCondition() or e = this.getBranch(_) }
+  }
+
+  /** A control-flow node that wraps a `ConditionalExpr` AST expression. */
+  class ConditionalExprCfgNode extends ExprCfgNode {
+    override ConditionalExprChildMapping e;
+
+    final override ConditionalExpr getExpr() { result = ExprCfgNode.super.getExpr() }
+
+    /** Gets the condition expression. */
+    final ExprCfgNode getCondition() { e.hasCfgChild(e.getCondition(), this, result) }
+
+    /**
+     * Gets the branch of this conditional expression that is taken when the condition
+     * evaluates to cond, if any.
+     */
+    final ExprCfgNode getBranch(boolean cond) { e.hasCfgChild(e.getBranch(cond), this, result) }
+  }
+
   private class ExprSequenceChildMapping extends ExprChildMapping, ExprSequence {
     override predicate relevantChild(Expr e) { e = this.getAnExpr() }
   }
@@ -255,7 +309,21 @@ module ExprNodes {
     final ExprCfgNode getExpr(int n) { e.hasCfgChild(e.getExpr(n), this, result) }
   }
 
-  /** A control-flow node that wraps an `ExprSequence` AST expression. */
+  private class ForExprChildMapping extends ExprChildMapping, ForExpr {
+    override predicate relevantChild(Expr e) { e = this.getValue() }
+  }
+
+  /** A control-flow node that wraps a `ForExpr` AST expression. */
+  class ForExprCfgNode extends ExprCfgNode {
+    override ForExprChildMapping e;
+
+    final override ForExpr getExpr() { result = ExprCfgNode.super.getExpr() }
+
+    /** Gets the value being iterated over. */
+    final ExprCfgNode getValue() { e.hasCfgChild(e.getValue(), this, result) }
+  }
+
+  /** A control-flow node that wraps a `ParenthesizedExpr` AST expression. */
   class ParenthesizedExprCfgNode extends ExprSequenceCfgNode {
     ParenthesizedExprCfgNode() { this.getExpr() instanceof ParenthesizedExpr }
   }

ql/src/codeql_ruby/dataflow/internal/DataFlowDispatch.qll

@@ -2,7 +2,9 @@ private import ruby
 private import codeql_ruby.CFG
 private import DataFlowPrivate
 
-newtype TReturnKind = TNormalReturnKind()
+newtype TReturnKind =
+  TNormalReturnKind() or
+  TBreakReturnKind()
 
 /**
  * Gets a node that can read the value returned from `call` with return kind
@@ -27,6 +29,13 @@ class NormalReturnKind extends ReturnKind, TNormalReturnKind {
   override string toString() { result = "return" }
 }
 
+/**
+ * A value returned from a callable using a `break` statement.
+ */
+class BreakReturnKind extends ReturnKind, TBreakReturnKind {
+  override string toString() { result = "break" }
+}
+
 class DataFlowCallable = CfgScope;
 
 class DataFlowCall extends CfgNodes::ExprNodes::CallCfgNode {

ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll

@@ -109,6 +109,7 @@ private module Cached {
   cached
   newtype TNode =
     TExprNode(CfgNodes::ExprCfgNode n) or
+    TReturningNode(CfgNodes::ReturningCfgNode n) or
     TSsaDefinitionNode(Ssa::Definition def) or
     TParameterNode(Parameter p) or
     TExprPostUpdateNode(CfgNodes::ExprCfgNode n) { n.getNode() instanceof Argument }
@@ -125,8 +126,30 @@ private module Cached {
     or
     nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs()
     or
-    nodeFrom.asExpr() =
-      nodeTo.asExpr().(CfgNodes::ExprNodes::ParenthesizedExprCfgNode).getLastExpr()
+    nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::ExprSequenceCfgNode).getLastExpr()
+    or
+    nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::ConditionalExprCfgNode).getBranch(_)
+    or
+    nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::CaseExprCfgNode).getBranch(_)
+    or
+    exists(CfgNodes::ExprCfgNode exprTo, ExprReturnNode n |
+      nodeFrom = n and
+      exprTo = nodeTo.asExpr() and
+      n.getKind() instanceof BreakReturnKind and
+      exprTo.getNode() instanceof Loop and
+      nodeTo.asExpr().getAPredecessor(any(SuccessorTypes::BreakSuccessor s)) = n.getExprNode()
+    )
+    or
+    nodeFrom.asExpr() = nodeTo.(ExprReturnNode).getExprNode().getReturnedValueNode()
+    or
+    nodeTo.asExpr() =
+      any(CfgNodes::ExprNodes::ForExprCfgNode for |
+        exists(SuccessorType s |
+          not s instanceof SuccessorTypes::BreakSuccessor and
+          exists(for.getAPredecessor(s))
+        ) and
+        nodeFrom.asExpr() = for.getValue()
+      )
   }
 
   cached
@@ -222,12 +245,25 @@ private module ReturnNodes {
    * A data-flow node that represents an expression returned by a callable,
    * either using an explict `return` statement or as the expression of a method body.
    */
-  class ExprReturnNode extends ReturnNode, ExprNode {
-    ExprReturnNode() {
-      none() // TODO
+  class ExprReturnNode extends ReturnNode, NodeImpl, TReturningNode {
+    private CfgNodes::ReturningCfgNode n;
+
+    ExprReturnNode() { this = TReturningNode(n) }
+
+    /** Gets the statement corresponding to this node. */
+    CfgNodes::ReturningCfgNode getExprNode() { result = n }
+
+    override ReturnKind getKind() {
+      if n.getNode() instanceof BreakStmt
+      then result instanceof BreakReturnKind
+      else result instanceof NormalReturnKind
     }
 
-    override ReturnKind getKind() { result instanceof NormalReturnKind }
+    override CfgScope getCfgScope() { result = n.getScope() }
+
+    override Location getLocationImpl() { result = n.getLocation() }
+
+    override string toStringImpl() { result = n.toString() }
   }
 }
 

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

@@ -199,7 +199,7 @@ break_ensure.rb:
 
 #    3| ... > ...
 #-----| false -> if ...
-#-----| true -> Break
+#-----| true -> break
 
 #    3| element
 #-----|  -> 0
@@ -207,7 +207,7 @@ break_ensure.rb:
 #    3| 0
 #-----|  -> ... > ...
 
-#    4| Break
+#    4| break
 #-----| break -> for ... in ...
 
 #    7| Ensure
@@ -262,7 +262,7 @@ break_ensure.rb:
 
 #   16| ... > ...
 #-----| false -> if ...
-#-----| true -> Break
+#-----| true -> break
 
 #   16| element
 #-----|  -> 0
@@ -270,7 +270,7 @@ break_ensure.rb:
 #   16| 0
 #-----|  -> ... > ...
 
-#   17| Break
+#   17| break
 #-----| break -> [ensure: break] Ensure
 
 #   19| Ensure
@@ -337,7 +337,7 @@ break_ensure.rb:
 
 #   29| call to nil?
 #-----| false -> if ...
-#-----| true -> Return
+#-----| true -> return
 
 #   29| elements
 #-----|  -> nil?
@@ -345,7 +345,7 @@ break_ensure.rb:
 #   29| nil?
 #-----|  -> call to nil?
 
-#   30| Return
+#   30| return
 #-----| return -> [ensure: return] Ensure
 
 #   32| Ensure
@@ -388,11 +388,11 @@ break_ensure.rb:
 
 #   35| ... > ...
 #-----| false -> if ...
-#-----| true -> Break
+#-----| true -> break
 
 #   35| [ensure: return] ... > ...
 #-----| false -> [ensure: return] if ...
-#-----| true -> [ensure: return] Break
+#-----| true -> [ensure: return] break
 
 #   35| call to x
 #-----|  -> 0
@@ -406,10 +406,10 @@ break_ensure.rb:
 #   35| [ensure: return] 0
 #-----|  -> [ensure: return] ... > ...
 
-#   36| Break
+#   36| break
 #-----| break -> for ... in ...
 
-#   36| [ensure: return] Break
+#   36| [ensure: return] break
 #-----| break -> [ensure: return] for ... in ...
 
 #   41| call to puts
@@ -497,17 +497,17 @@ break_ensure.rb:
 #   51| [ensure: raise] 0
 #-----|  -> [ensure: raise] ... > ...
 
-#   52| Break
+#   52| break
 #-----| break -> for ... in ...
 
-#   52| [ensure: raise] Break
+#   52| [ensure: raise] break
 #-----| break -> for ... in ...
 
 #   52| 10
-#-----|  -> Break
+#-----|  -> break
 
 #   52| [ensure: raise] 10
-#-----|  -> [ensure: raise] Break
+#-----|  -> [ensure: raise] break
 
 case.rb:
 #    1| if_in_case
@@ -727,11 +727,11 @@ cfg.rb:
 #   31| true
 #-----| true -> 1
 
-#   32| Break
+#   32| break
 #-----| break -> while ...
 
 #   32| 1
-#-----|  -> Break
+#-----|  -> break
 
 #   35| if ...
 #-----|  -> self
@@ -1172,7 +1172,7 @@ cfg.rb:
 
 #   91| ... > ...
 #-----| false -> if ...
-#-----| true -> Next
+#-----| true -> next
 
 #   91| x
 #-----|  -> 3
@@ -1180,7 +1180,7 @@ cfg.rb:
 #   91| 3
 #-----|  -> ... > ...
 
-#   91| Next
+#   91| next
 #-----| next -> In
 
 #   92| call to puts
@@ -1295,11 +1295,11 @@ cfg.rb:
 #  102| value
 #-----|  -> call to puts
 
-#  103| Return
+#  103| return
 #-----| return -> exit parameters (normal)
 
 #  103| ElementReference
-#-----|  -> Return
+#-----|  -> return
 
 #  103| kwargs
 #-----|  -> key
@@ -2288,17 +2288,17 @@ ifs.rb:
 #-----| false -> if ...
 #-----| true -> 0
 
-#   13| Return
+#   13| return
 #-----| return -> exit m2 (normal)
 
 #   13| 0
-#-----|  -> Return
+#-----|  -> return
 
-#   15| Return
+#   15| return
 #-----| return -> exit m2 (normal)
 
 #   15| 1
-#-----|  -> Return
+#-----|  -> return
 
 #   18| m3
 #-----|  -> m4
@@ -2386,11 +2386,11 @@ ifs.rb:
 #   28| b3
 #-----|  -> b1
 
-#   29| Return
+#   29| return
 #-----| return -> exit m4 (normal)
 
 #   29| ... ? ... : ...
-#-----|  -> Return
+#-----|  -> return
 
 #   29| [false] (... ? ... : ...)
 #-----| false -> !b2 || !b3
@@ -2618,7 +2618,7 @@ loops.rb:
 #-----|  -> puts
 
 #   12| ... > ...
-#-----| true -> Break
+#-----| true -> break
 #-----| false -> x
 
 #   12| x
@@ -2627,14 +2627,14 @@ loops.rb:
 #   12| 100
 #-----|  -> ... > ...
 
-#   13| Break
+#   13| break
 #-----| break -> while ...
 
 #   14| elsif ...
 #-----|  -> if ...
 
 #   14| ... > ...
-#-----| true -> Next
+#-----| true -> next
 #-----| false -> x
 
 #   14| x
@@ -2643,7 +2643,7 @@ loops.rb:
 #   14| 50
 #-----|  -> ... > ...
 
-#   15| Next
+#   15| next
 #-----| next -> x
 
 #   16| elsif ...
@@ -3070,11 +3070,11 @@ raise.rb:
 #   71| 0
 #-----|  -> ... < ...
 
-#   72| Return
+#   72| return
 #-----| return -> [ensure: return] Ensure
 
 #   72| x < 0
-#-----|  -> Return
+#-----|  -> return
 
 #   74| call to puts
 #-----|  -> Ensure
@@ -3174,11 +3174,11 @@ raise.rb:
 #   84| 0
 #-----|  -> ... < ...
 
-#   85| Return
+#   85| return
 #-----| return -> [ensure: return] Ensure
 
 #   85| x < 0
-#-----|  -> Return
+#-----|  -> return
 
 #   87| call to puts
 #-----|  -> Ensure
@@ -3293,11 +3293,11 @@ raise.rb:
 #   99| 0
 #-----|  -> ... < ...
 
-#  100| Return
+#  100| return
 #-----| return -> [ensure: return] Ensure
 
 #  100| x < 0
-#-----|  -> Return
+#-----|  -> return
 
 #  102| call to puts
 #-----|  -> Ensure
@@ -3700,17 +3700,17 @@ raise.rb:
 #  146| [ensure: raise] Ensure
 #-----|  -> [ensure: raise] 3
 
-#  147| Return
+#  147| return
 #-----| return -> exit m12 (normal)
 
-#  147| [ensure: raise] Return
+#  147| [ensure: raise] return
 #-----| return -> exit m12 (normal)
 
 #  147| 3
-#-----|  -> Return
+#-----|  -> return
 
 #  147| [ensure: raise] 3
-#-----|  -> [ensure: raise] Return
+#-----|  -> [ensure: raise] return
 
 #  150| m13
 #-----|  -> m14

ql/test/library-tests/dataflow/local/DataflowStep.expected

@@ -17,3 +17,17 @@
 | local_dataflow.rb:5:12:5:12 | a | local_dataflow.rb:6:8:6:8 | a |
 | local_dataflow.rb:6:7:6:14 | (... += ...) | local_dataflow.rb:6:3:6:14 | ... = ... |
 | local_dataflow.rb:6:8:6:13 | ... += ... | local_dataflow.rb:6:7:6:14 | (... += ...) |
+| local_dataflow.rb:9:1:9:15 | ... = ... | local_dataflow.rb:10:14:10:18 | array |
+| local_dataflow.rb:10:5:13:3 | for ... in ... | local_dataflow.rb:10:1:13:3 | ... = ... |
+| local_dataflow.rb:10:9:10:9 | x | local_dataflow.rb:12:5:12:5 | x |
+| local_dataflow.rb:10:14:10:18 | array | local_dataflow.rb:10:5:13:3 | for ... in ... |
+| local_dataflow.rb:10:14:10:18 | array | local_dataflow.rb:15:10:15:14 | array |
+| local_dataflow.rb:15:10:15:14 | array | local_dataflow.rb:15:1:17:3 | for ... in ... |
+| local_dataflow.rb:15:10:15:14 | array | local_dataflow.rb:19:10:19:14 | array |
+| local_dataflow.rb:16:3:16:10 | break | local_dataflow.rb:15:1:17:3 | for ... in ... |
+| local_dataflow.rb:16:9:16:10 | 10 | local_dataflow.rb:16:3:16:10 | break |
+| local_dataflow.rb:19:5:19:5 | x | local_dataflow.rb:20:6:20:6 | x |
+| local_dataflow.rb:19:10:19:14 | array | local_dataflow.rb:19:1:21:3 | for ... in ... |
+| local_dataflow.rb:20:17:20:21 | break | local_dataflow.rb:19:1:21:3 | for ... in ... |
+| local_dataflow.rb:24:2:24:8 | break | local_dataflow.rb:23:1:25:3 | while ... |
+| local_dataflow.rb:24:8:24:8 | 5 | local_dataflow.rb:24:2:24:8 | break |

ql/test/library-tests/dataflow/local/local_dataflow.rb

@@ -5,3 +5,21 @@ def foo(a)
   d = (c = a)
   e = (a += b)
 end
+
+array = [1,2,3]
+y = for x in array
+do
+  p x
+end
+
+for x in array do
+  break 10
+end
+
+for x in array do
+  if x > 1 then break end
+end
+
+while true
+ break 5
+end