diff --git a/ql/src/codeql_ruby/AST.qll b/ql/src/codeql_ruby/AST.qll index 25edc0ec394..b1c6262a244 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.AST diff --git a/ql/src/codeql_ruby/ast/Control.qll b/ql/src/codeql_ruby/ast/Control.qll index 64b27212840..66110be14f3 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/ast/Statement.qll b/ql/src/codeql_ruby/ast/Statement.qll new file mode 100644 index 00000000000..b4d7ff892db --- /dev/null +++ b/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" } +} 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 new file mode 100644 index 00000000000..b4fa88ce159 --- /dev/null +++ b/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() } + } +} diff --git a/ql/src/codeql_ruby/controlflow/CfgNodes.qll b/ql/src/codeql_ruby/controlflow/CfgNodes.qll index 6a8410d8434..4ab22541942 100644 --- a/ql/src/codeql_ruby/controlflow/CfgNodes.qll +++ b/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 } } 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 bbfb4019c33..4ac583160ff 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 } @@ -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() } } } 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 b920f8a2876..d877f7a867b 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 | 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 | 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