From c991d550cdc73043170adfb88e151223b6bec84e Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Mon, 8 Feb 2021 18:58:09 +0100 Subject: [PATCH 1/8] AST: add Statement and ReturningStatement --- ql/src/codeql_ruby/AST.qll | 1 + ql/src/codeql_ruby/ast/Statement.qll | 97 +++++++++++++++++++ ql/src/codeql_ruby/ast/internal/Statement.qll | 44 +++++++++ 3 files changed, 142 insertions(+) create mode 100644 ql/src/codeql_ruby/ast/Statement.qll create mode 100644 ql/src/codeql_ruby/ast/internal/Statement.qll diff --git a/ql/src/codeql_ruby/AST.qll b/ql/src/codeql_ruby/AST.qll index 5131e6fd027..c914be66882 100644 --- a/ql/src/codeql_ruby/AST.qll +++ b/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.TreeSitter diff --git a/ql/src/codeql_ruby/ast/Statement.qll b/ql/src/codeql_ruby/ast/Statement.qll new file mode 100644 index 00000000000..28ae2f933e3 --- /dev/null +++ b/ql/src/codeql_ruby/ast/Statement.qll @@ -0,0 +1,97 @@ +private import codeql_ruby.AST +private import codeql_ruby.CFG +private import internal.Statement +private import codeql_ruby.controlflow.internal.ControlFlowGraphImpl +private import codeql_ruby.ast.internal.TreeSitter + +/** + * A statement. + * + * This is the root QL class for all statements. + */ +class Statement extends AstNode { + Statement::Range range; + + Statement() { this = 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 ReturningStatement extends Statement { + override ReturningStatement::Range range; + + final override string toString() { + not exists(getValue()) and result = range.getStatementName() + or + result = range.getStatementName() + " " + getValue().toString() + } + + /** Gets the returned value, if any. */ + final Expr getValue() { + exists(Generated::ArgumentList a, int c | + a = range.getArgumentList() and c = count(a.getChild(_)) + | + result = a.getChild(0) and c = 1 + or + result = a and c > 1 + ) + } +} + +/** + * A `return` statement. + * ```rb + * return + * return value + * ``` + */ +class ReturnStmt extends ReturningStatement, @return { + final override ReturnStmt::Range range; + + final override string getAPrimaryQlClass() { result = "ReturnStmt" } +} + +/** + * A `break` statement. + * ```rb + * break + * break value + * ``` + */ +class BreakStmt extends ReturningStatement, @break { + final override BreakStmt::Range range; + + final override string getAPrimaryQlClass() { result = "BreakStmt" } +} + +/** + * A `next` statement. + * ```rb + * next + * next value + * ``` + */ +class NextStmt extends ReturningStatement, @next { + final override NextStmt::Range range; + + final override string getAPrimaryQlClass() { result = "NextStmt" } +} diff --git a/ql/src/codeql_ruby/ast/internal/Statement.qll b/ql/src/codeql_ruby/ast/internal/Statement.qll new file mode 100644 index 00000000000..72da586ebbb --- /dev/null +++ b/ql/src/codeql_ruby/ast/internal/Statement.qll @@ -0,0 +1,44 @@ +private import codeql_ruby.AST +private import codeql_ruby.ast.internal.TreeSitter + +module Statement { + abstract class Range extends AstNode { } +} + +module ReturningStatement { + abstract class Range extends Statement::Range { + abstract Generated::ArgumentList getArgumentList(); + + abstract string getStatementName(); + } +} + +module ReturnStmt { + class Range extends ReturningStatement::Range, @return { + final override Generated::Return generated; + + final override string getStatementName() { result = "return" } + + final override Generated::ArgumentList getArgumentList() { result = generated.getChild() } + } +} + +module BreakStmt { + class Range extends ReturningStatement::Range, @break { + final override Generated::Break generated; + + final override string getStatementName() { result = "break" } + + final override Generated::ArgumentList getArgumentList() { result = generated.getChild() } + } +} + +module NextStmt { + class Range extends ReturningStatement::Range, @next { + final override Generated::Next generated; + + final override string getStatementName() { result = "next" } + + final override Generated::ArgumentList getArgumentList() { result = generated.getChild() } + } +} From adb88df638b1a2d82ef87cd533718b9b13ed8b40 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Thu, 4 Feb 2021 19:09:45 +0100 Subject: [PATCH 2/8] Add flow steps for conditional and case expressions --- ql/src/codeql_ruby/ast/Control.qll | 2 +- ql/src/codeql_ruby/controlflow/CfgNodes.qll | 41 ++++++++++++++++++- .../dataflow/internal/DataFlowPrivate.qll | 7 +++- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Control.qll b/ql/src/codeql_ruby/ast/Control.qll index f41b711a8f7..ebf408e72fc 100644 --- a/ql/src/codeql_ruby/ast/Control.qll +++ b/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; /** diff --git a/ql/src/codeql_ruby/controlflow/CfgNodes.qll b/ql/src/codeql_ruby/controlflow/CfgNodes.qll index c08df48c8a6..04e1799410d 100644 --- a/ql/src/codeql_ruby/controlflow/CfgNodes.qll +++ b/ql/src/codeql_ruby/controlflow/CfgNodes.qll @@ -232,6 +232,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 an `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 i) { e.hasCfgChild(e.getBranch(i), 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 an `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() } } @@ -249,7 +288,7 @@ module ExprNodes { final ExprCfgNode getExpr(int n) { e.hasCfgChild(e.getExpr(n), this, result) } } - /** A control-flow node that wraps an `ExprSequence` AST expression. */ + /** 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/DataFlowPrivate.qll b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll index bbfb4019c33..31deccb48db 100644 --- a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll @@ -125,8 +125,11 @@ 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(_) } cached From a752491c5f1273c543f9bf25d11343fea1711875 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Mon, 8 Feb 2021 09:43:37 +0100 Subject: [PATCH 3/8] Add flow steps for loop 'return' values --- ql/src/codeql_ruby/controlflow/CfgNodes.qll | 27 +++++++++++++ .../dataflow/internal/DataFlowDispatch.qll | 11 +++++- .../dataflow/internal/DataFlowPrivate.qll | 38 +++++++++++++++++-- .../dataflow/local/DataflowStep.expected | 14 +++++++ .../dataflow/local/local_dataflow.rb | 18 +++++++++ 5 files changed, 103 insertions(+), 5 deletions(-) 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 From bb89e134c48cabd9a9b963c8f06a27ca815502f8 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Tue, 9 Feb 2021 13:54:46 +0100 Subject: [PATCH 4/8] Address comments --- ql/src/codeql_ruby/ast/Statement.qll | 6 +- ql/src/codeql_ruby/ast/internal/Statement.qll | 8 +- ql/src/codeql_ruby/controlflow/CfgNodes.qll | 12 +-- .../dataflow/internal/DataFlowPrivate.qll | 19 +++-- .../controlflow/graph/Cfg.expected | 80 +++++++++---------- .../dataflow/local/DataflowStep.expected | 8 +- 6 files changed, 65 insertions(+), 68 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Statement.qll b/ql/src/codeql_ruby/ast/Statement.qll index 28ae2f933e3..8cc65fd7d8a 100644 --- a/ql/src/codeql_ruby/ast/Statement.qll +++ b/ql/src/codeql_ruby/ast/Statement.qll @@ -39,11 +39,7 @@ class Statement extends AstNode { class ReturningStatement extends Statement { override ReturningStatement::Range range; - final override string toString() { - not exists(getValue()) and result = range.getStatementName() - or - result = range.getStatementName() + " " + getValue().toString() - } + final override string toString() { result = range.toString() } /** Gets the returned value, if any. */ final Expr getValue() { diff --git a/ql/src/codeql_ruby/ast/internal/Statement.qll b/ql/src/codeql_ruby/ast/internal/Statement.qll index 72da586ebbb..d09b5027023 100644 --- a/ql/src/codeql_ruby/ast/internal/Statement.qll +++ b/ql/src/codeql_ruby/ast/internal/Statement.qll @@ -8,8 +8,6 @@ module Statement { module ReturningStatement { abstract class Range extends Statement::Range { abstract Generated::ArgumentList getArgumentList(); - - abstract string getStatementName(); } } @@ -17,7 +15,7 @@ module ReturnStmt { class Range extends ReturningStatement::Range, @return { final override Generated::Return generated; - final override string getStatementName() { result = "return" } + final override string toString() { result = "return" } final override Generated::ArgumentList getArgumentList() { result = generated.getChild() } } @@ -27,7 +25,7 @@ module BreakStmt { class Range extends ReturningStatement::Range, @break { final override Generated::Break generated; - final override string getStatementName() { result = "break" } + final override string toString() { result = "break" } final override Generated::ArgumentList getArgumentList() { result = generated.getChild() } } @@ -37,7 +35,7 @@ module NextStmt { class Range extends ReturningStatement::Range, @next { final override Generated::Next generated; - final override string getStatementName() { result = "next" } + final override string toString() { result = "next" } final override Generated::ArgumentList getArgumentList() { result = generated.getChild() } } diff --git a/ql/src/codeql_ruby/controlflow/CfgNodes.qll b/ql/src/codeql_ruby/controlflow/CfgNodes.qll index 0568291669b..c0296624b3a 100644 --- a/ql/src/codeql_ruby/controlflow/CfgNodes.qll +++ b/ql/src/codeql_ruby/controlflow/CfgNodes.qll @@ -249,7 +249,7 @@ module ExprNodes { override predicate relevantChild(Expr e) { e = this.getValue() or e = this.getBranch(_) } } - /** A control-flow node that wraps an `CaseExpr` AST expression. */ + /** A control-flow node that wraps a `CaseExpr` AST expression. */ class CaseExprCfgNode extends ExprCfgNode { override CaseExprChildMapping e; @@ -259,16 +259,16 @@ module ExprNodes { final ExprCfgNode getValue() { e.hasCfgChild(e.getValue(), this, result) } /** - * Gets the `n`th branch of this case expression, + * Gets the `n`th branch of this case expression. */ - final ExprCfgNode getBranch(int i) { e.hasCfgChild(e.getBranch(i), this, result) } + 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 an `ConditionalExpr` AST expression. */ + /** A control-flow node that wraps a `ConditionalExpr` AST expression. */ class ConditionalExprCfgNode extends ExprCfgNode { override ConditionalExprChildMapping e; @@ -305,7 +305,7 @@ module ExprNodes { override predicate relevantChild(Expr e) { e = this.getValue() } } - /** A control-flow node that wraps an `ForExpr` AST expression. */ + /** A control-flow node that wraps a `ForExpr` AST expression. */ class ForExprCfgNode extends ExprCfgNode { override ForExprChildMapping e; @@ -315,7 +315,7 @@ module ExprNodes { final ExprCfgNode getValue() { e.hasCfgChild(e.getValue(), this, result) } } - /** A control-flow node that wraps an `ParenthesizedExpr` AST expression. */ + /** A control-flow node that wraps a `ParenthesizedExpr` AST expression. */ class ParenthesizedExprCfgNode extends ExprSequenceCfgNode { ParenthesizedExprCfgNode() { this.getExpr() instanceof ParenthesizedExpr } } diff --git a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll index 068951ab0eb..4ac583160ff 100644 --- a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll @@ -133,20 +133,23 @@ private module Cached { 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 - | + 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 - 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() - ) + 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 diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected index dde6226e833..d9bb072e5d0 100644 --- a/ql/test/library-tests/controlflow/graph/Cfg.expected +++ b/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 diff --git a/ql/test/library-tests/dataflow/local/DataflowStep.expected b/ql/test/library-tests/dataflow/local/DataflowStep.expected index e639acbe7fe..d877f7a867b 100644 --- a/ql/test/library-tests/dataflow/local/DataflowStep.expected +++ b/ql/test/library-tests/dataflow/local/DataflowStep.expected @@ -24,10 +24,10 @@ | 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: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 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 | +| 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 | From e398837bdca1cc5d51dc36ea45e7f1229039c495 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Tue, 9 Feb 2021 13:55:06 +0100 Subject: [PATCH 5/8] Rename Statement to Stmt --- ql/src/codeql_ruby/ast/Statement.qll | 16 ++++++++-------- ql/src/codeql_ruby/ast/internal/Statement.qll | 12 ++++++------ ql/src/codeql_ruby/controlflow/CfgNodes.qll | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Statement.qll b/ql/src/codeql_ruby/ast/Statement.qll index 8cc65fd7d8a..79f8dc935d5 100644 --- a/ql/src/codeql_ruby/ast/Statement.qll +++ b/ql/src/codeql_ruby/ast/Statement.qll @@ -9,10 +9,10 @@ private import codeql_ruby.ast.internal.TreeSitter * * This is the root QL class for all statements. */ -class Statement extends AstNode { - Statement::Range range; +class Stmt extends AstNode { + Stmt::Range range; - Statement() { this = range } + Stmt() { this = range } /** Gets a control-flow node for this statement, if any. */ CfgNodes::AstCfgNode getAControlFlowNode() { result.getNode() = this } @@ -36,8 +36,8 @@ class Statement extends AstNode { * next value * ``` */ -class ReturningStatement extends Statement { - override ReturningStatement::Range range; +class ReturningStmt extends Stmt { + override ReturningStmt::Range range; final override string toString() { result = range.toString() } @@ -60,7 +60,7 @@ class ReturningStatement extends Statement { * return value * ``` */ -class ReturnStmt extends ReturningStatement, @return { +class ReturnStmt extends ReturningStmt, @return { final override ReturnStmt::Range range; final override string getAPrimaryQlClass() { result = "ReturnStmt" } @@ -73,7 +73,7 @@ class ReturnStmt extends ReturningStatement, @return { * break value * ``` */ -class BreakStmt extends ReturningStatement, @break { +class BreakStmt extends ReturningStmt, @break { final override BreakStmt::Range range; final override string getAPrimaryQlClass() { result = "BreakStmt" } @@ -86,7 +86,7 @@ class BreakStmt extends ReturningStatement, @break { * next value * ``` */ -class NextStmt extends ReturningStatement, @next { +class NextStmt extends ReturningStmt, @next { final override NextStmt::Range range; final override string getAPrimaryQlClass() { result = "NextStmt" } diff --git a/ql/src/codeql_ruby/ast/internal/Statement.qll b/ql/src/codeql_ruby/ast/internal/Statement.qll index d09b5027023..3d61261e37e 100644 --- a/ql/src/codeql_ruby/ast/internal/Statement.qll +++ b/ql/src/codeql_ruby/ast/internal/Statement.qll @@ -1,18 +1,18 @@ private import codeql_ruby.AST private import codeql_ruby.ast.internal.TreeSitter -module Statement { +module Stmt { abstract class Range extends AstNode { } } -module ReturningStatement { - abstract class Range extends Statement::Range { +module ReturningStmt { + abstract class Range extends Stmt::Range { abstract Generated::ArgumentList getArgumentList(); } } module ReturnStmt { - class Range extends ReturningStatement::Range, @return { + class Range extends ReturningStmt::Range, @return { final override Generated::Return generated; final override string toString() { result = "return" } @@ -22,7 +22,7 @@ module ReturnStmt { } module BreakStmt { - class Range extends ReturningStatement::Range, @break { + class Range extends ReturningStmt::Range, @break { final override Generated::Break generated; final override string toString() { result = "break" } @@ -32,7 +32,7 @@ module BreakStmt { } module NextStmt { - class Range extends ReturningStatement::Range, @next { + class Range extends ReturningStmt::Range, @next { final override Generated::Next generated; final override string toString() { result = "next" } diff --git a/ql/src/codeql_ruby/controlflow/CfgNodes.qll b/ql/src/codeql_ruby/controlflow/CfgNodes.qll index c0296624b3a..4cce51e6f84 100644 --- a/ql/src/codeql_ruby/controlflow/CfgNodes.qll +++ b/ql/src/codeql_ruby/controlflow/CfgNodes.qll @@ -108,7 +108,7 @@ class ExprCfgNode extends AstCfgNode { /** A control-flow node that wraps a return-like statement. */ class ReturningCfgNode extends AstCfgNode { - ReturningStatement s; + ReturningStmt s; ReturningCfgNode() { s = this.getNode() } From daa7bd7fd4ae498a6aaefbd6816c783970619ff4 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Tue, 9 Feb 2021 14:01:08 +0100 Subject: [PATCH 6/8] Move ReturningStmt::getValue implementation to internal library --- ql/src/codeql_ruby/ast/Statement.qll | 11 +---------- ql/src/codeql_ruby/ast/internal/Statement.qll | 10 ++++++++++ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Statement.qll b/ql/src/codeql_ruby/ast/Statement.qll index 79f8dc935d5..edc62cf0627 100644 --- a/ql/src/codeql_ruby/ast/Statement.qll +++ b/ql/src/codeql_ruby/ast/Statement.qll @@ -2,7 +2,6 @@ private import codeql_ruby.AST private import codeql_ruby.CFG private import internal.Statement private import codeql_ruby.controlflow.internal.ControlFlowGraphImpl -private import codeql_ruby.ast.internal.TreeSitter /** * A statement. @@ -42,15 +41,7 @@ class ReturningStmt extends Stmt { final override string toString() { result = range.toString() } /** Gets the returned value, if any. */ - final Expr getValue() { - exists(Generated::ArgumentList a, int c | - a = range.getArgumentList() and c = count(a.getChild(_)) - | - result = a.getChild(0) and c = 1 - or - result = a and c > 1 - ) - } + final Expr getValue() { result = range.getValue() } } /** diff --git a/ql/src/codeql_ruby/ast/internal/Statement.qll b/ql/src/codeql_ruby/ast/internal/Statement.qll index 3d61261e37e..2a3a7ce1ef4 100644 --- a/ql/src/codeql_ruby/ast/internal/Statement.qll +++ b/ql/src/codeql_ruby/ast/internal/Statement.qll @@ -8,6 +8,16 @@ module Stmt { 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 + ) + } } } From 1e64b264ba7beaab16450212b0a25fd0111182e7 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Tue, 9 Feb 2021 18:50:30 +0100 Subject: [PATCH 7/8] Fix compilation errors after merge --- ql/src/codeql_ruby/ast/Statement.qll | 4 +--- ql/src/codeql_ruby/ast/internal/AST.qll | 6 ------ ql/src/codeql_ruby/ast/internal/Statement.qll | 3 ++- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Statement.qll b/ql/src/codeql_ruby/ast/Statement.qll index edc62cf0627..7557799e712 100644 --- a/ql/src/codeql_ruby/ast/Statement.qll +++ b/ql/src/codeql_ruby/ast/Statement.qll @@ -9,7 +9,7 @@ private import codeql_ruby.controlflow.internal.ControlFlowGraphImpl * This is the root QL class for all statements. */ class Stmt extends AstNode { - Stmt::Range range; + override Stmt::Range range; Stmt() { this = range } @@ -38,8 +38,6 @@ class Stmt extends AstNode { class ReturningStmt extends Stmt { override ReturningStmt::Range range; - final override string toString() { result = range.toString() } - /** Gets the returned value, if any. */ final Expr getValue() { result = range.getValue() } } diff --git a/ql/src/codeql_ruby/ast/internal/AST.qll b/ql/src/codeql_ruby/ast/internal/AST.qll index 4073265a4d2..c8ef3f886c5 100644 --- a/ql/src/codeql_ruby/ast/internal/AST.qll +++ b/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 diff --git a/ql/src/codeql_ruby/ast/internal/Statement.qll b/ql/src/codeql_ruby/ast/internal/Statement.qll index 2a3a7ce1ef4..b4fa88ce159 100644 --- a/ql/src/codeql_ruby/ast/internal/Statement.qll +++ b/ql/src/codeql_ruby/ast/internal/Statement.qll @@ -1,8 +1,9 @@ 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 { } + abstract class Range extends AstNode::Range { } } module ReturningStmt { From 9cfc08319d1632d0a2f281a924494b3770cd931a Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 9 Feb 2021 19:32:41 +0100 Subject: [PATCH 8/8] Use `Generated::AstNode` in `ExprChildMapping` --- ql/src/codeql_ruby/ast/Statement.qll | 2 -- ql/src/codeql_ruby/controlflow/CfgNodes.qll | 6 ++++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Statement.qll b/ql/src/codeql_ruby/ast/Statement.qll index 7557799e712..b4d7ff892db 100644 --- a/ql/src/codeql_ruby/ast/Statement.qll +++ b/ql/src/codeql_ruby/ast/Statement.qll @@ -11,8 +11,6 @@ private import codeql_ruby.controlflow.internal.ControlFlowGraphImpl class Stmt extends AstNode { override Stmt::Range range; - Stmt() { this = range } - /** Gets a control-flow node for this statement, if any. */ CfgNodes::AstCfgNode getAControlFlowNode() { result.getNode() = this } diff --git a/ql/src/codeql_ruby/controlflow/CfgNodes.qll b/ql/src/codeql_ruby/controlflow/CfgNodes.qll index 9b419871753..4ab22541942 100644 --- a/ql/src/codeql_ruby/controlflow/CfgNodes.qll +++ b/ql/src/codeql_ruby/controlflow/CfgNodes.qll @@ -135,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) {