Shared CFG: fix nested and self-looping goto targets

Author: owen-mc

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll

@@ -162,6 +162,12 @@ module Ast implements AstSig<Location> {
 
   class Stmt = CS::Stmt;
 
+  class LabeledStmt extends Stmt {
+    LabeledStmt() { none() }
+
+    Stmt getStmt() { none() }
+  }
+
   class Expr = CS::Expr;
 
   class BlockStmt = CS::BlockStmt;

go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll

@@ -186,6 +186,8 @@ module CfgImpl {
 
     class Stmt = Go::Stmt;
 
+    class LabeledStmt = Go::LabeledStmt;
+
     class Expr = Go::Expr;
 
     class BlockStmt extends Go::BlockStmt {
@@ -434,29 +436,23 @@ module CfgImpl {
     }
 
     predicate hasLabel(Ast::AstNode n, Label l) {
-      // A statement carries the label of every `LabeledStmt` that wraps it.
-      // This is recursive because Go allows stacked labels (`L1: L2: stmt`),
-      // which the extractor represents as nested `LabeledStmt`s, so a single
-      // statement may have several labels.
-      exists(Go::LabeledStmt ls | n = ls.getStmt() | l = ls.getLabel() or hasLabel(ls, l))
-      or
-      // The `LabeledStmt` wrapper itself also carries its label. Blocks contain
-      // the wrapper (not the inner statement) as a direct child, so the shared
-      // library's block-level `goto` target resolution -- which looks for a
-      // labelled statement that is a direct child of a block -- matches on the
-      // wrapper.
       l = n.(Go::LabeledStmt).getLabel()
       or
       l = n.(Go::BreakStmt).getLabel()
       or
       l = n.(Go::ContinueStmt).getLabel()
       or
-      // A `goto` statement carries its target label, so that the shared
-      // library's `beginAbruptCompletion` produces a *labelled* goto completion
-      // (matching the target label) rather than an unlabelled one.
       l = n.(Go::GotoStmt).getLabel()
     }
 
+    private predicate hasLabelOrEnclosingLabel(Ast::AstNode n, Label l) {
+      hasLabel(n, l)
+      or
+      exists(Go::LabeledStmt labeled |
+        labeled.getStmt() = n and hasLabelOrEnclosingLabel(labeled, l)
+      )
+    }
+
     predicate preOrderExpr(Ast::Expr e) {
       // The call of a `defer` statement is not invoked at the statement
       // itself; its callee expression and arguments are evaluated in place,
@@ -799,13 +795,6 @@ module CfgImpl {
       n.isAdditional(ast, "catch-return") and
       c.getSuccessorType() instanceof ReturnSuccessor
       or
-      exists(Go::LabeledStmt lbl |
-        ast = lbl.getStmt() and
-        n.isAfter(lbl) and
-        c.getSuccessorType() instanceof BreakSuccessor and
-        c.hasLabel(lbl.getLabel())
-      )
-      or
       // A `break` in a communication clause body terminates the enclosing
       // `select` statement, continuing after it. This mirrors the shared
       // library's handling of `break` in a `switch` case body, but `select` is
@@ -823,7 +812,7 @@ module CfgImpl {
       |
         not c.hasLabel(_)
         or
-        exists(Label l | c.hasLabel(l) and hasLabel(sel, l))
+        exists(Label l | c.hasLabel(l) and hasLabelOrEnclosingLabel(sel, l))
       )
       or
       exists(Go::FuncDef fd |
@@ -835,18 +824,6 @@ module CfgImpl {
         exists(fd.getResultVar(0)) and
         n.isAdditional(fd.getBody(), "result-read:0")
       )
-      or
-      // Function bodies are excluded from `Ast::BlockStmt`, so handle goto
-      // targets among their top-level statements here.
-      exists(Go::FuncDef fd, Go::Stmt target, Label l |
-        ast = fd.getBody() and
-        target = fd.getBody().getAStmt() and
-        not target instanceof Go::GotoStmt and
-        hasLabel(target, l) and
-        n.isBefore(target) and
-        c.getSuccessorType() instanceof GotoSuccessor and
-        c.hasLabel(l)
-      )
     }
 
     /** Holds if `ast` or one of its CFG children may panic. */

go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.expected

@@ -0,0 +1,19 @@
+import go
+import utils.test.InlineExpectationsTest
+
+module GotoTargetTest implements TestSig {
+  string getARelevantTag() { result = "gotoTarget" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(GotoStmt jump, LabeledStmt target, ControlFlow::Node source |
+      jump.getLocation() = location and
+      source.getAstNode() = jump and
+      source.getASuccessor().getAstNode() = target and
+      element = jump.toString() and
+      tag = "gotoTarget" and
+      value = target.getLabel()
+    )
+  }
+}
+
+import MakeTest<GotoTargetTest>

go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.ql

@@ -0,0 +1,46 @@
+package main
+
+func gotoStackedSiblingTarget(flag bool) {
+	if flag {
+		goto inner // $ gotoTarget=inner
+	}
+outer:
+inner:
+	flag = false
+	goto outer // $ gotoTarget=outer
+}
+
+func gotoNestedSiblingTarget(flag bool) {
+	if flag {
+		goto inner // $ gotoTarget=inner
+	} else {
+		goto outer // $ gotoTarget=outer
+	}
+outer:
+inner:
+	{
+		flag = false
+	}
+}
+
+func gotoSelfLoop(flag bool) {
+self:
+	if flag {
+		goto self // $ gotoTarget=self
+	}
+}
+
+func gotoDirectSelfLoop() {
+self:
+	goto self // $ gotoTarget=self
+}
+
+func gotoEnclosingStackedLabel(flag bool) {
+outer:
+inner:
+	if flag {
+		goto inner // $ gotoTarget=inner
+	} else {
+		goto outer // $ gotoTarget=outer
+	}
+}

go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go

@@ -70,6 +70,8 @@ private module Ast implements AstSig<Location> {
 
   class Stmt = J::Stmt;
 
+  class LabeledStmt = J::LabeledStmt;
+
   class Expr = J::Expr;
 
   class BlockStmt = J::BlockStmt;
@@ -543,15 +545,8 @@ private module Input implements InputSig1, InputSig2 {
     }
   }
 
-  private Label getLabelOfLoop(Stmt s) {
-    exists(LabeledStmt l | s = l.getStmt() |
-      result = TJavaLabel(l.getLabel()) or
-      result = getLabelOfLoop(l)
-    )
-  }
-
   predicate hasLabel(Ast::AstNode n, Label l) {
-    l = getLabelOfLoop(n)
+    l = TJavaLabel(n.(LabeledStmt).getLabel())
     or
     l = TJavaLabel(n.(BreakStmt).getLabel())
     or
@@ -616,12 +611,7 @@ private module Input implements InputSig1, InputSig2 {
    * flow continuing at `n`.
    */
   predicate endAbruptCompletion(Ast::AstNode ast, PreControlFlowNode n, AbruptCompletion c) {
-    exists(LabeledStmt lbl |
-      ast = lbl.getStmt() and
-      n.isAfter(lbl) and
-      c.getSuccessorType() instanceof BreakSuccessor and
-      c.hasLabel(TJavaLabel(lbl.getLabel()))
-    )
+    none()
   }
 
   /** Holds if there is a local non-abrupt step from `n1` to `n2`. */

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

@@ -231,6 +231,12 @@ module Ast implements AstSig<Py::Location> {
     override Callable getEnclosingCallable() { result.asScope() = this.asStmt().getScope() }
   }
 
+  class LabeledStmt extends Stmt {
+    LabeledStmt() { none() }
+
+    Stmt getStmt() { none() }
+  }
+
   /** An expression. */
   class Expr extends AstNodeImpl, TExpr {
     // For `TPyExpr` instances, delegate to the wrapped Python expression.

python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll

@@ -308,6 +308,12 @@ private module Ast implements AstSig<Location> {
 
   class ContinueStmt extends Stmt instanceof R::Ast::NextStmt { }
 
+  class LabeledStmt extends Stmt {
+    LabeledStmt() { none() }
+
+    Stmt getStmt() { none() }
+  }
+
   class GotoStmt extends Stmt {
     GotoStmt() { none() }
   }

ruby/ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll

@@ -71,6 +71,12 @@ signature module AstSig<LocationSig Location> {
   /** A statement. */
   class Stmt extends AstNode;
 
+  /** A labeled statement. */
+  class LabeledStmt extends Stmt {
+    /** Gets the statement carrying the label. */
+    Stmt getStmt();
+  }
+
   /** An expression. */
   class Expr extends AstNode;
 
@@ -439,10 +445,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
       string toString();
     }
 
-    /**
-     * Holds if the node `n` has the label `l`. For example, a label in a goto
-     * statement or a goto target.
-     */
+    /** Holds if the node `n` directly has the label `l`. */
     default predicate hasLabel(AstNode n, Label l) { none() }
 
     /**
@@ -1282,9 +1285,23 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
         )
       }
 
-      private Stmt getAStmtInBlock(AstNode block) {
-        result = block.(BlockStmt).getStmt(_) or
-        result = block.(Switch).getStmt(_)
+      /** Holds if `n` has `l`, possibly through enclosing labeled statements. */
+      private predicate hasLabel(AstNode n, Input1::Label l) {
+        Input1::hasLabel(n, l)
+        or
+        exists(LabeledStmt labeled | labeled.getStmt() = n and hasLabel(labeled, l))
+      }
+
+      /**
+       * Holds if `target` is a labeled statement at the start of `root`,
+       * possibly nested under other labeled statements.
+       */
+      private predicate labeledTargetInRoot(Stmt root, LabeledStmt target) {
+        root = target
+        or
+        exists(LabeledStmt labeled |
+          root = labeled and labeledTargetInRoot(labeled.getStmt(), target)
+        )
       }
 
       private predicate callableHasParamDefault(Callable c, Expr defaultValue) {
@@ -1325,7 +1342,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
             or
             exists(Input1::Label l |
               c.hasLabel(l) and
-              Input1::hasLabel(loop, l)
+              hasLabel(loop, l)
             )
           )
         )
@@ -1365,16 +1382,32 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           or
           exists(Input1::Label l |
             c.hasLabel(l) and
-            Input1::hasLabel(switch, l)
+            hasLabel(switch, l)
           )
         )
         or
-        exists(AstNode block, Input1::Label l, Stmt lblstmt |
-          ast = getAStmtInBlock(block) and
-          lblstmt = getAStmtInBlock(block) and
-          not lblstmt instanceof GotoStmt and
-          Input1::hasLabel(pragma[only_bind_into](lblstmt), l) and
-          n.isBefore(lblstmt) and
+        exists(LabeledStmt target, Input1::Label l |
+          ast = target.getStmt() and
+          Input1::hasLabel(target, l) and
+          n.isAfter(target) and
+          c.getSuccessorType() instanceof BreakSuccessor and
+          c.hasLabel(l)
+        )
+        or
+        exists(AstNode parent, Stmt root, LabeledStmt target, Input1::Label l |
+          ast = getChild(parent, _) and
+          root = getChild(parent, _) and
+          labeledTargetInRoot(root, target) and
+          Input1::hasLabel(target, l) and
+          n.isBefore(target) and
+          c.getSuccessorType() instanceof GotoSuccessor and
+          c.hasLabel(l)
+        )
+        or
+        exists(LabeledStmt target, Input1::Label l |
+          ast = target.getStmt() and
+          Input1::hasLabel(target, l) and
+          n.isBefore(target) and
           c.getSuccessorType() instanceof GotoSuccessor and
           c.hasLabel(l)
         )

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

@@ -130,6 +130,12 @@ module Ast implements AstSig<Location> {
 
   class ContinueStmt = U::ContinueExpr;
 
+  class LabeledStmt extends Stmt {
+    LabeledStmt() { none() }
+
+    Stmt getStmt() { none() }
+  }
+
   class GotoStmt extends Stmt {
     GotoStmt() { none() }
   }