Merge pull request #16122 from hvitved/ruby/cfg-may-raise-issue

Ruby: Fix CFG for nodes that may raise

ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll

@@ -90,9 +90,7 @@ private predicate mayRaise(Call c) { c = getARescuableBodyChild() }
 
 /** A completion of a statement or an expression. */
 abstract class Completion extends TCompletion {
-  private predicate isValidForSpecific(AstNode n) {
-    exists(AstNode other | n = other.getDesugared() and this.isValidForSpecific(other))
-    or
+  private predicate isValidForSpecific0(AstNode n) {
     this = n.(NonReturningCall).getACompletion()
     or
     completionIsValidForStmt(n, this)
@@ -110,12 +108,19 @@ abstract class Completion extends TCompletion {
     or
     n = any(RescueModifierExpr parent).getBody() and
     this = [TSimpleCompletion().(TCompletion), TRaiseCompletion()]
+  }
+
+  private predicate isValidForSpecific(AstNode n) {
+    this.isValidForSpecific0(n)
+    or
+    exists(AstNode other | n = other.getDesugared() and this.isValidForSpecific(other))
     or
     mayRaise(n) and
     (
       this = TRaiseCompletion()
       or
-      this = TSimpleCompletion() and not n instanceof NonReturningCall
+      not any(Completion c).isValidForSpecific0(n) and
+      this = TSimpleCompletion()
     )
   }
 

ruby/ql/test/library-tests/controlflow/graph/Cfg.expected

@@ -2397,18 +2397,9 @@ cfg.rb:
 #   90| ...
 #-----|  -> $global
 
-#   90| ... = ...
-#-----|  -> if ...
-
 #   90| ... = ...
 #-----|  -> x
 
-#   90| [false] ! ...
-#-----| false -> if ...
-
-#   90| [true] ! ...
-#-----| true -> x
-
 #   90| __synth__0__1
 #-----|  -> x
 
@@ -2418,10 +2409,6 @@ cfg.rb:
 #   90| call to each
 #-----|  -> ...
 
-#   90| defined? ...
-#-----| false -> [true] ! ...
-#-----| true -> [false] ! ...
-
 #   90| enter { ... }
 #-----|  -> __synth__0__1
 
@@ -2430,6 +2417,22 @@ cfg.rb:
 #   90| exit { ... } (normal)
 #-----|  -> exit { ... }
 
+#   90| { ... }
+#-----|  -> call to each
+
+#   90| ... = ...
+#-----|  -> if ...
+
+#   90| [false] ! ...
+#-----| false -> if ...
+
+#   90| [true] ! ...
+#-----| true -> x
+
+#   90| defined? ...
+#-----| false -> [true] ! ...
+#-----| true -> [false] ! ...
+
 #   90| if ...
 #-----|  -> Array
 
@@ -2442,9 +2445,6 @@ cfg.rb:
 #   90| x
 #-----|  -> defined? ...
 
-#   90| { ... }
-#-----|  -> call to each
-
 #   90| x
 #-----|  -> __synth__0__1
 
@@ -6915,7 +6915,7 @@ raise.rb:
 #-----|  -> exit m
 
 #  167| m
-#-----|  -> exit raise.rb (normal)
+#-----|  -> m16
 
 #  167| self
 #-----|  -> m
@@ -6928,3 +6928,68 @@ raise.rb:
 
 #  168| ""
 #-----|  -> call to raise
+
+#  172| enter m16
+#-----|  -> b1
+
+#  172| exit m16
+
+#  172| exit m16 (abnormal)
+#-----|  -> exit m16
+
+#  172| exit m16 (normal)
+#-----|  -> exit m16
+
+#  172| m16
+#-----|  -> exit raise.rb (normal)
+
+#  172| b1
+#-----|  -> b2
+
+#  172| b2
+#-----|  -> b1
+
+#  174| b1
+#-----| true -> [true] ... || ...
+#-----| false -> b2
+
+#  174| [false] ... || ...
+#-----| false -> 2
+#-----| raise -> ExceptionA
+
+#  174| [true] ... || ...
+#-----| true -> 1
+#-----| raise -> ExceptionA
+
+#  174| b2
+#-----|  -> true
+
+#  174| ... == ...
+#-----| false -> [false] ... || ...
+#-----| true -> [true] ... || ...
+#-----| raise -> ExceptionA
+
+#  174| true
+#-----|  -> ... == ...
+
+#  175| return
+#-----| return -> exit m16 (normal)
+
+#  175| 1
+#-----|  -> return
+
+#  177| return
+#-----| return -> exit m16 (normal)
+
+#  177| 2
+#-----|  -> return
+
+#  179| ExceptionA
+#-----| raise -> exit m16 (abnormal)
+#-----| match -> 3
+
+#  180| return
+#-----| return -> exit m16 (normal)
+
+#  180| 3
+#-----|  -> return

ruby/ql/test/library-tests/controlflow/graph/Nodes.expected

@@ -367,6 +367,9 @@ positionalArguments
 | raise.rb:160:5:162:7 | call to bar | raise.rb:160:9:162:7 | -> { ... } |
 | raise.rb:161:7:161:14 | call to raise | raise.rb:161:13:161:14 | "" |
 | raise.rb:168:5:168:12 | call to raise | raise.rb:168:11:168:12 | "" |
+| raise.rb:174:8:174:23 | [false] ... \|\| ... | raise.rb:174:14:174:23 | ... == ... |
+| raise.rb:174:8:174:23 | [true] ... \|\| ... | raise.rb:174:14:174:23 | ... == ... |
+| raise.rb:174:14:174:23 | ... == ... | raise.rb:174:20:174:23 | true |
 keywordArguments
 | cfg.html.erb:6:9:6:58 | call to stylesheet_link_tag | media | cfg.html.erb:6:54:6:58 | "all" |
 | cfg.html.erb:12:11:12:33 | call to link_to | id | cfg.html.erb:12:31:12:33 | "a" |

ruby/ql/test/library-tests/controlflow/graph/ifs.rb

@@ -55,4 +55,4 @@ def disjunct (b1, b2)
   if (b1 || b2) then
     puts "b1 or b2"
   end
-end
+end

ruby/ql/test/library-tests/controlflow/graph/raise.rb

@@ -168,3 +168,15 @@ class C
     raise ""
   end
 end
+
+def m16(b1, b2)
+  begin
+    if b1 || b2 == true
+      return 1
+    else
+      return 2
+    end
+  rescue ExceptionA
+    return 3
+  end
+end