From 0ed2ab62620b5c597dbe78e6ae996e1a0960c0e7 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 23 Sep 2026 22:53:19 +0100 Subject: [PATCH 1/6] Add tests showing bug --- .../CONSISTENCY/CfgConsistency.expected | 3 ++- .../RedundantCode/UnreachableStatement/main.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/CONSISTENCY/CfgConsistency.expected b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/CONSISTENCY/CfgConsistency.expected index e38f26b8c1ce..887fef4a49c7 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/CONSISTENCY/CfgConsistency.expected +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/CONSISTENCY/CfgConsistency.expected @@ -1,5 +1,5 @@ consistencyOverview -| deadEnd | 10 | +| deadEnd | 11 | deadEnd | main.go:17:2:17:10 | select statement | | main.go:109:2:109:10 | select statement | @@ -11,3 +11,4 @@ deadEnd | main.go:151:2:151:10 | select statement | | main.go:157:2:157:10 | select statement | | main.go:164:2:164:10 | select statement | +| main.go:168:2:168:10 | select statement | diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go index 85d5fc80a938..8b89f29fde63 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go @@ -164,4 +164,19 @@ func test20() { select {} // OK: reachable after starting the goroutine } +func test21() { + select {} + panic("unreachable") // OK: allowlisted statement + // OK: empty statement + unreachable() // $ MISSING: Alert +} + +func test23() { + if true { + return + } + unreachable() // OK: deliberately unreachable + unreachable() // OK: deliberately unreachable +} + func main() {} From 31b8064f475e4c908bebdf857cc3143fb56b6a77 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 23 Sep 2026 23:02:06 +0100 Subject: [PATCH 2/6] Fix bug relating to allowlist --- .../src/RedundantCode/UnreachableStatement.ql | 41 +++++++++++++++---- .../UnreachableStatement.expected | 1 + .../UnreachableStatement/main.go | 2 +- 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/go/ql/src/RedundantCode/UnreachableStatement.ql b/go/ql/src/RedundantCode/UnreachableStatement.ql index 68df01215919..d5869350e790 100644 --- a/go/ql/src/RedundantCode/UnreachableStatement.ql +++ b/go/ql/src/RedundantCode/UnreachableStatement.ql @@ -37,7 +37,6 @@ Stmt getPreviousStmt(Stmt s) { */ predicate firstUnreachableStmt(Stmt s) { not isReachable(s) and - not s instanceof EmptyStmt and ( // a statement whose preceding statement in the same list is reachable isReachable(getPreviousStmt(s)) @@ -47,6 +46,16 @@ predicate firstUnreachableStmt(Stmt s) { ) } +/** Holds if `s` is in a run of unreachable statements following a constant condition. */ +predicate isInUnreachableRunAfterConstantCondition(Stmt s) { + not isReachable(s) and + ( + exists(getPreviousStmt(s).(IfStmt).getCondition().getBoolValue()) + or + isInUnreachableRunAfterConstantCondition(getPreviousStmt(s)) + ) +} + /** * Matches if `retval` is a constant or a struct composed wholly of constants. */ @@ -87,14 +96,32 @@ predicate allowlist(Stmt s) { exists(ReturnStmt ret | ret = s | forall(Expr retval | retval = ret.getAnExpr() | isAllowedReturnValue(retval)) ) - or - // statements deliberately made unreachable by a constant condition, such as the code - // following `if true { return }` - exists(getPreviousStmt(s).(IfStmt).getCondition().getBoolValue()) +} + +/** Holds if `s` is part of a non-reportable prefix of a run of unreachable statements. */ +predicate isInNonReportableUnreachablePrefix(Stmt s) { + (allowlist(s) or s instanceof EmptyStmt) and + ( + firstUnreachableStmt(s) + or + isInNonReportableUnreachablePrefix(getPreviousStmt(s)) + ) +} + +/** Holds if `s` is the first non-allowlisted statement in a run of unreachable statements. */ +predicate firstNonAllowlistedUnreachableStmt(Stmt s) { + not isReachable(s) and + not s instanceof EmptyStmt and + not allowlist(s) and + ( + firstUnreachableStmt(s) + or + isInNonReportableUnreachablePrefix(getPreviousStmt(s)) + ) } from Stmt s where - firstUnreachableStmt(s) and - not allowlist(s) + firstNonAllowlistedUnreachableStmt(s) and + not isInUnreachableRunAfterConstantCondition(s) select s, "This statement is unreachable." diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected index 9c70b439d5b3..240cfcaf1e5e 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected @@ -10,3 +10,4 @@ | main.go:147:2:147:17 | return statement | This statement is unreachable. | | main.go:153:2:153:22 | return statement | This statement is unreachable. | | main.go:159:2:159:43 | return statement | This statement is unreachable. | +| main.go:171:2:171:14 | expression statement | This statement is unreachable. | diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go index 8b89f29fde63..dade20c2c3c7 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go @@ -168,7 +168,7 @@ func test21() { select {} panic("unreachable") // OK: allowlisted statement // OK: empty statement - unreachable() // $ MISSING: Alert + unreachable() // $ Alert } func test23() { From a72bc0c6730e716ee662955bd274522835b56132 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 28 Sep 2026 13:47:27 +0100 Subject: [PATCH 3/6] Include empty statements in `allowlist` --- go/ql/src/RedundantCode/UnreachableStatement.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/go/ql/src/RedundantCode/UnreachableStatement.ql b/go/ql/src/RedundantCode/UnreachableStatement.ql index d5869350e790..22736e460cae 100644 --- a/go/ql/src/RedundantCode/UnreachableStatement.ql +++ b/go/ql/src/RedundantCode/UnreachableStatement.ql @@ -87,6 +87,8 @@ predicate isAllowedReturnValue(Expr retval) { * Matches if `s` is an allowed unreachable statement. */ predicate allowlist(Stmt s) { + s instanceof EmptyStmt + or // `panic("unreachable")` and similar exists(CallExpr ce | ce = s.(ExprStmt).getExpr() or ce = s.(ReturnStmt).getExpr() | ce.getTarget().mustPanic() or ce.getCalleeName().toLowerCase() = "error" @@ -100,7 +102,7 @@ predicate allowlist(Stmt s) { /** Holds if `s` is part of a non-reportable prefix of a run of unreachable statements. */ predicate isInNonReportableUnreachablePrefix(Stmt s) { - (allowlist(s) or s instanceof EmptyStmt) and + allowlist(s) and ( firstUnreachableStmt(s) or @@ -111,7 +113,6 @@ predicate isInNonReportableUnreachablePrefix(Stmt s) { /** Holds if `s` is the first non-allowlisted statement in a run of unreachable statements. */ predicate firstNonAllowlistedUnreachableStmt(Stmt s) { not isReachable(s) and - not s instanceof EmptyStmt and not allowlist(s) and ( firstUnreachableStmt(s) From 51f654d60e83c000b3aec82fb8abfcc1e2de9310 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 28 Sep 2026 13:58:06 +0100 Subject: [PATCH 4/6] Make `firstNonAllowlistedUnreachableStmt` simpler --- .../src/RedundantCode/UnreachableStatement.ql | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/go/ql/src/RedundantCode/UnreachableStatement.ql b/go/ql/src/RedundantCode/UnreachableStatement.ql index 22736e460cae..bd630cc87241 100644 --- a/go/ql/src/RedundantCode/UnreachableStatement.ql +++ b/go/ql/src/RedundantCode/UnreachableStatement.ql @@ -100,24 +100,18 @@ predicate allowlist(Stmt s) { ) } -/** Holds if `s` is part of a non-reportable prefix of a run of unreachable statements. */ -predicate isInNonReportableUnreachablePrefix(Stmt s) { +Stmt firstNonAllowlisted(Stmt s) { + not allowlist(s) and result = s + or allowlist(s) and - ( - firstUnreachableStmt(s) - or - isInNonReportableUnreachablePrefix(getPreviousStmt(s)) - ) + exists(Stmt next | getPreviousStmt(next) = s | result = firstNonAllowlisted(next)) } /** Holds if `s` is the first non-allowlisted statement in a run of unreachable statements. */ predicate firstNonAllowlistedUnreachableStmt(Stmt s) { - not isReachable(s) and - not allowlist(s) and - ( - firstUnreachableStmt(s) - or - isInNonReportableUnreachablePrefix(getPreviousStmt(s)) + exists(Stmt unreachable | + firstUnreachableStmt(unreachable) and + s = firstNonAllowlisted(unreachable) ) } From e3fb7f80b54a4fd74ace9495655f23a49475afd6 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 28 Sep 2026 14:03:28 +0100 Subject: [PATCH 5/6] Add failing test for no unreachable statement not in allowlist --- .../UnreachableStatement/UnreachableStatement.expected | 1 + .../query-tests/RedundantCode/UnreachableStatement/main.go | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected index 240cfcaf1e5e..ab5f8511b2a6 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected @@ -11,3 +11,4 @@ | main.go:153:2:153:22 | return statement | This statement is unreachable. | | main.go:159:2:159:43 | return statement | This statement is unreachable. | | main.go:171:2:171:14 | expression statement | This statement is unreachable. | +| main.go:177:1:178:12 | labeled statement | This statement is unreachable. | diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go index dade20c2c3c7..06b0985ce2d5 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go @@ -171,6 +171,13 @@ func test21() { unreachable() // $ Alert } +func test22() { + goto reachableLabel + panic("unreachable") // OK: allowlisted statement +reachableLabel: + reachable() // $ SPURIOUS: Alert // OK: reachable through the goto +} + func test23() { if true { return From e5fc41942f6969658f5dec88b81c78289798a94b Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 28 Sep 2026 14:04:18 +0100 Subject: [PATCH 6/6] Make test pass --- go/ql/src/RedundantCode/UnreachableStatement.ql | 11 +++++++---- .../UnreachableStatement.expected | 1 - .../RedundantCode/UnreachableStatement/main.go | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/go/ql/src/RedundantCode/UnreachableStatement.ql b/go/ql/src/RedundantCode/UnreachableStatement.ql index bd630cc87241..edd2b17ab674 100644 --- a/go/ql/src/RedundantCode/UnreachableStatement.ql +++ b/go/ql/src/RedundantCode/UnreachableStatement.ql @@ -101,10 +101,13 @@ predicate allowlist(Stmt s) { } Stmt firstNonAllowlisted(Stmt s) { - not allowlist(s) and result = s - or - allowlist(s) and - exists(Stmt next | getPreviousStmt(next) = s | result = firstNonAllowlisted(next)) + not isReachable(s) and + ( + not allowlist(s) and result = s + or + allowlist(s) and + exists(Stmt next | getPreviousStmt(next) = s | result = firstNonAllowlisted(next)) + ) } /** Holds if `s` is the first non-allowlisted statement in a run of unreachable statements. */ diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected index ab5f8511b2a6..240cfcaf1e5e 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected @@ -11,4 +11,3 @@ | main.go:153:2:153:22 | return statement | This statement is unreachable. | | main.go:159:2:159:43 | return statement | This statement is unreachable. | | main.go:171:2:171:14 | expression statement | This statement is unreachable. | -| main.go:177:1:178:12 | labeled statement | This statement is unreachable. | diff --git a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go index 06b0985ce2d5..0dacf0c44984 100644 --- a/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go +++ b/go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go @@ -175,7 +175,7 @@ func test22() { goto reachableLabel panic("unreachable") // OK: allowlisted statement reachableLabel: - reachable() // $ SPURIOUS: Alert // OK: reachable through the goto + reachable() // OK: reachable through the goto } func test23() {