From d69aa96f23d4b7cb897b6000de0703a1d63f1479 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Wed, 10 Feb 2021 14:59:54 +0100 Subject: [PATCH 1/3] More tests --- .../dataflow/local/DataflowStep.expected | 15 ++++++++++ .../dataflow/local/local_dataflow.rb | 28 +++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/ql/test/library-tests/dataflow/local/DataflowStep.expected b/ql/test/library-tests/dataflow/local/DataflowStep.expected index d877f7a867b..3c8f5cfd3a1 100644 --- a/ql/test/library-tests/dataflow/local/DataflowStep.expected +++ b/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 | | 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 | diff --git a/ql/test/library-tests/dataflow/local/local_dataflow.rb b/ql/test/library-tests/dataflow/local/local_dataflow.rb index b212fa9ce16..27fc5f4d841 100644 --- a/ql/test/library-tests/dataflow/local/local_dataflow.rb +++ b/ql/test/library-tests/dataflow/local/local_dataflow.rb @@ -23,3 +23,31 @@ end 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 From 0f6854301ef251e6ba7eaad8f75e132efeffb196 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Wed, 10 Feb 2021 17:04:27 +0100 Subject: [PATCH 2/3] Dataflow: identify ReturnNodes --- .../dataflow/internal/DataFlowPrivate.qll | 66 +++++++++++++++---- .../dataflow/local/ReturnNodes.expected | 9 +++ .../dataflow/local/ReturnNodes.ql | 4 ++ 3 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 ql/test/library-tests/dataflow/local/ReturnNodes.expected create mode 100644 ql/test/library-tests/dataflow/local/ReturnNodes.ql diff --git a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll index 4ac583160ff..2694e8d2a2d 100644 --- a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll +++ b/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.getExprNode().getNode() instanceof BreakStmt and exprTo.getNode() instanceof Loop and nodeTo.asExpr().getAPredecessor(any(SuccessorTypes::BreakSuccessor s)) = n.getExprNode() ) or - nodeFrom.asExpr() = nodeTo.(ExprReturnNode).getExprNode().getReturnedValueNode() + nodeFrom.asExpr() = nodeTo.(ReturningStatementNode).getExprNode().getReturnedValueNode() or nodeTo.asExpr() = any(CfgNodes::ExprNodes::ForExprCfgNode for | @@ -182,6 +182,28 @@ class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode { override string toStringImpl() { result = def.toString() } } +/** + * An 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 getExprNode() { 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.getExprNode()) 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() } + class ExprReturnNode extends ReturnNode, ExprNode { + ExprReturnNode() { + this.getExprNode().getASuccessor().(CfgNodes::AnnotatedExitNode).isNormal() and + this.getEnclosingCallable() instanceof Callable + } - override Location getLocationImpl() { result = n.getLocation() } - - override string toStringImpl() { result = n.toString() } + override ReturnKind getKind() { result instanceof NormalReturnKind } } } diff --git a/ql/test/library-tests/dataflow/local/ReturnNodes.expected b/ql/test/library-tests/dataflow/local/ReturnNodes.expected new file mode 100644 index 00000000000..a4971c21ade --- /dev/null +++ b/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 | diff --git a/ql/test/library-tests/dataflow/local/ReturnNodes.ql b/ql/test/library-tests/dataflow/local/ReturnNodes.ql new file mode 100644 index 00000000000..8b04dcd3dc5 --- /dev/null +++ b/ql/test/library-tests/dataflow/local/ReturnNodes.ql @@ -0,0 +1,4 @@ +import ruby +import codeql_ruby.dataflow.internal.DataFlowPrivate + +select any(ReturnNode node) From 4f3412fff9edd4e3e0c0d41be4053b0988f960c3 Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Thu, 11 Feb 2021 13:46:34 +0100 Subject: [PATCH 3/3] Address comments --- .../dataflow/internal/DataFlowPrivate.qll | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll index 2694e8d2a2d..38669d32736 100644 --- a/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ql/src/codeql_ruby/dataflow/internal/DataFlowPrivate.qll @@ -135,12 +135,12 @@ private module Cached { exists(CfgNodes::ExprCfgNode exprTo, ReturningStatementNode n | nodeFrom = n and exprTo = nodeTo.asExpr() and - n.getExprNode().getNode() instanceof BreakStmt 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.(ReturningStatementNode).getExprNode().getReturnedValueNode() + nodeFrom.asExpr() = nodeTo.(ReturningStatementNode).getReturningNode().getReturnedValueNode() or nodeTo.asExpr() = any(CfgNodes::ExprNodes::ForExprCfgNode for | @@ -183,7 +183,7 @@ class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode { } /** - * An value returning statement, viewed as a node in a data flow graph. + * 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 @@ -195,7 +195,7 @@ class ReturningStatementNode extends NodeImpl, TReturningNode { ReturningStatementNode() { this = TReturningNode(n) } /** Gets the expression corresponding to this node. */ - CfgNodes::ReturningCfgNode getExprNode() { result = n } + CfgNodes::ReturningCfgNode getReturningNode() { result = n } override CfgScope getCfgScope() { result = n.getScope() } @@ -287,7 +287,7 @@ private module ReturnNodes { private CfgNodes::ReturningCfgNode n; ExplicitReturnNode() { - isValid(this.getExprNode()) and + isValid(this.getReturningNode()) and n.getASuccessor().(CfgNodes::AnnotatedExitNode).isNormal() and n.getScope() instanceof Callable }