diff --git a/go/ql/lib/semmle/go/controlflow/ControlFlowGraphShared.qll b/go/ql/lib/semmle/go/controlflow/ControlFlowGraphShared.qll index 8cc4f446bb0..47160577aff 100644 --- a/go/ql/lib/semmle/go/controlflow/ControlFlowGraphShared.qll +++ b/go/ql/lib/semmle/go/controlflow/ControlFlowGraphShared.qll @@ -785,6 +785,26 @@ module GoCfg { 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 + // modelled 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. + exists(Go::SelectStmt sel, Go::CommClause cc | + cc = sel.getACommClause() and + ast = cc.getStmt(_) and + n.isAfter(sel) and + c.getSuccessorType() instanceof BreakSuccessor + | + not c.hasLabel(_) + or + exists(Label l | c.hasLabel(l) and hasLabel(sel, l)) + ) + or exists(Go::FuncDef fd | ast = fd.getBody() and not funcHasDefer(fd) and @@ -1482,20 +1502,43 @@ module GoCfg { ) } + /** + * Holds if there is a control-flow step from `n1` to `n2` for the + * communication operation of a comm clause of `sel` that has been selected. + * + * The channel operands (and, for a send, the value) of every clause are + * evaluated up front in the prep phase (see `selectCommPrepStart` and + * friends), and the `select` then non-deterministically dispatches to one + * clause via `In(sel) -> Before(cc)`. The communication node itself + * (`In(recv.getExpr())` for a receive, `In(send)` for a send) is therefore + * only reached through that dispatch, never by ordinary left-to-right + * evaluation of the clause. + * + * The `Before(...) -> ...` edges below are deliberately dead (their source + * nodes are unreachable): they exist solely to suppress the shared + * library's default left-to-right child sequencing for these AST nodes, + * which is keyed off the presence of an explicit step out of a node's + * `Before` node. Without them, the default sequencing would, for example, + * wire `After(channel) -> In(send)`, spuriously connecting the prep phase + * straight to the communication and bypassing the dispatch. + */ private predicate selectedCommStep( Go::SelectStmt sel, PreControlFlowNode n1, PreControlFlowNode n2 ) { exists(Go::RecvStmt recv | recv = sel.getACommClause().getComm() | + // Suppress default sequencing of the receive statement and of the + // receive expression (both source nodes are unreachable). n1.isBefore(recv) and n2.isIn(recv.getExpr()) or n1.isBefore(recv.getExpr()) and n2.isBefore(recv.getExpr().getOperand()) ) or exists(Go::SendStmt send | send = sel.getACommClause().getComm() | + // Suppress default sequencing of the send statement (source unreachable). n1.isBefore(send) and n2.isBefore(send.getChannel()) or - n1.isAfter(send.getChannel()) and n2.isBefore(send.getValue()) - or + // The send communication happens at `In(send)`; flow then continues to + // the clause body via `selectStmt`. n1.isIn(send) and n2.isAfter(send) ) } diff --git a/go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected b/go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected index f82c68778ae..060fb1edd69 100644 --- a/go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected +++ b/go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected @@ -2780,6 +2780,7 @@ | stmts2.go:21:6:21:6 | y | stmts2.go:21:6:21:6 | After y [true] | | stmts2.go:21:8:23:3 | block statement | stmts2.go:22:4:22:8 | Before break statement | | stmts2.go:22:4:22:8 | Before break statement | stmts2.go:22:4:22:8 | break statement | +| stmts2.go:22:4:22:8 | break statement | stmts2.go:16:2:26:2 | After select statement | | stmts2.go:24:3:24:10 | Before return statement | stmts2.go:24:10:24:10 | Before 0 | | stmts2.go:24:3:24:10 | return statement | stmts2.go:15:1:28:1 | Normal Exit | | stmts2.go:24:10:24:10 | 0 | stmts2.go:24:10:24:10 | After 0 |