diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index d6e675c8ab9b..4ec8ff8450ac 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -132,27 +132,11 @@ private module Ast implements AstSig { result = this.(SwitchStmt).getCase(index) or result = this.(SwitchExpr).getCase(index) } - } - - 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) - } - 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 { @@ -166,32 +150,20 @@ 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 - or - result = this.(J::SwitchCase).getRuleStatement() and index = 0 + AstNode getBody() { + result = this.(J::SwitchCase).getRuleExpression() 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() } } - predicate fallsThrough(Case c) { not c.(J::SwitchCase).isRule() } + class DefaultCase extends Case instanceof J::DefaultCase { } class ConditionalExpr = J::ConditionalExpr; diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 56a909a006c0..ac7c5b59cfe2 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -224,17 +224,18 @@ signature module AstSig { /** Gets the case at the specified (zero-based) `index`. */ 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 } + /** + * 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 accommodates the latter. + */ + Stmt getStmt(int index); + } /** A case in a switch. */ class Case extends AstNode { @@ -245,19 +246,22 @@ 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; + /** * 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 { @@ -938,7 +942,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 @@ -1083,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 | @@ -1096,14 +1100,59 @@ 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) } + 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: @@ -1113,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: @@ -1467,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)) ) }