From 06a6a3feb0b57cb024ac18f907fbbe471f275789 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 19 Nov 2020 14:31:08 +0100 Subject: [PATCH] Address review comments --- .../codeql_ruby/controlflow/BasicBlocks.qll | 2 +- .../controlflow/ControlFlowGraph.qll | 48 ++++++++++++++----- .../controlflow/internal/AstNodes.qll | 2 +- .../controlflow/internal/Completion.qll | 13 ++++- .../controlflow/internal/Consistency.qll | 8 ++-- .../internal/ControlFlowGraphImpl.qll | 3 +- .../controlflow/internal/NonReturning.qll | 2 +- .../controlflow/internal/Splitting.qll | 2 +- ql/src/codeql_ruby/printAst.qll | 2 +- .../controlflow/graph/Cfg.expected | 12 ++--- 10 files changed, 66 insertions(+), 28 deletions(-) diff --git a/ql/src/codeql_ruby/controlflow/BasicBlocks.qll b/ql/src/codeql_ruby/controlflow/BasicBlocks.qll index 9b46e082a3b..5f320a554ea 100644 --- a/ql/src/codeql_ruby/controlflow/BasicBlocks.qll +++ b/ql/src/codeql_ruby/controlflow/BasicBlocks.qll @@ -1,6 +1,6 @@ /** Provides classes representing basic blocks. */ -import codeql_ruby.ast +private import codeql_ruby.ast private import codeql_ruby.controlflow.ControlFlowGraph private import internal.ControlFlowGraphImpl private import SuccessorTypes diff --git a/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll b/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll index 9e464b80158..080f64da579 100644 --- a/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll +++ b/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll @@ -1,24 +1,26 @@ /** Provides classes representing the control flow graph. */ -import codeql_ruby.ast +private import codeql_ruby.ast private import codeql_ruby.controlflow.BasicBlocks private import SuccessorTypes private import internal.ControlFlowGraphImpl private import internal.Splitting private import internal.Completion +private class CfgScopeRange = @method or @block or @do_block; + /** An AST node with an associated control-flow graph. */ -class CfgScope extends AstNode { - private string name; - - CfgScope() { - name = this.(Method).getName().toString() - or - this = any(MethodCall mc | name = "block for " + mc.getMethod()).getBlock() - } - +class CfgScope extends AstNode, CfgScopeRange { /** Gets the name of this scope. */ - string getName() { result = name } + string getName() { + result = this.(Method).getName().toString() + or + this instanceof Block and + result = "block" + or + this instanceof DoBlock and + result = "do block" + } } /** @@ -276,6 +278,30 @@ module SuccessorTypes { final override string toString() { result = "redo" } } + /** + * A `retry` control flow successor. + * + * Example: + * + * Example: + * + * ```rb + * def m + * begin + * puts "Retry" + * raise + * rescue + * retry + * end + * end + * ``` + * + * The node `puts "Retry"` is `retry` successor of the node `retry`. + */ + class RetrySuccessor extends SuccessorType, TRetrySuccessor { + final override string toString() { result = "retry" } + } + /** * An exceptional control flow successor. * diff --git a/ql/src/codeql_ruby/controlflow/internal/AstNodes.qll b/ql/src/codeql_ruby/controlflow/internal/AstNodes.qll index 5655f4d5bea..2562fc88e29 100644 --- a/ql/src/codeql_ruby/controlflow/internal/AstNodes.qll +++ b/ql/src/codeql_ruby/controlflow/internal/AstNodes.qll @@ -3,7 +3,7 @@ * will likely be part of the hand-written user-facing AST layer. */ -import codeql_ruby.ast +private import codeql_ruby.ast class LogicalNotAstNode extends Unary { AstNode operand; diff --git a/ql/src/codeql_ruby/controlflow/internal/Completion.qll b/ql/src/codeql_ruby/controlflow/internal/Completion.qll index 88594e8d169..4ca358c0497 100644 --- a/ql/src/codeql_ruby/controlflow/internal/Completion.qll +++ b/ql/src/codeql_ruby/controlflow/internal/Completion.qll @@ -4,7 +4,7 @@ * A completion represents how a statement or expression terminates. */ -import codeql_ruby.ast +private import codeql_ruby.ast private import codeql_ruby.controlflow.ControlFlowGraph private import AstNodes private import NonReturning @@ -17,6 +17,7 @@ private newtype TCompletion = TBreakCompletion() or TNextCompletion() or TRedoCompletion() or + TRetryCompletion() or TRaiseCompletion() or // TODO: Add exception type? TExitCompletion() or TNestedCompletion(Completion inner, Completion outer) { @@ -220,6 +221,16 @@ class RedoCompletion extends Completion, TRedoCompletion { override string toString() { result = "redo" } } +/** + * A completion that represents evaluation of a statement or an + * expression resulting in a retry. + */ +class RetryCompletion extends Completion, TRetryCompletion { + override RetrySuccessor getAMatchingSuccessorType() { any() } + + override string toString() { result = "retry" } +} + /** * A completion that represents evaluation of a statement or an * expression resulting in a thrown exception. diff --git a/ql/src/codeql_ruby/controlflow/internal/Consistency.qll b/ql/src/codeql_ruby/controlflow/internal/Consistency.qll index 6252645fecf..1488d32a6a5 100644 --- a/ql/src/codeql_ruby/controlflow/internal/Consistency.qll +++ b/ql/src/codeql_ruby/controlflow/internal/Consistency.qll @@ -1,7 +1,7 @@ -import codeql_ruby.ast -import codeql_ruby.controlflow.ControlFlowGraph -import Completion -import Splitting +private import codeql_ruby.ast +private import codeql_ruby.controlflow.ControlFlowGraph +private import Completion +private import Splitting query predicate nonUniqueSetRepresentation(Splits s1, Splits s2) { forex(Split s | s = s1.getASplit() | s = s2.getASplit()) and diff --git a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll index 16719988da5..283f77d4908 100644 --- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll +++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll @@ -31,7 +31,7 @@ * caught up by its surrounding loop and turned into a `NormalCompletion`. */ -import codeql_ruby.ast +private import codeql_ruby.ast private import AstNodes private import codeql_ruby.controlflow.ControlFlowGraph private import Completion @@ -501,6 +501,7 @@ private module Cached { TBreakSuccessor() or TNextSuccessor() or TRedoSuccessor() or + TRetrySuccessor() or TRaiseSuccessor() or // TODO: Add exception type? TExitSuccessor() diff --git a/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll b/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll index 31323635a0c..119a1e85ac5 100644 --- a/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll +++ b/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll @@ -1,6 +1,6 @@ /** Provides a simple analysis for identifying calls that will not return. */ -import codeql_ruby.ast +private import codeql_ruby.ast private import Completion /** A call that definitely does not return (conservative analysis). */ diff --git a/ql/src/codeql_ruby/controlflow/internal/Splitting.qll b/ql/src/codeql_ruby/controlflow/internal/Splitting.qll index bfa96864faa..4edb360ecec 100644 --- a/ql/src/codeql_ruby/controlflow/internal/Splitting.qll +++ b/ql/src/codeql_ruby/controlflow/internal/Splitting.qll @@ -2,7 +2,7 @@ * Provides classes and predicates relevant for splitting the control flow graph. */ -import codeql_ruby.ast +private import codeql_ruby.ast private import AstNodes private import Completion private import ControlFlowGraphImpl diff --git a/ql/src/codeql_ruby/printAst.qll b/ql/src/codeql_ruby/printAst.qll index 287e8d90637..68049d28ad2 100644 --- a/ql/src/codeql_ruby/printAst.qll +++ b/ql/src/codeql_ruby/printAst.qll @@ -6,7 +6,7 @@ * to hold for only the AST nodes you wish to view. */ -import codeql_ruby.ast +private import codeql_ruby.ast /** * The query can extend this class to control which nodes are printed. diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected index a97c9efeebd..5bb2e3b2a6f 100644 --- a/ql/test/library-tests/controlflow/graph/Cfg.expected +++ b/ql/test/library-tests/controlflow/graph/Cfg.expected @@ -150,9 +150,9 @@ nodes | 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:25:16:27:5 | enter do block | +| loops.rb:25:16:27:5 | exit do block | +| loops.rb:25:16:27:5 | exit do block (normal) | | loops.rb:26:5:26:8 | puts | | loops.rb:26:5:26:10 | MethodCall | | loops.rb:26:10:26:10 | x | @@ -328,10 +328,10 @@ edges | 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:25:16:27:5 | enter do block | loops.rb:26:10:26:10 | x | semmle.label | successor | +| loops.rb:25:16:27:5 | exit do block (normal) | loops.rb:25:16:27:5 | exit do block | 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:5:26:10 | MethodCall | loops.rb:25:16:27:5 | exit do block (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 |