From 4626168969749b074cf717a2c0c32d1a4ff5683b Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 19 Nov 2020 09:29:15 +0100 Subject: [PATCH] CFG: Separate scope for method blocks --- .../controlflow/ControlFlowGraph.qll | 13 +++++++- .../internal/ControlFlowGraphImpl.qll | 15 +++++----- .../controlflow/internal/Splitting.qll | 7 ++++- .../controlflow/graph/Cfg.expected | 30 +++++++++++++++++++ .../library-tests/controlflow/graph/loops.rb | 6 ++++ 5 files changed, 62 insertions(+), 9 deletions(-) diff --git a/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll b/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll index fed5c07473f..9e464b80158 100644 --- a/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll +++ b/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll @@ -8,7 +8,18 @@ private import internal.Splitting private import internal.Completion /** An AST node with an associated control-flow graph. */ -class CfgScope = Method; // TODO: add more cases (e.g. bodies of lambdas/do blocks) +class CfgScope extends AstNode { + private string name; + + CfgScope() { + name = this.(Method).getName().toString() + or + this = any(MethodCall mc | name = "block for " + mc.getMethod()).getBlock() + } + + /** Gets the name of this scope. */ + string getName() { result = name } +} /** * A control flow node. diff --git a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll index 831ea1111c5..16719988da5 100644 --- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll +++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll @@ -256,11 +256,7 @@ private module Trees { } } - private class BlockParameterTree extends LeafTree, BlockParameter { } - - private class BlockParametersTree extends StandardPostOrderTree, BlockParameters { - final override AstNode getChildNode(int i) { result = this.getChild(i) } - + private class BlockParametersTree extends LeafTree, BlockParameters { override predicate isHidden() { any() } } @@ -282,6 +278,12 @@ private module Trees { override predicate isHidden() { any() } } + private class DoBlockTree extends StandardPreOrderTree, DoBlock { + final override AstNode getChildNode(int i) { result = this.getChild(i) } + + override predicate isHidden() { any() } + } + private class ElseTree extends StandardPreOrderTree, Else { final override AstNode getChildNode(int i) { result = this.getChild(i) } @@ -382,12 +384,11 @@ private module Trees { } private class MethodCallTree extends StandardPostOrderTree, MethodCall { + // this.getBlock() is not included as it uses a different scope final override AstNode getChildNode(int i) { result = this.getArguments() and i = 0 or result = this.getMethod() and i = 1 - or - result = this.getBlock() and i = 2 } } diff --git a/ql/src/codeql_ruby/controlflow/internal/Splitting.qll b/ql/src/codeql_ruby/controlflow/internal/Splitting.qll index 5a6f81a08d1..bfa96864faa 100644 --- a/ql/src/codeql_ruby/controlflow/internal/Splitting.qll +++ b/ql/src/codeql_ruby/controlflow/internal/Splitting.qll @@ -60,7 +60,12 @@ class Split extends TSplit { string toString() { none() } } -private CfgScope getScope(AstNode n) { result.getAFieldOrChild*() = n } +private AstNode parent(AstNode n) { + result.getAFieldOrChild() = n and + not n instanceof CfgScope +} + +private CfgScope getScope(AstNode n) { result = parent+(n) } /** * Holds if split kinds `sk1` and `sk2` may overlap. That is, they may apply diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected index e61fc6f0a07..a97c9efeebd 100644 --- a/ql/test/library-tests/controlflow/graph/Cfg.expected +++ b/ql/test/library-tests/controlflow/graph/Cfg.expected @@ -140,6 +140,22 @@ nodes | loops.rb:21:3:21:6 | puts | | loops.rb:21:3:21:13 | MethodCall | | loops.rb:21:8:21:13 | String | +| loops.rb:24:1:28:3 | enter m3 | +| loops.rb:24:1:28:3 | exit m3 | +| loops.rb:24:1:28:3 | exit m3 (normal) | +| loops.rb:25:3:25:9 | Array | +| loops.rb:25:3:25:14 | Call | +| loops.rb:25:3:27:5 | MethodCall | +| loops.rb:25:4:25:4 | 1 | +| loops.rb:25:6:25:6 | 2 | +| loops.rb:25:8:25:8 | 3 | +| loops.rb:25:11:25:14 | each | +| loops.rb:25:16:27:5 | enter block for Call | +| loops.rb:25:16:27:5 | exit block for Call | +| loops.rb:25:16:27:5 | exit block for Call (normal) | +| loops.rb:26:5:26:8 | puts | +| loops.rb:26:5:26:10 | MethodCall | +| loops.rb:26:10:26:10 | x | | raise.rb:1:1:6:3 | enter m1 | | raise.rb:1:1:6:3 | exit m1 | | raise.rb:1:1:6:3 | exit m1 (abnormal) | @@ -303,6 +319,20 @@ edges | loops.rb:21:3:21:6 | puts | loops.rb:21:3:21:13 | MethodCall | semmle.label | successor | | loops.rb:21:3:21:13 | MethodCall | loops.rb:8:1:22:3 | exit m2 (normal) | semmle.label | successor | | loops.rb:21:8:21:13 | String | loops.rb:21:3:21:6 | puts | semmle.label | successor | +| loops.rb:24:1:28:3 | enter m3 | loops.rb:25:4:25:4 | 1 | semmle.label | successor | +| loops.rb:24:1:28:3 | exit m3 (normal) | loops.rb:24:1:28:3 | exit m3 | semmle.label | successor | +| loops.rb:25:3:25:9 | Array | loops.rb:25:11:25:14 | each | semmle.label | successor | +| loops.rb:25:3:25:14 | Call | loops.rb:25:3:27:5 | MethodCall | semmle.label | successor | +| loops.rb:25:3:27:5 | MethodCall | loops.rb:24:1:28:3 | exit m3 (normal) | semmle.label | successor | +| loops.rb:25:4:25:4 | 1 | loops.rb:25:6:25:6 | 2 | semmle.label | successor | +| loops.rb:25:6:25:6 | 2 | loops.rb:25:8:25:8 | 3 | semmle.label | successor | +| loops.rb:25:8:25:8 | 3 | loops.rb:25:3:25:9 | Array | semmle.label | successor | +| loops.rb:25:11:25:14 | each | loops.rb:25:3:25:14 | Call | semmle.label | successor | +| loops.rb:25:16:27:5 | enter block for Call | loops.rb:26:10:26:10 | x | semmle.label | successor | +| loops.rb:25:16:27:5 | exit block for Call (normal) | loops.rb:25:16:27:5 | exit block for Call | semmle.label | successor | +| loops.rb:26:5:26:8 | puts | loops.rb:26:5:26:10 | MethodCall | semmle.label | successor | +| loops.rb:26:5:26:10 | MethodCall | loops.rb:25:16:27:5 | exit block for Call (normal) | semmle.label | successor | +| loops.rb:26:10:26:10 | x | loops.rb:26:5:26:8 | puts | semmle.label | successor | | raise.rb:1:1:6:3 | enter m1 | raise.rb:2:3:4:5 | If | semmle.label | successor | | raise.rb:1:1:6:3 | exit m1 (abnormal) | raise.rb:1:1:6:3 | exit m1 | semmle.label | successor | | raise.rb:1:1:6:3 | exit m1 (normal) | raise.rb:1:1:6:3 | exit m1 | semmle.label | successor | diff --git a/ql/test/library-tests/controlflow/graph/loops.rb b/ql/test/library-tests/controlflow/graph/loops.rb index b6fc850c572..01222db77e1 100644 --- a/ql/test/library-tests/controlflow/graph/loops.rb +++ b/ql/test/library-tests/controlflow/graph/loops.rb @@ -20,3 +20,9 @@ def m2 x end puts "Done" end + +def m3 + [1,2,3].each do |x| + puts x + end +end \ No newline at end of file