Merge pull request #121 from github/aibaars/dataflow-2

Dataflow: identify ReturnNodes

Author: aibaars

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

@@ -132,15 +132,15 @@ private module Cached {
     or
     nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::CaseExprCfgNode).getBranch(_)
     or
-    exists(CfgNodes::ExprCfgNode exprTo, ExprReturnNode n |
+    exists(CfgNodes::ExprCfgNode exprTo, ReturningStatementNode n |
       nodeFrom = n and
       exprTo = nodeTo.asExpr() and
-      n.getKind() instanceof BreakReturnKind and
+      n.getReturningNode().getNode() instanceof BreakStmt and
       exprTo.getNode() instanceof Loop and
-      nodeTo.asExpr().getAPredecessor(any(SuccessorTypes::BreakSuccessor s)) = n.getExprNode()
+      nodeTo.asExpr().getAPredecessor(any(SuccessorTypes::BreakSuccessor s)) = n.getReturningNode()
     )
     or
-    nodeFrom.asExpr() = nodeTo.(ExprReturnNode).getExprNode().getReturnedValueNode()
+    nodeFrom.asExpr() = nodeTo.(ReturningStatementNode).getReturningNode().getReturnedValueNode()
     or
     nodeTo.asExpr() =
       any(CfgNodes::ExprNodes::ForExprCfgNode for |
@@ -182,6 +182,28 @@ class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode {
   override string toStringImpl() { result = def.toString() }
 }
 
+/**
+ * A value returning statement, viewed as a node in a data flow graph.
+ *
+ * Note that because of control-flow splitting, one `ReturningStmt` may correspond
+ * to multiple `ReturningStatementNode`s, just like it may correspond to multiple
+ * `ControlFlow::Node`s.
+ */
+class ReturningStatementNode extends NodeImpl, TReturningNode {
+  private CfgNodes::ReturningCfgNode n;
+
+  ReturningStatementNode() { this = TReturningNode(n) }
+
+  /** Gets the expression corresponding to this node. */
+  CfgNodes::ReturningCfgNode getReturningNode() { result = n }
+
+  override CfgScope getCfgScope() { result = n.getScope() }
+
+  override Location getLocationImpl() { result = n.getLocation() }
+
+  override string toStringImpl() { result = n.toString() }
+}
+
 private module ParameterNodes {
   abstract private class ParameterNodeImpl extends ParameterNode, NodeImpl { }
 
@@ -241,29 +263,49 @@ abstract class ReturnNode extends Node {
 }
 
 private module ReturnNodes {
+  private predicate isValid(CfgNodes::ReturningCfgNode node) {
+    exists(ReturningStmt stmt, Callable scope |
+      stmt = node.getNode() and
+      scope = node.getScope()
+    |
+      stmt instanceof ReturnStmt and
+      (scope instanceof Method or scope instanceof SingletonMethod or scope instanceof Lambda)
+      or
+      stmt instanceof NextStmt and
+      (scope instanceof Block or scope instanceof Lambda)
+      or
+      stmt instanceof BreakStmt and
+      (scope instanceof Block or scope instanceof Lambda)
+    )
+  }
+
   /**
    * 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, NodeImpl, TReturningNode {
+  class ExplicitReturnNode extends ReturnNode, ReturningStatementNode {
     private CfgNodes::ReturningCfgNode n;
 
-    ExprReturnNode() { this = TReturningNode(n) }
-
-    /** Gets the statement corresponding to this node. */
-    CfgNodes::ReturningCfgNode getExprNode() { result = n }
+    ExplicitReturnNode() {
+      isValid(this.getReturningNode()) and
+      n.getASuccessor().(CfgNodes::AnnotatedExitNode).isNormal() and
+      n.getScope() instanceof Callable
+    }
 
     override ReturnKind getKind() {
       if n.getNode() instanceof BreakStmt
       then result instanceof BreakReturnKind
       else result instanceof NormalReturnKind
     }
+  }
 
-    override CfgScope getCfgScope() { result = n.getScope() }
-
-    override Location getLocationImpl() { result = n.getLocation() }
+  class ExprReturnNode extends ReturnNode, ExprNode {
+    ExprReturnNode() {
+      this.getExprNode().getASuccessor().(CfgNodes::AnnotatedExitNode).isNormal() and
+      this.getEnclosingCallable() instanceof Callable
+    }
 
-    override string toStringImpl() { result = n.toString() }
+    override ReturnKind getKind() { result instanceof NormalReturnKind }
   }
 }
 

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

@@ -31,3 +31,18 @@
 | 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 |
+| local_dataflow.rb:28:5:28:26 | M | local_dataflow.rb:28:1:28:26 | ... = ... |
+| local_dataflow.rb:28:15:28:22 | module | local_dataflow.rb:28:5:28:26 | M |
+| local_dataflow.rb:30:5:30:24 | C | local_dataflow.rb:30:1:30:24 | ... = ... |
+| local_dataflow.rb:30:14:30:20 | class | local_dataflow.rb:30:5:30:24 | C |
+| local_dataflow.rb:32:5:32:25 | bar | local_dataflow.rb:32:1:32:25 | ... = ... |
+| local_dataflow.rb:32:5:32:25 | bar | local_dataflow.rb:32:1:32:25 | ... = ... |
+| local_dataflow.rb:34:7:34:7 | x | local_dataflow.rb:35:6:35:6 | x |
+| local_dataflow.rb:36:13:36:13 | 7 | local_dataflow.rb:36:6:36:13 | return |
+| local_dataflow.rb:41:7:41:7 | x | local_dataflow.rb:42:6:42:6 | x |
+| local_dataflow.rb:43:13:43:13 | 7 | local_dataflow.rb:43:6:43:13 | return |
+| local_dataflow.rb:45:10:45:10 | 6 | local_dataflow.rb:45:3:45:10 | return |
+| local_dataflow.rb:49:3:53:3 | <captured> | local_dataflow.rb:50:18:50:18 | x |
+| local_dataflow.rb:50:8:50:13 | next | local_dataflow.rb:50:3:50:13 | next |
+| local_dataflow.rb:50:18:50:18 | x | local_dataflow.rb:51:20:51:20 | x |
+| local_dataflow.rb:51:9:51:15 | break | local_dataflow.rb:51:3:51:15 | break |

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

@@ -0,0 +1,9 @@
+| local_dataflow.rb:6:3:6:14 | ... = ... |
+| local_dataflow.rb:32:14:32:21 | method |
+| local_dataflow.rb:36:6:36:13 | return |
+| local_dataflow.rb:38:3:38:13 | reachable |
+| local_dataflow.rb:43:6:43:13 | return |
+| local_dataflow.rb:45:3:45:10 | return |
+| local_dataflow.rb:50:3:50:13 | next |
+| local_dataflow.rb:51:3:51:15 | break |
+| local_dataflow.rb:52:3:52:10 | normal |

ql/test/library-tests/dataflow/local/ReturnNodes.ql

@@ -0,0 +1,4 @@
+import ruby
+import codeql_ruby.dataflow.internal.DataFlowPrivate
+
+select any(ReturnNode node)

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

@@ -23,3 +23,31 @@ def foo(a)
 while true
  break 5
 end
+
+# string flows to x
+x = module M; "module" end
+# string flows to x
+x = class C; "class" end
+# string does not flow to x because "def" evaluates to a method symbol
+x = def bar; "method" end
+
+def m x 
+  if x == 4
+     return 7
+  end
+  "reachable"
+end
+
+def m x 
+  if x == 4
+     return 7
+  end
+  return 6
+  "unreachable"
+end
+
+m do
+  next "next" if x < 4 
+  break "break" if x < 9
+  "normal"
+end