From 46fc17da582b26eb2f882731c10214bf891f0304 Mon Sep 17 00:00:00 2001
From: Tom Hvitved <hvitved@github.com>
Date: Thu, 17 Dec 2020 10:23:52 +0100
Subject: [PATCH] CFG: Fix multiple abnormal successors

---
 .../controlflow/ControlFlowGraph.qll          |  3 +++
 .../internal/ControlFlowGraphImpl.qll         | 24 +++++++++++--------
 .../controlflow/graph/Cfg.expected            |  4 ----
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll b/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll
index 98a2398db09..9c059516f72 100644
--- a/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll
+++ b/ql/src/codeql_ruby/controlflow/ControlFlowGraph.qll
@@ -118,6 +118,9 @@ private module CfgScope {
     }
 
     final override predicate exit(Generated::AstNode last, Completion c) {
+      last(this.getParameters(), last, c) and
+      not c instanceof NormalCompletion
+      or
       last(this.getBody().(Trees::BlockTree).getLastChildNode(), last, c)
       or
       this.getBody().(Trees::RescueEnsureBlockTree).lastBody(last, c)
diff --git a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll
index 4474165b4a0..a0762c46d51 100644
--- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll
+++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll
@@ -164,7 +164,7 @@ abstract private class StandardNode extends ControlFlowTree {
     )
   }
 
-  final override predicate propagatesAbnormal(AstNode child) { child = this.getChildNodeRanked(_) }
+  override predicate propagatesAbnormal(AstNode child) { child = this.getChildNode(_) }
 
   pragma[nomagic]
   override predicate succ(AstNode pred, AstNode succ, Completion c) {
@@ -223,9 +223,17 @@ abstract private class StandardPostOrderTree extends StandardNode, PostOrderTree
 }
 
 abstract private class LeafTree extends PreOrderTree, PostOrderTree {
+  override predicate propagatesAbnormal(AstNode child) { none() }
+
+  override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }
+}
+
+abstract class ScopeTree extends StandardNode, LeafTree {
   final override predicate propagatesAbnormal(AstNode child) { none() }
 
-  final override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }
+  final override predicate succ(AstNode pred, AstNode succ, Completion c) {
+    StandardNode.super.succ(pred, succ, c)
+  }
 }
 
 /** Defines the CFG by dispatch on the various AST types. */
@@ -274,7 +282,7 @@ module Trees {
     override predicate isHidden() { any() }
   }
 
-  class BeginBlockTree extends PreOrderTree, PostOrderTree, StandardNode, BeginBlock {
+  class BeginBlockTree extends ScopeTree, BeginBlock {
     final override AstNode getChildNode(int i) { result = this.getChild(i) }
   }
 
@@ -288,7 +296,7 @@ module Trees {
     }
   }
 
-  class BlockTree extends StandardNode, PreOrderTree, PostOrderTree, Block {
+  class BlockTree extends ScopeTree, Block {
     final override AstNode getChildNode(int i) {
       result = this.getParameters() and i = 0
       or
@@ -457,7 +465,7 @@ module Trees {
 
   private class EmptyStatementTree extends LeafTree, EmptyStatement { }
 
-  class EndBlockTree extends StandardNode, PreOrderTree, PostOrderTree, EndBlock {
+  class EndBlockTree extends ScopeTree, EndBlock {
     final override AstNode getChildNode(int i) { result = this.getChild(i) }
   }
 
@@ -679,12 +687,8 @@ module Trees {
     final override AstNode getDefaultValue() { result = this.getValue() }
   }
 
-  class LambdaTree extends PreOrderTree, PostOrderTree, Lambda {
-    final override predicate propagatesAbnormal(AstNode child) { child = this.getParameters() }
-
+  class LambdaTree extends LeafTree, Lambda {
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      this.getBody().(ControlFlowTree).succ(pred, succ, c)
-      or
       last(this.getParameters(), pred, c) and
       c instanceof NormalCompletion and
       (
diff --git a/ql/test/library-tests/controlflow/graph/Cfg.expected b/ql/test/library-tests/controlflow/graph/Cfg.expected
index ec9b794b989..8c8ea06a75c 100644
--- a/ql/test/library-tests/controlflow/graph/Cfg.expected
+++ b/ql/test/library-tests/controlflow/graph/Cfg.expected
@@ -3683,7 +3683,6 @@ raise.rb:
 #-----|  -> exit block (normal)
 
 #  155| Call
-#-----| raise -> exit m14 (abnormal)
 #-----| raise -> exit block (abnormal)
 
 #  155| raise
@@ -4010,9 +4009,6 @@ raise.rb:
 #  150| exit m13 (normal)
 #-----|  -> exit m13
 
-#  154| exit m14 (abnormal)
-#-----|  -> exit m14
-
 #  154| exit m14 (normal)
 #-----|  -> exit m14