From fc044f4cda249ad6971ced0f191ecdf214d8ba6a Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 30 Sep 2026 16:56:06 +0100 Subject: [PATCH 1/3] Shared CFG: fix nested and self-looping goto targets --- .../controlflow/internal/ControlFlowGraph.qll | 2 + .../go/controlflow/ControlFlowGraphImpl.qll | 45 ++++--------- .../GotoTarget/GotoTarget.expected | 0 .../go/controlflow/GotoTarget/GotoTarget.ql | 19 ++++++ .../semmle/go/controlflow/GotoTarget/gotos.go | 46 ++++++++++++++ .../lib/semmle/code/java/ControlFlowGraph.qll | 18 ++---- .../controlflow/internal/AstNodeImpl.qll | 6 ++ .../ruby/controlflow/ControlFlowGraph.qll | 6 ++ .../codeql/controlflow/ControlFlowGraph.qll | 63 ++++++++++++++----- .../unified/internal/ControlFlowGraph.qll | 6 ++ 10 files changed, 148 insertions(+), 63 deletions(-) create mode 100644 go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.expected create mode 100644 go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.ql create mode 100644 go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll index b813d58875c1..60051b7e6c3c 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll @@ -162,6 +162,8 @@ module Ast implements AstSig { class Stmt = CS::Stmt; + class LabeledStmt = CS::LabelStmt; + class Expr = CS::Expr; class BlockStmt = CS::BlockStmt; diff --git a/go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll b/go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll index 1eb9a84f50fb..ea2133d77647 100644 --- a/go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll +++ b/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, @@ -800,13 +796,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 @@ -824,7 +813,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 | @@ -836,18 +825,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. */ diff --git a/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.expected b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.ql b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.ql new file mode 100644 index 000000000000..4b0a288bcf98 --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/GotoTarget.ql @@ -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 diff --git a/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go new file mode 100644 index 000000000000..7370a4974079 --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go @@ -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 + } +} diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index 1281afc52728..dce6ba6afd03 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -70,6 +70,8 @@ private module Ast implements AstSig { 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`. */ diff --git a/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll b/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll index 1747b6cdd1a0..802cf2cf79f0 100644 --- a/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll +++ b/python/ql/lib/semmle/python/controlflow/internal/AstNodeImpl.qll @@ -231,6 +231,12 @@ module Ast implements AstSig { 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. diff --git a/ruby/ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll b/ruby/ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll index 419f840290c4..8f0ecd549991 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/ControlFlowGraph.qll @@ -308,6 +308,12 @@ private module Ast implements AstSig { class ContinueStmt extends Stmt instanceof R::Ast::NextStmt { } + class LabeledStmt extends Stmt { + LabeledStmt() { none() } + + Stmt getStmt() { none() } + } + class GotoStmt extends Stmt { GotoStmt() { none() } } diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 3b1ba8887584..9b8b01e20ab6 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -71,6 +71,12 @@ signature module AstSig { /** 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 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 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 Ast> { or exists(Input1::Label l | c.hasLabel(l) and - Input1::hasLabel(loop, l) + hasLabel(loop, l) ) ) ) @@ -1365,16 +1382,32 @@ module Make0 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) ) diff --git a/unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll b/unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll index a01b95244c39..9a810d930f47 100644 --- a/unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll +++ b/unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll @@ -130,6 +130,12 @@ module Ast implements AstSig { class ContinueStmt = U::ContinueExpr; + class LabeledStmt extends Stmt { + LabeledStmt() { none() } + + Stmt getStmt() { none() } + } + class GotoStmt extends Stmt { GotoStmt() { none() } } From e7691bea4f3d9094e13be2f0d092ebbe1089baa2 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 1 Oct 2026 14:12:10 +0100 Subject: [PATCH 2/3] Address labeled CFG review feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../go/controlflow/ControlFlowGraphImpl.qll | 22 ++--------- .../semmle/go/controlflow/GotoTarget/gotos.go | 7 ++++ .../codeql/controlflow/ControlFlowGraph.qll | 38 ++++--------------- 3 files changed, 18 insertions(+), 49 deletions(-) diff --git a/go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll b/go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll index ea2133d77647..9d73c09a2a75 100644 --- a/go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll +++ b/go/ql/lib/semmle/go/controlflow/ControlFlowGraphImpl.qll @@ -445,14 +445,6 @@ module CfgImpl { 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, @@ -796,15 +788,9 @@ module CfgImpl { n.isAdditional(ast, "catch-return") and c.getSuccessorType() instanceof ReturnSuccessor 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 - // modeled language-specifically (it is not a `Switch`), so the break - // must be caught here. The break completion bubbles up the AST until it - // reaches a top-level statement of the comm clause body, at which point - // flow resumes after the `select`. An unlabeled `break` targets the - // innermost enclosing construct; a labeled `break` only targets this - // `select` if it (or a `LabeledStmt` wrapping it) carries that label. + // An unlabeled `break` in a communication clause body terminates the + // enclosing `select`. Labeled breaks are handled by the shared + // `LabeledStmt` logic. exists(Go::SelectStmt sel, Go::CommClause cc | cc = sel.getACommClause() and ast = cc.getStmt(_) and @@ -812,8 +798,6 @@ module CfgImpl { c.getSuccessorType() instanceof BreakSuccessor | not c.hasLabel(_) - or - exists(Label l | c.hasLabel(l) and hasLabelOrEnclosingLabel(sel, l)) ) or exists(Go::FuncDef fd | diff --git a/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go index 7370a4974079..48f2b170e58c 100644 --- a/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go +++ b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go @@ -35,6 +35,13 @@ self: goto self // $ gotoTarget=self } +func gotoDirectTarget() { +step1: + goto step2 // $ gotoTarget=step2 +step2: + goto step1 // $ gotoTarget=step1 +} + func gotoEnclosingStackedLabel(flag bool) { outer: inner: diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 9b8b01e20ab6..4b61d8f462c3 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -1285,23 +1285,9 @@ module Make0 Ast> { ) } - /** 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) - ) + /** Holds if `n` is marked with a `LabeledStmt` with label `l`. */ + private predicate hasEnclosingLabel(AstNode n, Input1::Label l) { + exists(LabeledStmt labeled | labeled.getStmt+() = n and Input1::hasLabel(labeled, l)) } private predicate callableHasParamDefault(Callable c, Expr defaultValue) { @@ -1342,7 +1328,7 @@ module Make0 Ast> { or exists(Input1::Label l | c.hasLabel(l) and - hasLabel(loop, l) + hasEnclosingLabel(loop, l) ) ) ) @@ -1382,7 +1368,7 @@ module Make0 Ast> { or exists(Input1::Label l | c.hasLabel(l) and - hasLabel(switch, l) + hasEnclosingLabel(switch, l) ) ) or @@ -1394,19 +1380,11 @@ module Make0 Ast> { c.hasLabel(l) ) or - exists(AstNode parent, Stmt root, LabeledStmt target, Input1::Label l | + exists(AstNode parent, LabeledStmt 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 + root.getStmt*() = target and + Input1::hasLabel(pragma[only_bind_into](target), l) and n.isBefore(target) and c.getSuccessorType() instanceof GotoSuccessor and c.hasLabel(l) From f851af0876a159756096684ca0e36b74eabe2191 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 1 Oct 2026 16:13:43 +0100 Subject: [PATCH 3/3] Preserve direct labels in shared CFG Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- shared/controlflow/codeql/controlflow/ControlFlowGraph.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 4b61d8f462c3..99c7db99416b 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -1328,7 +1328,7 @@ module Make0 Ast> { or exists(Input1::Label l | c.hasLabel(l) and - hasEnclosingLabel(loop, l) + (Input1::hasLabel(loop, l) or hasEnclosingLabel(loop, l)) ) ) ) @@ -1368,7 +1368,7 @@ module Make0 Ast> { or exists(Input1::Label l | c.hasLabel(l) and - hasEnclosingLabel(switch, l) + (Input1::hasLabel(switch, l) or hasEnclosingLabel(switch, l)) ) ) or