diff --git a/ql/src/codeql_ruby/controlflow/CfgNodes.qll b/ql/src/codeql_ruby/controlflow/CfgNodes.qll index 04e1799410d..0568291669b 100644 --- a/ql/src/codeql_ruby/controlflow/CfgNodes.qll +++ b/ql/src/codeql_ruby/controlflow/CfgNodes.qll @@ -106,6 +106,19 @@ class ExprCfgNode extends AstCfgNode { Expr getExpr() { result = e } } +/** A control-flow node that wraps a return-like statement. */ +class ReturningCfgNode extends AstCfgNode { + ReturningStatement 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. */ @@ -288,6 +301,20 @@ module ExprNodes { final ExprCfgNode getExpr(int n) { e.hasCfgChild(e.getExpr(n), this, result) } } + private class ForExprChildMapping extends ExprChildMapping, ForExpr { + override predicate relevantChild(Expr e) { e = this.getValue() } + } + + /** A control-flow node that wraps an `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 an `ParenthesizedExpr` AST expression. */ class ParenthesizedExprCfgNode extends ExprSequenceCfgNode { ParenthesizedExprCfgNode() { this.getExpr() instanceof ParenthesizedExpr } diff --git a/ql/src/codeql_ruby/dataflow/internal/DataFlowDispatch.qll b/ql/src/codeql_ruby/dataflow/internal/DataFlowDispatch.qll index 447fab40e50..b412793ffaa 100644 --- a/ql/src/codeql_ruby/dataflow/internal/DataFlowDispatch.qll +++ b/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 { diff --git a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll index 31deccb48db..068951ab0eb 100644 --- a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll +++ b/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 } @@ -130,6 +131,22 @@ private module Cached { 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 + | + exprTo.getNode() instanceof Loop and + nodeTo.asExpr().getAPredecessor(any(SuccessorTypes::BreakSuccessor s)) = n.getExprNode() + ) + or + nodeFrom.asExpr() = nodeTo.(ExprReturnNode).getExprNode().getReturnedValueNode() + or + exists(CfgNodes::ExprNodes::ForExprCfgNode for | for = nodeTo.asExpr() | + exists(SuccessorType s, CfgNode n | not s instanceof SuccessorTypes::BreakSuccessor | + for.getAPredecessor(s) = n + ) and + nodeFrom.asExpr() = for.getValue() + ) } cached @@ -225,12 +242,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() } } } diff --git a/ql/test/library-tests/dataflow/local/DataflowStep.expected b/ql/test/library-tests/dataflow/local/DataflowStep.expected index b920f8a2876..e639acbe7fe 100644 --- a/ql/test/library-tests/dataflow/local/DataflowStep.expected +++ b/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 10 | 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 10 | +| 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 5 | local_dataflow.rb:23:1:25:3 | while ... | +| local_dataflow.rb:24:8:24:8 | 5 | local_dataflow.rb:24:2:24:8 | break 5 | diff --git a/ql/test/library-tests/dataflow/local/local_dataflow.rb b/ql/test/library-tests/dataflow/local/local_dataflow.rb index 5ba6d140bf2..b212fa9ce16 100644 --- a/ql/test/library-tests/dataflow/local/local_dataflow.rb +++ b/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