From b553eb69643cb5e39d8f90e9afd2508ab9f7929e Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Fri, 5 Feb 2021 18:21:20 +0100 Subject: [PATCH] CFG: make 'for .. in' post-order Use the 'in' as the intermediate node that checks whether the Enumerable has more elements. --- .../controlflow/internal/Completion.qll | 3 + .../internal/ControlFlowGraphImpl.qll | 60 ++++++++------ .../controlflow/graph/Cfg.expected | 83 +++++++++++-------- ql/test/library-tests/variables/ssa.expected | 12 +-- 4 files changed, 91 insertions(+), 67 deletions(-) diff --git a/ql/src/codeql_ruby/controlflow/internal/Completion.qll b/ql/src/codeql_ruby/controlflow/internal/Completion.qll index 3bbfff3977d..4da3a0c1319 100644 --- a/ql/src/codeql_ruby/controlflow/internal/Completion.qll +++ b/ql/src/codeql_ruby/controlflow/internal/Completion.qll @@ -54,6 +54,9 @@ private predicate nestedEnsureCompletion(Completion outer, int nestLevel) { pragma[noinline] private predicate completionIsValidForStmt(AstNode n, Completion c) { + n instanceof In and + c instanceof EmptinessCompletion + or n instanceof Break and c = TBreakCompletion() or diff --git a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll index 4cfeba99799..a6163c0f01a 100644 --- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll +++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll @@ -338,7 +338,7 @@ private class LeftToRightPostOrderTree extends StandardPostOrderTree, LeftToRigh } private class LeftToRightPreOrderNodes = - @alias or @block_parameters or @class or @do or @else or @ensure or @in or @lambda_parameters or + @alias or @block_parameters or @class or @do or @else or @ensure or @lambda_parameters or @method_parameters or @pattern or @program or @then or @undef or @yield; private class LeftToRightPreOrderTree extends StandardPreOrderTree, LeftToRightPreOrderNodes { @@ -346,7 +346,6 @@ private class LeftToRightPreOrderTree extends StandardPreOrderTree, LeftToRightP this instanceof BlockParameters or this instanceof Do or this instanceof Else or - this instanceof In or this instanceof LambdaParameters or this instanceof MethodParameters or this instanceof Pattern or @@ -592,7 +591,7 @@ module Trees { * ``` * args * | - * for------<----- + * in------<----- * / \ \ * / \ | * / \ | @@ -600,58 +599,63 @@ module Trees { * empty non-empty | * | \ | * puts "done" \ | - * arg | - * | | - * puts arg | + * | arg | + * | | | + * for puts arg | * \___/ * ``` */ - private class ForTree extends ControlFlowTree, For { + private class ForTree extends PostOrderTree, For { final override predicate propagatesAbnormal(AstNode child) { - child = this.getPattern() or child = this.getValue() + child = this.getPattern() or child = this.getArray() } - final override predicate first(AstNode node) { node = this.getValue() } + final override predicate first(AstNode first) { first(this.getArray(), first) } - final override predicate last(AstNode last, Completion c) { - last = this and - c.(EmptinessCompletion).getValue() = true - or - last(this.getBody(), last, c) and - not c.continuesLoop() and - not c instanceof BreakCompletion and - not c instanceof RedoCompletion - or - last(this.getBody(), last, c.(NestedBreakCompletion).getAnInnerCompatibleCompletion()) - } + private In getIn() { result = this.getValue() } + + private UnderscoreArg getArray() { result = this.getValue().getChild() } /** * for pattern in value do body end * ``` - * value +-> for +--[non empty]--> pattern -> body -> for - * |--[empty]--> exit + * array +-> in +--[non empty]--> pattern -> body -> in + * |--[empty]--> for * ``` */ final override predicate succ(AstNode pred, AstNode succ, Completion c) { - last(this.getValue(), pred, c) and - succ = this and + last(this.getArray(), pred, c) and + first(this.getIn(), succ) and c instanceof SimpleCompletion or - pred = this and + last(this.getIn(), pred, c) and first(this.getPattern(), succ) and c.(EmptinessCompletion).getValue() = false or - first(this.getBody(), succ) and last(this.getPattern(), pred, c) and + first(this.getBody(), succ) and c instanceof NormalCompletion or last(this.getBody(), pred, c) and - succ = this and + first(this.getIn(), succ) and c.continuesLoop() or last(this.getBody(), pred, c) and first(this.getBody(), succ) and c instanceof RedoCompletion + or + succ = this and + ( + last(this.getIn(), pred, c) and + c.(EmptinessCompletion).getValue() = true + or + last(this.getBody(), pred, c) and + not c.continuesLoop() and + not c instanceof BreakCompletion and + not c instanceof RedoCompletion + or + last(this.getBody(), pred, c.(NestedBreakCompletion).getAnInnerCompatibleCompletion()) + ) } } @@ -711,6 +715,8 @@ module Trees { } } + private class InTree extends LeafTree, In { } + private class InstanceVariableTree extends LeafTree, InstanceVariable { } private class IntegerTree extends LeafTree, Integer { } diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected index a7e9778436e..95c57ea8478 100644 --- a/ql/test/library-tests/controlflow/graph/Cfg.expected +++ b/ql/test/library-tests/controlflow/graph/Cfg.expected @@ -182,17 +182,20 @@ break_ensure.rb: #-----| -> elements # 2| for ... in ... -#-----| non-empty -> element -#-----| empty -> Ensure +#-----| -> Ensure # 2| element #-----| -> element +# 2| In +#-----| empty -> for ... in ... +#-----| non-empty -> element + # 2| elements -#-----| -> for ... in ... +#-----| -> In # 3| if ... -#-----| -> for ... in ... +#-----| -> In # 3| ... > ... #-----| false -> if ... @@ -205,7 +208,7 @@ break_ensure.rb: #-----| -> ... > ... # 4| Break -#-----| break -> Ensure +#-----| break -> for ... in ... # 7| Ensure #-----| -> elements @@ -242,14 +245,17 @@ break_ensure.rb: #-----| -> elements # 14| for ... in ... -#-----| non-empty -> element -#-----| empty -> exit m2 (normal) +#-----| -> exit m2 (normal) # 14| element #-----| -> element +# 14| In +#-----| empty -> for ... in ... +#-----| non-empty -> element + # 14| elements -#-----| -> for ... in ... +#-----| -> In # 16| if ... #-----| -> Ensure @@ -274,10 +280,10 @@ break_ensure.rb: #-----| -> [ensure: break] elements # 20| if ... -#-----| -> for ... in ... +#-----| -> In # 20| [ensure: break] if ... -#-----| break -> exit m2 (normal) +#-----| break -> for ... in ... # 20| call to nil? #-----| false -> if ... @@ -349,11 +355,9 @@ break_ensure.rb: #-----| -> [ensure: return] elements # 33| for ... in ... -#-----| non-empty -> element -#-----| empty -> puts +#-----| -> puts # 33| [ensure: return] for ... in ... -#-----| non-empty -> [ensure: return] element #-----| return -> exit m3 (normal) # 33| element @@ -362,17 +366,25 @@ break_ensure.rb: # 33| [ensure: return] element #-----| -> [ensure: return] call to x +# 33| In +#-----| empty -> for ... in ... +#-----| non-empty -> element + +# 33| [ensure: return] In +#-----| empty -> [ensure: return] for ... in ... +#-----| non-empty -> [ensure: return] element + # 33| elements -#-----| -> for ... in ... +#-----| -> In # 33| [ensure: return] elements -#-----| -> [ensure: return] for ... in ... +#-----| -> [ensure: return] In # 35| if ... -#-----| -> for ... in ... +#-----| -> In # 35| [ensure: return] if ... -#-----| -> [ensure: return] for ... in ... +#-----| -> [ensure: return] In # 35| ... > ... #-----| false -> if ... @@ -395,10 +407,10 @@ break_ensure.rb: #-----| -> [ensure: return] ... > ... # 36| Break -#-----| break -> puts +#-----| break -> for ... in ... # 36| [ensure: return] Break -#-----| return -> exit m3 (normal) +#-----| break -> [ensure: return] for ... in ... # 41| call to puts #-----| -> exit m3 (normal) @@ -419,14 +431,17 @@ break_ensure.rb: #-----| -> elements # 45| for ... in ... -#-----| non-empty -> element -#-----| empty -> exit m4 (normal) +#-----| -> exit m4 (normal) # 45| element #-----| -> element +# 45| In +#-----| empty -> for ... in ... +#-----| non-empty -> element + # 45| elements -#-----| -> for ... in ... +#-----| -> In # 47| if ... #-----| -> Ensure @@ -457,10 +472,10 @@ break_ensure.rb: #-----| -> [ensure: raise] element # 51| if ... -#-----| -> for ... in ... +#-----| -> In # 51| [ensure: raise] if ... -#-----| raise -> exit m4 (abnormal) +#-----| raise -> for ... in ... # 51| ... > ... #-----| false -> if ... @@ -483,10 +498,10 @@ break_ensure.rb: #-----| -> [ensure: raise] ... > ... # 52| Break -#-----| break -> exit m4 (normal) +#-----| break -> for ... in ... # 52| [ensure: raise] Break -#-----| break -> exit m4 (normal) +#-----| break -> for ... in ... # 52| 10 #-----| -> Break @@ -1131,14 +1146,17 @@ cfg.rb: #-----| -> Interpolation # 90| for ... in ... -#-----| non-empty -> x -#-----| empty -> 42 +#-----| -> 42 # 90| x #-----| -> x +# 90| In +#-----| empty -> for ... in ... +#-----| non-empty -> x + # 90| Array -#-----| -> for ... in ... +#-----| -> In # 90| 1.4 #-----| -> 2.5 @@ -1163,10 +1181,10 @@ cfg.rb: #-----| -> ... > ... # 91| Next -#-----| next -> for ... in ... +#-----| next -> In # 92| call to puts -#-----| -> for ... in ... +#-----| -> In # 92| puts #-----| -> x @@ -3878,9 +3896,6 @@ break_ensure.rb: # 27| exit m3 (normal) #-----| -> exit m3 -# 44| exit m4 (abnormal) -#-----| -> exit m4 - # 44| exit m4 (normal) #-----| -> exit m4 diff --git a/ql/test/library-tests/variables/ssa.expected b/ql/test/library-tests/variables/ssa.expected index e58a577689c..87f3097503e 100644 --- a/ql/test/library-tests/variables/ssa.expected +++ b/ql/test/library-tests/variables/ssa.expected @@ -57,8 +57,8 @@ definition | ssa.rb:21:5:21:10 | ... -= ... | ssa.rb:18:8:18:8 | x | | ssa.rb:25:1:30:3 | | ssa.rb:26:7:26:10 | elem | | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements | -| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | +| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x | | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 | | ssa.rb:44:1:47:3 | | ssa.rb:45:3:45:3 | x | @@ -141,8 +141,8 @@ read | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:20:10:20:10 | x | | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:21:5:21:5 | x | | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements | ssa.rb:26:15:26:22 | elements | -| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem | | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | ssa.rb:27:10:27:13 | elem | +| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem | | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x | ssa.rb:34:10:34:10 | x | | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 | ssa.rb:41:8:41:9 | m3 | | ssa.rb:44:8:44:8 | b | ssa.rb:44:8:44:8 | b | ssa.rb:45:12:45:12 | b | @@ -210,8 +210,8 @@ firstRead | ssa.rb:10:5:10:9 | ... = ... | ssa.rb:2:3:2:3 | i | ssa.rb:11:10:11:10 | i | | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:19:9:19:9 | x | | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements | ssa.rb:26:15:26:22 | elements | -| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem | | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | ssa.rb:27:10:27:13 | elem | +| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem | | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x | ssa.rb:34:10:34:10 | x | | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 | ssa.rb:41:8:41:9 | m3 | | ssa.rb:44:8:44:8 | b | ssa.rb:44:8:44:8 | b | ssa.rb:45:12:45:12 | b | @@ -279,8 +279,8 @@ lastRead | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:19:9:19:9 | x | | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:21:5:21:5 | x | | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements | ssa.rb:26:15:26:22 | elements | -| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem | | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | ssa.rb:27:10:27:13 | elem | +| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem | | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x | ssa.rb:34:10:34:10 | x | | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 | ssa.rb:41:8:41:9 | m3 | | ssa.rb:44:8:44:8 | b | ssa.rb:44:8:44:8 | b | ssa.rb:45:12:45:12 | b | @@ -318,8 +318,8 @@ phi | ssa.rb:5:3:13:5 | phi | ssa.rb:2:3:2:3 | i | ssa.rb:10:5:10:9 | ... = ... | | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:18:8:18:8 | x | | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:21:5:21:10 | ... -= ... | -| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:25:1:30:3 | | -| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | +| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:25:1:30:3 | | +| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | | ssa.rb:45:3:45:12 | phi | ssa.rb:45:3:45:3 | x | ssa.rb:44:1:47:3 | | | ssa.rb:45:3:45:12 | phi | ssa.rb:45:3:45:3 | x | ssa.rb:45:3:45:7 | ... = ... | | ssa.rb:50:3:50:6 | phi | ssa.rb:49:14:49:14 | y | ssa.rb:49:1:51:3 | |