diff --git a/ql/src/codeql_ruby/controlflow/internal/Completion.qll b/ql/src/codeql_ruby/controlflow/internal/Completion.qll index 3b86799b8c9..ebd84634ea1 100644 --- a/ql/src/codeql_ruby/controlflow/internal/Completion.qll +++ b/ql/src/codeql_ruby/controlflow/internal/Completion.qll @@ -222,8 +222,13 @@ class SimpleCompletion extends NonNestedNormalCompletion, TSimpleCompletion { * completion (`EmptinessCompletion`), or a matching completion (`MatchingCompletion`). */ abstract class ConditionalCompletion extends NonNestedNormalCompletion { + boolean value; + + bindingset[value] + ConditionalCompletion() { any() } + /** Gets the Boolean value of this conditional completion. */ - abstract boolean getValue(); + final boolean getValue() { result = value } } /** @@ -231,12 +236,8 @@ abstract class ConditionalCompletion extends NonNestedNormalCompletion { * with a Boolean value. */ class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion { - private boolean value; - BooleanCompletion() { this = TBooleanCompletion(value) } - override boolean getValue() { result = value } - /** Gets the dual Boolean completion. */ BooleanCompletion getDual() { result = TBooleanCompletion(value.booleanNot()) } @@ -260,12 +261,8 @@ class FalseCompletion extends BooleanCompletion { * a test in a `for in` statement. */ class EmptinessCompletion extends ConditionalCompletion, TEmptinessCompletion { - private boolean value; - EmptinessCompletion() { this = TEmptinessCompletion(value) } - override boolean getValue() { result = value } - override EmptinessSuccessor getAMatchingSuccessorType() { result.getValue() = value } override string toString() { if value = true then result = "empty" else result = "non-empty" } @@ -276,12 +273,8 @@ class EmptinessCompletion extends ConditionalCompletion, TEmptinessCompletion { * a test in a `rescue` statement. */ class MatchingCompletion extends ConditionalCompletion, TMatchingCompletion { - private boolean value; - MatchingCompletion() { this = TMatchingCompletion(value) } - override boolean getValue() { result = value } - override MatchingSuccessor getAMatchingSuccessorType() { result.getValue() = value } override string toString() { if value = true then result = "match" else result = "no-match" } diff --git a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll index b37af98d2c6..66199da4003 100644 --- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll +++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll @@ -476,41 +476,13 @@ module Trees { private class ExceptionsTree extends PreOrderTree, Exceptions { final override predicate propagatesAbnormal(AstNode child) { none() } - /** Holds if this exception filter belongs to a final `rescue` block. */ - pragma[noinline] - private predicate inLastRescue() { - exists(RescueEnsureBlockTree block, Rescue rescue, int i | - rescue = block.getRescue(i) and - this = rescue.getExceptions() and - not exists(block.getRescue(i + 1)) - ) - } - final override predicate last(AstNode last, Completion c) { last(this.getChild(_), last, c) and c.(MatchingCompletion).getValue() = true or - exists(int lst, Completion c0 | - last(this.getChild(lst), last, c0) and + exists(int lst | + last(this.getChild(lst), last, c) and not exists(this.getChild(lst + 1)) - | - // The last exception filter in a last `rescue` block propagates - // the exception when it doesn't match - this.inLastRescue() and - c = - any(NestedEnsureCompletion nec | - nec.getOuterCompletion() instanceof RaiseCompletion and - nec.getInnerCompletion() = c0 and - c0.(MatchingCompletion).getValue() = false and - nec.getNestLevel() = 0 - ) - or - c = c0 and - ( - not this.inLastRescue() - or - not c0.(MatchingCompletion).getValue() = false - ) ) } @@ -889,23 +861,36 @@ module Trees { private class RescueTree extends PreOrderTree, Rescue { final override predicate propagatesAbnormal(AstNode child) { child = this.getExceptions() } - final override predicate last(AstNode last, Completion c) { - last(this.getExceptions(), last, c) and - c.(MatchingCompletion).getValue() = false - or + predicate lastMatch(AstNode last, Completion c) { last(this.getBody(), last, c) or - not exists(this.getExceptions()) and not exists(this.getBody()) and ( last(this.getVariable(), last, c) or not exists(this.getVariable()) and - last = this and - c.isValidFor(this) + ( + last(this.getExceptions(), last, c) and + c.(MatchingCompletion).getValue() = true + or + not exists(this.getExceptions()) and + last = this and + c.isValidFor(this) + ) ) } + predicate lastNoMatch(AstNode last, Completion c) { + last(this.getExceptions(), last, c) and + c.(MatchingCompletion).getValue() = false + } + + final override predicate last(AstNode last, Completion c) { + this.lastNoMatch(last, c) + or + this.lastMatch(last, c) + } + final override predicate succ(AstNode pred, AstNode succ, Completion c) { exists(AstNode next | pred = this and @@ -983,7 +968,11 @@ module Trees { final private predicate hasEnsure() { exists(this.getEnsure()) } - final override predicate propagatesAbnormal(AstNode child) { child = this.getEnsure() } + final override predicate propagatesAbnormal(AstNode child) { + child = this.getEnsure() + or + child = this.getBodyChild(_, false) + } /** * Gets a descendant that belongs to the `ensure` block of this block, if any. @@ -1045,37 +1034,39 @@ module Trees { */ pragma[nomagic] private AstNode getAnEnsurePredecessor(Completion c, boolean ensurable) { - exists(boolean ensurable0 | - // Exit completions ignore the `ensure` block (TODO: CHECK THIS) - if c instanceof ExitCompletion then ensurable = false else ensurable = ensurable0 - | - this.lastBody(result, c, ensurable0) and - ( - // Any non-throw completion will always continue directly to the `ensure` block, - // unless there is an `else` block - not c instanceof RaiseCompletion and - not exists(this.getElse()) - or - // Any completion will continue to the `ensure` block when there are no `rescue` - // blocks - not exists(this.getRescue(_)) - ) + this.lastBody(result, c, ensurable) and + ( + // Any non-throw completion will always continue directly to the `ensure` block, + // unless there is an `else` block + not c instanceof RaiseCompletion and + not exists(this.getElse()) or - // Last element from any of the `rescue` blocks continues to the `ensure` block - last(this.getRescue(_).getBody(), result, c) and - ensurable0 = true - or - // Last element of last `rescue` block continues to the `ensure` block - exists(int lst | - last(this.getRescue(lst), result, c) and - not exists(this.getRescue(lst + 1)) and - ensurable0 = true - ) - or - // Last element of `else` block continues to the `ensure` block - last(this.getElse(), result, c) and - ensurable0 = true + // Any completion will continue to the `ensure` block when there are no `rescue` + // blocks + not exists(this.getRescue(_)) ) + or + // Last element from any matching `rescue` block continues to the `ensure` block + this.getRescue(_).(RescueTree).lastMatch(result, c) and + ensurable = true + or + // If the last `rescue` block does not match, continue to the `ensure` block + exists(int lst, MatchingCompletion mc | + this.getRescue(lst).(RescueTree).lastNoMatch(result, mc) and + mc.getValue() = false and + not exists(this.getRescue(lst + 1)) and + c = + any(NestedEnsureCompletion nec | + nec.getOuterCompletion() instanceof RaiseCompletion and + nec.getInnerCompletion() = mc and + nec.getNestLevel() = 0 + ) and + ensurable = true + ) + or + // Last element of `else` block continues to the `ensure` block + last(this.getElse(), result, c) and + ensurable = true } pragma[nomagic] @@ -1129,19 +1120,10 @@ module Trees { first(this.getRescue(0), succ) and c instanceof RaiseCompletion or - exists(Rescue rescue, int i | rescue = this.getRescue(i) | - pred = rescue and - last(this.getRescue(i), rescue, c) and - ( - // Flow from one `rescue` clause to the next when there is no match - first(this.getRescue(i + 1), succ) and - c.(MatchingCompletion).getValue() = false - or - // Flow from last `rescue` clause to `ensure` block - not exists(this.getRescue(i + 1)) and - first(this.getEnsure(), succ) and - c.getInnerCompletion().(MatchingCompletion).getValue() = false - ) + // Flow from one `rescue` clause to the next when there is no match + exists(RescueTree rescue, int i | rescue = this.getRescue(i) | + rescue.lastNoMatch(pred, c) and + first(this.getRescue(i + 1), succ) ) or // Flow from body to `else` block when no exception diff --git a/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll b/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll index dc065750c5f..acba5a0ed26 100644 --- a/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll +++ b/ql/src/codeql_ruby/controlflow/internal/NonReturning.qll @@ -18,5 +18,5 @@ private class RaiseCall extends NonReturningCall, MethodCall { private class ExitCall extends NonReturningCall, MethodCall { ExitCall() { this.getMethod().toString() in ["abort", "exit"] } - override ExitCompletion getACompletion() { any() } + override ExitCompletion getACompletion() { not result instanceof NestedCompletion } } diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected index d071817a5ef..5426d01dd9a 100644 --- a/ql/test/library-tests/controlflow/graph/Cfg.expected +++ b/ql/test/library-tests/controlflow/graph/Cfg.expected @@ -125,6 +125,9 @@ raise.rb: # 121| enter m10 #-----| -> OptionalParameter +# 128| enter m11 +#-----| -> b + break_ensure.rb: # 1| elements #-----| -> elements @@ -3059,16 +3062,91 @@ raise.rb: # 121| String #-----| -> raise -# 122| Ensure +# 124| Ensure #-----| -> String -# 123| MethodCall +# 125| MethodCall #-----| -> exit m10 (normal) -# 123| puts +# 125| puts #-----| -> MethodCall -# 123| String +# 125| String +#-----| -> puts + +# 128| b +#-----| -> If + +# 130| If +#-----| -> b + +# 130| b +#-----| true -> ExceptionA +#-----| false -> Ensure + +# 131| MethodCall +#-----| raise -> Rescue + +# 131| raise +#-----| -> MethodCall + +# 131| ExceptionA +#-----| -> raise + +# 133| Rescue +#-----| -> ExceptionA + +# 133| ExceptionA +#-----| no-match -> Rescue +#-----| match -> Ensure + +# 134| Rescue +#-----| -> ExceptionB + +# 134| ExceptionB +#-----| match -> String +#-----| raise -> [ensure: raise] Ensure + +# 135| MethodCall +#-----| -> Ensure + +# 135| puts +#-----| -> MethodCall + +# 135| String +#-----| -> puts + +# 136| Ensure +#-----| -> String + +# 136| [ensure: raise] Ensure +#-----| -> [ensure: raise] String + +# 137| MethodCall +#-----| -> String + +# 137| [ensure: raise] MethodCall +#-----| raise -> exit m11 (abnormal) + +# 137| puts +#-----| -> MethodCall + +# 137| [ensure: raise] puts +#-----| -> [ensure: raise] MethodCall + +# 137| String +#-----| -> puts + +# 137| [ensure: raise] String +#-----| -> [ensure: raise] puts + +# 139| MethodCall +#-----| -> exit m11 (normal) + +# 139| puts +#-----| -> MethodCall + +# 139| String #-----| -> puts break_ensure.rb: @@ -3156,6 +3234,8 @@ raise.rb: # 121| exit m10 +# 128| exit m11 + break_ensure.rb: # 1| exit m1 (normal) #-----| -> exit m1 @@ -3306,3 +3386,9 @@ raise.rb: # 121| exit m10 (normal) #-----| -> exit m10 + +# 128| exit m11 (abnormal) +#-----| -> exit m11 + +# 128| exit m11 (normal) +#-----| -> exit m11 diff --git a/ql/test/library-tests/controlflow/graph/raise.rb b/ql/test/library-tests/controlflow/graph/raise.rb index d9db0cc2821..cf775077594 100644 --- a/ql/test/library-tests/controlflow/graph/raise.rb +++ b/ql/test/library-tests/controlflow/graph/raise.rb @@ -119,6 +119,22 @@ ensure end def m10(p = (raise "Exception")) +rescue + puts "Will not get executed if p is not supplied" ensure puts "Will not get executed if p is not supplied" -end \ No newline at end of file +end + +def m11 b + begin + if b + raise ExceptionA + end + rescue ExceptionA + rescue ExceptionB + puts "ExceptionB" + ensure + puts "Ensure" + end + puts "End m5" +end