diff --git a/java/ql/lib/change-notes/2022-03-28-JumpStmt-as-superclass.md b/java/ql/lib/change-notes/2022-03-28-JumpStmt-as-superclass.md new file mode 100644 index 000000000000..e2e97a847425 --- /dev/null +++ b/java/ql/lib/change-notes/2022-03-28-JumpStmt-as-superclass.md @@ -0,0 +1,4 @@ +--- +category: fix +--- +* The QL class `JumpStmt` has been made the superclass of `BreakStmt`, `ContinueStmt` and `YieldStmt`. This allows directly using its inherited predicates without having to explicitly cast to `JumpStmt` first. diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 8f77b1800a2f..b154595abc51 100755 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -1399,7 +1399,7 @@ class SwitchExpr extends Expr, StmtParent, @switchexpr { Expr getAResult() { result = this.getACase().getRuleExpression() or - exists(YieldStmt yield | yield.(JumpStmt).getTarget() = this and result = yield.getValue()) + exists(YieldStmt yield | yield.getTarget() = this and result = yield.getValue()) } /** Gets a printable representation of this expression. */ diff --git a/java/ql/lib/semmle/code/java/Statement.qll b/java/ql/lib/semmle/code/java/Statement.qll index 3658033b8243..f842dea58184 100755 --- a/java/ql/lib/semmle/code/java/Statement.qll +++ b/java/ql/lib/semmle/code/java/Statement.qll @@ -556,14 +556,10 @@ class ThrowStmt extends Stmt, @throwstmt { override string getAPrimaryQlClass() { result = "ThrowStmt" } } -/** A `break`, `yield` or `continue` statement. */ -class JumpStmt extends Stmt { - JumpStmt() { - this instanceof BreakStmt or - this instanceof YieldStmt or - this instanceof ContinueStmt - } +private class JumpStmt_ = @breakstmt or @yieldstmt or @continuestmt; +/** A `break`, `yield` or `continue` statement. */ +class JumpStmt extends Stmt, JumpStmt_ { /** * Gets the labeled statement that this `break` or * `continue` statement refers to, if any. @@ -584,12 +580,7 @@ class JumpStmt extends Stmt { ) } - private SwitchExpr getSwitchExprTarget() { result = this.(YieldStmt).getParent+() } - private StmtParent getEnclosingTarget() { - result = this.getSwitchExprTarget() - or - not exists(this.getSwitchExprTarget()) and result = this.getAPotentialTarget() and not exists(Stmt other | other = this.getAPotentialTarget() | other.getEnclosingStmt+() = result) } @@ -598,6 +589,7 @@ class JumpStmt extends Stmt { * Gets the statement or `switch` expression that this `break`, `yield` or `continue` jumps to. */ StmtParent getTarget() { + // Note: This implementation only considers `break` and `continue`; YieldStmt overrides this predicate result = this.getLabelTarget() or not exists(this.getLabelTarget()) and result = this.getEnclosingTarget() @@ -605,7 +597,7 @@ class JumpStmt extends Stmt { } /** A `break` statement. */ -class BreakStmt extends Stmt, @breakstmt { +class BreakStmt extends JumpStmt, @breakstmt { /** Gets the label targeted by this `break` statement, if any. */ string getLabel() { namestrings(result, _, this) } @@ -626,12 +618,21 @@ class BreakStmt extends Stmt, @breakstmt { /** * A `yield` statement. */ -class YieldStmt extends Stmt, @yieldstmt { +class YieldStmt extends JumpStmt, @yieldstmt { /** * Gets the value of this `yield` statement. */ Expr getValue() { result.getParent() = this } + /** + * Gets the `switch` expression target of this `yield` statement. + */ + override SwitchExpr getTarget() { + // Get the innermost enclosing SwitchExpr; this works because getParent() is defined for Stmt and + // therefore won't proceed after the innermost SwitchExpr (due to it being an Expr) + result = this.getParent+() + } + override string pp() { result = "yield ..." } override string toString() { result = "yield ..." } @@ -642,7 +643,7 @@ class YieldStmt extends Stmt, @yieldstmt { } /** A `continue` statement. */ -class ContinueStmt extends Stmt, @continuestmt { +class ContinueStmt extends JumpStmt, @continuestmt { /** Gets the label targeted by this `continue` statement, if any. */ string getLabel() { namestrings(result, _, this) } diff --git a/java/ql/src/Likely Bugs/Statements/ContinueInFalseLoop.ql b/java/ql/src/Likely Bugs/Statements/ContinueInFalseLoop.ql index e6095791ddb8..b99f64a1e4de 100644 --- a/java/ql/src/Likely Bugs/Statements/ContinueInFalseLoop.ql +++ b/java/ql/src/Likely Bugs/Statements/ContinueInFalseLoop.ql @@ -16,6 +16,6 @@ import java from DoStmt do, ContinueStmt continue where do.getCondition().(BooleanLiteral).getBooleanValue() = false and - continue.(JumpStmt).getTarget() = do + continue.getTarget() = do select continue, "This 'continue' never re-runs the loop - the $@ is always false.", do.getCondition(), "loop condition" diff --git a/java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql b/java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql index 3d0b45dabedf..0b3c12560501 100644 --- a/java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql +++ b/java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql @@ -32,7 +32,7 @@ predicate loopExit(LoopStmt loop, Stmt exit) { exit.getEnclosingStmt*() = loop.getBody() and ( exit instanceof ReturnStmt or - exit.(BreakStmt).(JumpStmt).getTarget() = loop.getEnclosingStmt*() + exit.(BreakStmt).getTarget() = loop.getEnclosingStmt*() ) } diff --git a/java/ql/src/Security/CWE/CWE-129/ArraySizing.qll b/java/ql/src/Security/CWE/CWE-129/ArraySizing.qll index d8725b930a97..972485c6cd6f 100644 --- a/java/ql/src/Security/CWE/CWE-129/ArraySizing.qll +++ b/java/ql/src/Security/CWE/CWE-129/ArraySizing.qll @@ -44,11 +44,11 @@ class PointlessLoop extends WhileStmt { PointlessLoop() { this.getCondition().(BooleanLiteral).getBooleanValue() = true and // The only `break` must be the last statement. - forall(BreakStmt break | break.(JumpStmt).getTarget() = this | + forall(BreakStmt break | break.getTarget() = this | this.getStmt().(BlockStmt).getLastStmt() = break ) and // No `continue` statements. - not exists(ContinueStmt continue | continue.(JumpStmt).getTarget() = this) + not exists(ContinueStmt continue | continue.getTarget() = this) } } diff --git a/java/ql/src/Security/CWE/CWE-835/InfiniteLoop.ql b/java/ql/src/Security/CWE/CWE-835/InfiniteLoop.ql index 35f951d6d529..a04cfd6ac43f 100644 --- a/java/ql/src/Security/CWE/CWE-835/InfiniteLoop.ql +++ b/java/ql/src/Security/CWE/CWE-835/InfiniteLoop.ql @@ -26,7 +26,7 @@ predicate loopCondition(LoopStmt loop, Expr cond, boolean polarity) { ifstmt.getEnclosingStmt*() = loop.getBody() and ifstmt.getCondition() = cond and ( - exit.(BreakStmt).(JumpStmt).getTarget() = loop or + exit.(BreakStmt).getTarget() = loop or exit.(ReturnStmt).getEnclosingStmt*() = loop.getBody() ) and ( diff --git a/java/ql/test/library-tests/stmts/JumpTargets.expected b/java/ql/test/library-tests/stmts/JumpTargets.expected index b9f21faa4771..4117bf813b13 100644 --- a/java/ql/test/library-tests/stmts/JumpTargets.expected +++ b/java/ql/test/library-tests/stmts/JumpTargets.expected @@ -1,3 +1,8 @@ +| stmts/A.java:18:5:18:12 | yield ... | stmts/A.java:16:10:16:18 | switch (...) | +| stmts/A.java:22:6:22:13 | yield ... | stmts/A.java:20:14:20:22 | switch (...) | +| stmts/A.java:25:6:25:13 | yield ... | stmts/A.java:20:14:20:22 | switch (...) | +| stmts/A.java:34:7:34:14 | yield ... | stmts/A.java:16:10:16:18 | switch (...) | +| stmts/A.java:37:5:37:13 | yield ... | stmts/A.java:16:10:16:18 | switch (...) | | stmts/B.java:8:5:8:10 | break | stmts/B.java:6:3:6:26 | for (...;...;...) | | stmts/B.java:10:5:10:13 | continue | stmts/B.java:6:3:6:26 | for (...;...;...) | | stmts/B.java:13:6:13:11 | break | stmts/B.java:11:4:11:17 | while (...) | diff --git a/java/ql/test/library-tests/stmts/SwitchCases.expected b/java/ql/test/library-tests/stmts/SwitchCases.expected index e10bb74d654c..2de66f20ae0e 100644 --- a/java/ql/test/library-tests/stmts/SwitchCases.expected +++ b/java/ql/test/library-tests/stmts/SwitchCases.expected @@ -1,6 +1,14 @@ | stmts/A.java:6:3:6:10 | case ... | | stmts/A.java:8:3:8:10 | case ... | | stmts/A.java:10:3:10:10 | default | +| stmts/A.java:17:4:17:12 | case ... | +| stmts/A.java:20:4:20:12 | case ... | +| stmts/A.java:21:5:21:13 | case ... | +| stmts/A.java:24:5:24:14 | default | +| stmts/A.java:28:4:28:12 | case ... | +| stmts/A.java:29:4:29:13 | default | +| stmts/A.java:32:6:32:14 | case ... | +| stmts/A.java:33:6:33:14 | case ... | | stmts/B.java:21:5:21:12 | case ... | | stmts/B.java:23:5:23:12 | case ... | | stmts/B.java:25:5:25:12 | case ... | diff --git a/java/ql/test/library-tests/stmts/options b/java/ql/test/library-tests/stmts/options new file mode 100644 index 000000000000..03edcc8fcc07 --- /dev/null +++ b/java/ql/test/library-tests/stmts/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -source 14 -target 14 \ No newline at end of file diff --git a/java/ql/test/library-tests/stmts/stmts/A.java b/java/ql/test/library-tests/stmts/stmts/A.java index 1f157e6d1acf..1f5103cde1b3 100644 --- a/java/ql/test/library-tests/stmts/stmts/A.java +++ b/java/ql/test/library-tests/stmts/stmts/A.java @@ -11,4 +11,31 @@ public int foo(int x) { return x; } } + + public int nestedSwitchExpr(int x, int y) { + return switch(x) { + case 1 -> { + yield 1; + } + case 2 -> switch(y) { + case 0 -> { + yield 0; + } + default -> { + yield 1; + } + }; + case 3 -> 3; + default -> { + // SwitchStmt inside SwitchExpr + switch (y) { + case 1 -> System.out.println("1"); + case 2 -> { + yield 2; + } + } + yield -1; + } + }; + } }