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..9d73c09a2a75 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,26 +436,12 @@ 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() } @@ -800,22 +788,9 @@ 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 - // 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 @@ -823,8 +798,6 @@ module CfgImpl { c.getSuccessorType() instanceof BreakSuccessor | not c.hasLabel(_) - or - exists(Label l | c.hasLabel(l) and hasLabel(sel, l)) ) or exists(Go::FuncDef fd | @@ -836,18 +809,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..48f2b170e58c --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/controlflow/GotoTarget/gotos.go @@ -0,0 +1,53 @@ +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 gotoDirectTarget() { +step1: + goto step2 // $ gotoTarget=step2 +step2: + goto step1 // $ gotoTarget=step1 +} + +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..99c7db99416b 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,9 @@ module Make0 Ast> { ) } - private Stmt getAStmtInBlock(AstNode block) { - result = block.(BlockStmt).getStmt(_) or - result = block.(Switch).getStmt(_) + /** 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) { @@ -1325,7 +1328,7 @@ module Make0 Ast> { or exists(Input1::Label l | c.hasLabel(l) and - Input1::hasLabel(loop, l) + (Input1::hasLabel(loop, l) or hasEnclosingLabel(loop, l)) ) ) ) @@ -1365,16 +1368,24 @@ module Make0 Ast> { or exists(Input1::Label l | c.hasLabel(l) and - Input1::hasLabel(switch, l) + (Input1::hasLabel(switch, l) or hasEnclosingLabel(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, LabeledStmt root, LabeledStmt target, Input1::Label l | + ast = getChild(parent, _) and + root = getChild(parent, _) 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) ) 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() } }