From 35ac66d3aa591a3521ba6a2ea2470acf0ee0e1a0 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 10 Mar 2026 10:39:32 +0100 Subject: [PATCH 1/4] Cfg: Move getCaseControlFlowOrder to shared code. --- .../lib/semmle/code/java/ControlFlowGraph.qll | 11 ++----- .../codeql/controlflow/ControlFlowGraph.qll | 29 ++++++++++++------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index d6e675c8ab9b..26672a22ed57 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -134,15 +134,6 @@ private module Ast implements AstSig { } } - int getCaseControlFlowOrder(Switch s, Case c) { - exists(int pos | s.getCase(pos) = c | - // if a default case is not last in the AST, move it last in the CFG order - if c instanceof DefaultCase and exists(s.getCase(pos + 1)) - then result = strictcount(s.getCase(_)) - else result = pos - ) - } - private Stmt getSwitchStmt(Switch s, int i) { result = s.(SwitchStmt).getStmt(i) or result = s.(SwitchExpr).getStmt(i) @@ -191,6 +182,8 @@ private module Ast implements AstSig { } } + class DefaultCase extends Case instanceof J::DefaultCase { } + predicate fallsThrough(Case c) { not c.(J::SwitchCase).isRule() } class ConditionalExpr = J::ConditionalExpr; diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 56a909a006c0..1d070564f1cd 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -226,16 +226,6 @@ signature module AstSig { Case getCase(int index); } - /** - * Gets an integer indicating the control flow order of a case within a switch. - * This is most often the same as the AST order, but can be different in some - * languages if the language allows a default case to appear before other - * cases. - * - * The values do not need to be contiguous; only the relative ordering matters. - */ - default int getCaseControlFlowOrder(Switch s, Case c) { s.getCase(result) = c } - /** A case in a switch. */ class Case extends AstNode { /** Gets a pattern being matched by this case. */ @@ -253,6 +243,8 @@ signature module AstSig { AstNode getBodyElement(int index); } + class DefaultCase extends Case; + /** * Holds if this case can fall through to the next case if it is not * otherwise prevented with a `break` or similar. @@ -938,7 +930,7 @@ module Make0 Ast> { * * A match-all case can still ultimately fail to match if it has a guard. */ - default predicate matchAll(Case c) { none() } + default predicate matchAll(Case c) { c instanceof DefaultCase } /** * Holds if `ast` may result in an abrupt completion `c` originating at @@ -1096,6 +1088,21 @@ module Make0 Ast> { ) } + /** + * Gets an integer indicating the control flow order of a case within a + * switch. This is equal to the AST order, except that default cases are + * always last in control flow order, even if some languages allow them + * to appear before other cases in the AST. + */ + private int getCaseControlFlowOrder(Switch s, Case c) { + exists(int pos | s.getCase(pos) = c | + // if a default case is not last in the AST, move it last in the CFG order + if c instanceof DefaultCase and exists(s.getCase(pos + 1)) + then result = strictcount(s.getCase(_)) + else result = pos + ) + } + private Case getRankedCaseCfgOrder(Switch s, int rnk) { result = rank[rnk](Case c, int i | getCaseControlFlowOrder(s, c) = i | c order by i) } From edf88b34da6c7bb1671804fb54661e5c38735c5b Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 10 Mar 2026 11:02:58 +0100 Subject: [PATCH 2/4] Cfg: Move Case.getBodyElement to shared code. --- .../lib/semmle/code/java/ControlFlowGraph.qll | 41 ++++-------- .../codeql/controlflow/ControlFlowGraph.qll | 64 +++++++++++++++---- 2 files changed, 64 insertions(+), 41 deletions(-) diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index 26672a22ed57..a2881e62e70a 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -132,18 +132,11 @@ private module Ast implements AstSig { result = this.(SwitchStmt).getCase(index) or result = this.(SwitchExpr).getCase(index) } - } - - private Stmt getSwitchStmt(Switch s, int i) { - result = s.(SwitchStmt).getStmt(i) or - result = s.(SwitchExpr).getStmt(i) - } - private int numberOfStmts(Switch s) { result = strictcount(getSwitchStmt(s, _)) } - - private predicate caseIndex(Switch s, Case c, int caseIdx, int caseStmtPos) { - c = s.getCase(caseIdx) and - c = getSwitchStmt(s, caseStmtPos) + Stmt getStmt(int index) { + result = this.(SwitchStmt).getStmt(index) or + result = this.(SwitchExpr).getStmt(index) + } } class Case extends AstNode instanceof J::SwitchCase { @@ -157,28 +150,16 @@ private module Ast implements AstSig { Expr getGuard() { result = this.(PatternCase).getGuard() } /** - * Gets the body element of this case at the specified (zero-based) `index`. + * Gets the body of this case, if any. * - * This is either unique when the case has a single right-hand side, or it - * is the sequence of statements between this case and the next case. + * A case can either have a body as a single child AST node given by this + * predicate, or it can have an implicit body given by the sequence of + * statements between this case and the next case. */ - AstNode getBodyElement(int index) { - result = this.(J::SwitchCase).getRuleExpression() and index = 0 + AstNode getBody() { + result = this.(J::SwitchCase).getRuleExpression() or - result = this.(J::SwitchCase).getRuleStatement() and index = 0 - or - not this.(J::SwitchCase).isRule() and - exists(Switch s, int caseIdx, int caseStmtPos, int nextCaseStmtPos | - caseIndex(pragma[only_bind_into](s), this, caseIdx, caseStmtPos) and - ( - caseIndex(pragma[only_bind_into](s), _, caseIdx + 1, nextCaseStmtPos) - or - not exists(s.getCase(caseIdx + 1)) and - nextCaseStmtPos = numberOfStmts(s) - ) and - index = [0 .. nextCaseStmtPos - caseStmtPos - 2] and - result = getSwitchStmt(pragma[only_bind_into](s), caseStmtPos + 1 + index) - ) + result = this.(J::SwitchCase).getRuleStatement() } } diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 1d070564f1cd..f8a4b903579b 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -224,6 +224,17 @@ signature module AstSig { /** Gets the case at the specified (zero-based) `index`. */ Case getCase(int index); + + /** + * Gets the statement at the specified (zero-based) `index`, if any. + * + * Depending on the language, switches may have their case bodies nested + * under the case nodes, which may or may not be statements themselves, or + * the switches may have a flat structure where the cases are just labels + * and the case bodies are sequences of statements between case statements. + * This predicate accomodates the latter. + */ + Stmt getStmt(int index); } /** A case in a switch. */ @@ -235,12 +246,13 @@ signature module AstSig { Expr getGuard(); /** - * Gets the body element of this case at the specified (zero-based) `index`. + * Gets the body of this case, if any. * - * This is either unique when the case has a single right-hand side, or it - * is the sequence of statements between this case and the next case. + * A case can either have a body as a single child AST node given by this + * predicate, or it can have an implicit body given by the sequence of + * statements between this case and the next case. */ - AstNode getBodyElement(int index); + AstNode getBody(); } class DefaultCase extends Case; @@ -1075,7 +1087,7 @@ module Make0 Ast> { ) or exists(Switch switch | - ast = switch.getCase(_).getBodyElement(_) and + ast = getCaseBodyElement(switch.getCase(_), _) and n.isAfter(switch) and c.getSuccessorType() instanceof BreakSuccessor | @@ -1107,10 +1119,40 @@ module Make0 Ast> { result = rank[rnk](Case c, int i | getCaseControlFlowOrder(s, c) = i | c order by i) } + private int numberOfStmts(Switch s) { result = strictcount(s.getStmt(_)) } + + private predicate caseIndex(Switch s, Case c, int caseIdx, int caseStmtPos) { + c = s.getCase(caseIdx) and + c = s.getStmt(caseStmtPos) + } + + /** + * Gets the body element of `case` at the specified (zero-based) `index`. + * + * This is either unique when the case has a single right-hand side, or it + * is the sequence of statements between this case and the next case. + */ + private AstNode getCaseBodyElement(Case case, int index) { + result = case.getBody() and index = 0 + or + not exists(case.getBody()) and + exists(Switch s, int caseIdx, int caseStmtPos, int nextCaseStmtPos | + caseIndex(pragma[only_bind_into](s), case, caseIdx, caseStmtPos) and + ( + caseIndex(pragma[only_bind_into](s), _, caseIdx + 1, nextCaseStmtPos) + or + not exists(s.getCase(caseIdx + 1)) and + nextCaseStmtPos = numberOfStmts(s) + ) and + index = [0 .. nextCaseStmtPos - caseStmtPos - 2] and + result = pragma[only_bind_into](s).getStmt(caseStmtPos + 1 + index) + ) + } + private AstNode getFirstCaseBodyElement(Case case) { - result = case.getBodyElement(0) + result = getCaseBodyElement(case, 0) or - not exists(case.getBodyElement(0)) and + not exists(getCaseBodyElement(case, 0)) and exists(Switch s, int i | fallsThrough(case) and // fall-through follows AST order, not case control flow order: @@ -1120,10 +1162,10 @@ module Make0 Ast> { } private AstNode getNextCaseBodyElement(AstNode bodyElement) { - exists(Case case, int i | case.getBodyElement(i) = bodyElement | - result = case.getBodyElement(i + 1) + exists(Case case, int i | getCaseBodyElement(case, i) = bodyElement | + result = getCaseBodyElement(case, i + 1) or - not exists(case.getBodyElement(i + 1)) and + not exists(getCaseBodyElement(case, i + 1)) and exists(Switch s, int j | fallsThrough(case) and // fall-through follows AST order, not case control flow order: @@ -1474,7 +1516,7 @@ module Make0 Ast> { or n1.isAfter(caseBodyElement) and not exists(getNextCaseBodyElement(caseBodyElement)) and - n2.isAfter(any(Switch s | s.getCase(_).getBodyElement(_) = caseBodyElement)) + n2.isAfter(any(Switch s | getCaseBodyElement(s.getCase(_), _) = caseBodyElement)) ) } From 77d4f5a2dc0ce980c7a58afd6cd2d242e47bbfc1 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 10 Mar 2026 11:10:24 +0100 Subject: [PATCH 3/4] Cfg: Update fallsThrough default. --- java/ql/lib/semmle/code/java/ControlFlowGraph.qll | 2 -- shared/controlflow/codeql/controlflow/ControlFlowGraph.qll | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index a2881e62e70a..4ec8ff8450ac 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -165,8 +165,6 @@ private module Ast implements AstSig { class DefaultCase extends Case instanceof J::DefaultCase { } - predicate fallsThrough(Case c) { not c.(J::SwitchCase).isRule() } - class ConditionalExpr = J::ConditionalExpr; class BinaryExpr = J::BinaryExpr; diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index f8a4b903579b..0e53ea4d93b5 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -261,7 +261,7 @@ signature module AstSig { * Holds if this case can fall through to the next case if it is not * otherwise prevented with a `break` or similar. */ - default predicate fallsThrough(Case c) { none() } + default predicate fallsThrough(Case c) { not exists(c.getBody()) } /** A ternary conditional expression. */ class ConditionalExpr extends Expr { From efa797a21d7b896dfc772bf0a6b9ebc08a1d6530 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 10 Mar 2026 11:22:15 +0100 Subject: [PATCH 4/4] Update shared/controlflow/codeql/controlflow/ControlFlowGraph.qll Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- shared/controlflow/codeql/controlflow/ControlFlowGraph.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 0e53ea4d93b5..ac7c5b59cfe2 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -232,7 +232,7 @@ signature module AstSig { * under the case nodes, which may or may not be statements themselves, or * the switches may have a flat structure where the cases are just labels * and the case bodies are sequences of statements between case statements. - * This predicate accomodates the latter. + * This predicate accommodates the latter. */ Stmt getStmt(int index); }