From 38155033142e118324628565783850bc6e9bf456 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 5 Sep 2025 15:56:23 +0200 Subject: [PATCH 1/3] Java: Consolidate Assertions.qll and Preconditions.qll. --- .../lib/semmle/code/java/ControlFlowGraph.qll | 5 +- .../semmle/code/java/controlflow/Guards.qll | 10 +- .../controlflow/internal/Preconditions.qll | 193 +++++++++++------- .../semmle/code/java/dataflow/Nullness.qll | 21 +- .../code/java/frameworks/Assertions.qll | 4 +- .../Dead Code/DeadLocals.qll | 16 +- 6 files changed, 154 insertions(+), 95 deletions(-) diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index c1c19fa44508..d98395b49e59 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -365,10 +365,10 @@ private module ControlFlowGraphImpl { * Bind `t` to an unchecked exception that may occur in a precondition check or guard wrapper. */ private predicate uncheckedExceptionFromMethod(MethodCall ma, ThrowableType t) { - conditionCheckArgument(ma, _, _) and + (methodCallChecksArgument(ma) or methodCallUnconditionallyThrows(ma)) and (t instanceof TypeError or t instanceof TypeRuntimeException) or - methodMayThrow(ma.getMethod(), t) + methodMayThrow(ma.getMethod().getSourceDeclaration(), t) } /** @@ -586,6 +586,7 @@ private module ControlFlowGraphImpl { * Gets a `MethodCall` that always throws an exception or calls `exit`. */ private MethodCall nonReturningMethodCall() { + methodCallUnconditionallyThrows(result) or result.getMethod().getSourceDeclaration() = nonReturningMethod() or result = likelyNonReturningMethod().getAnAccess() } diff --git a/java/ql/lib/semmle/code/java/controlflow/Guards.qll b/java/ql/lib/semmle/code/java/controlflow/Guards.qll index 51d6a62f43bb..ac46a2a25b72 100644 --- a/java/ql/lib/semmle/code/java/controlflow/Guards.qll +++ b/java/ql/lib/semmle/code/java/controlflow/Guards.qll @@ -395,11 +395,13 @@ private module LogicInputCommon { predicate additionalImpliesStep( GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2 ) { - exists(MethodCall check, int argIndex | + exists(MethodCall check | g1 = check and - v1.getDualValue().isThrowsException() and - conditionCheckArgument(check, argIndex, v2.asBooleanValue()) and - g2 = check.getArgument(argIndex) + v1.getDualValue().isThrowsException() + | + methodCallChecksBoolean(check, g2, v2.asBooleanValue()) + or + methodCallChecksNotNull(check, g2) and v2.isNonNullValue() ) } } diff --git a/java/ql/lib/semmle/code/java/controlflow/internal/Preconditions.qll b/java/ql/lib/semmle/code/java/controlflow/internal/Preconditions.qll index a0d2e4ef03ec..cc8ee539a5b1 100644 --- a/java/ql/lib/semmle/code/java/controlflow/internal/Preconditions.qll +++ b/java/ql/lib/semmle/code/java/controlflow/internal/Preconditions.qll @@ -1,5 +1,5 @@ /** - * Provides predicates for identifying precondition checks like + * Provides predicates for identifying precondition and assertion checks like * `com.google.common.base.Preconditions` and * `org.apache.commons.lang3.Validate`. */ @@ -9,99 +9,150 @@ module; import java /** - * Holds if `m` is a non-overridable method that checks that its zero-indexed `argument` - * is equal to `checkTrue` and throws otherwise. + * Holds if `m` is a method that checks that its argument at position `arg` is + * equal to true and throws otherwise. */ -predicate conditionCheckMethodArgument(Method m, int argument, boolean checkTrue) { - condtionCheckMethodGooglePreconditions(m, checkTrue) and argument = 0 - or - conditionCheckMethodApacheCommonsLang3Validate(m, checkTrue) and argument = 0 +private predicate methodCheckTrue(Method m, int arg) { + arg = 0 and + ( + m.hasQualifiedName("com.google.common.base", "Preconditions", ["checkArgument", "checkState"]) or + m.hasQualifiedName("com.google.common.base", "Verify", "verify") or + m.hasQualifiedName("org.apache.commons.lang3", "Validate", ["isTrue", "validState"]) or + m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "assertTrue") or + m.hasQualifiedName("org.junit.jupiter.api", "Assumptions", "assumeTrue") or + m.hasQualifiedName("org.testng", "Assert", "assertTrue") + ) or - condtionCheckMethodTestingFramework(m, argument, checkTrue) + m.getParameter(arg).getType() instanceof BooleanType and + ( + m.hasQualifiedName("org.junit", "Assert", "assertTrue") or + m.hasQualifiedName("org.junit", "Assume", "assumeTrue") or + m.hasQualifiedName("junit.framework", _, "assertTrue") + ) +} + +/** + * Holds if `m` is a method that checks that its argument at position `arg` is + * equal to false and throws otherwise. + */ +private predicate methodCheckFalse(Method m, int arg) { + arg = 0 and + ( + m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "assertFalse") or + m.hasQualifiedName("org.junit.jupiter.api", "Assumptions", "assumeFalse") or + m.hasQualifiedName("org.testng", "Assert", "assertFalse") + ) or - exists(Parameter p, MethodCall ma, int argIndex, boolean ct, Expr arg | - p = m.getParameter(argument) and - not m.isOverridable() and - m.getBody().getStmt(0).(ExprStmt).getExpr() = ma and - conditionCheckArgument(ma, argIndex, ct) and - ma.getArgument(argIndex) = arg and - ( - arg.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and - checkTrue = ct.booleanNot() - or - arg.(VarAccess).getVariable() = p and checkTrue = ct - ) + m.getParameter(arg).getType() instanceof BooleanType and + ( + m.hasQualifiedName("org.junit", "Assert", "assertFalse") or + m.hasQualifiedName("org.junit", "Assume", "assumeFalse") or + m.hasQualifiedName("junit.framework", _, "assertFalse") + ) +} + +/** + * Holds if `m` is a method that checks that its argument at position `arg` is + * not null and throws otherwise. + */ +private predicate methodCheckNotNull(Method m, int arg) { + arg = 0 and + ( + m.hasQualifiedName("com.google.common.base", "Preconditions", "checkNotNull") or + m.hasQualifiedName("com.google.common.base", "Verify", "verifyNotNull") or + m.hasQualifiedName("org.apache.commons.lang3", "Validate", "notNull") or + m.hasQualifiedName("java.util", "Objects", "requireNonNull") or + m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "assertNotNull") or + m.hasQualifiedName("org.junit", "Assume", "assumeNotNull") or // vararg + m.hasQualifiedName("org.testng", "Assert", "assertNotNull") ) or - exists(Parameter p, IfStmt ifstmt, Expr cond | - p = m.getParameter(argument) and - not m.isOverridable() and - p.getType() instanceof BooleanType and - m.getBody().getStmt(0) = ifstmt and - ifstmt.getCondition() = cond and - ( - cond.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and checkTrue = true - or - cond.(VarAccess).getVariable() = p and checkTrue = false - ) and - ( - ifstmt.getThen() instanceof ThrowStmt or - ifstmt.getThen().(SingletonBlock).getStmt() instanceof ThrowStmt - ) + arg = m.getNumberOfParameters() - 1 and + ( + m.hasQualifiedName("org.junit", "Assert", "assertNotNull") or + m.hasQualifiedName("junit.framework", _, "assertNotNull") ) } -private predicate condtionCheckMethodGooglePreconditions(Method m, boolean checkTrue) { - m.getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") and - checkTrue = true and - (m.hasName("checkArgument") or m.hasName("checkState")) +/** + * Holds if `m` is a method that checks that its argument at position `arg` + * satisfies a property specified by another argument and throws otherwise. + */ +private predicate methodCheckThat(Method m, int arg) { + m.getParameter(arg).getType().getErasure() instanceof TypeObject and + ( + m.hasQualifiedName("org.hamcrest", "MatcherAssert", "assertThat") or + m.hasQualifiedName("org.junit", "Assert", "assertThat") or + m.hasQualifiedName("org.junit", "Assume", "assumeThat") + ) } -private predicate conditionCheckMethodApacheCommonsLang3Validate(Method m, boolean checkTrue) { - m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") and - checkTrue = true and - (m.hasName("isTrue") or m.hasName("validState")) +/** Holds if `m` is a method that unconditionally throws. */ +private predicate methodUnconditionallyThrows(Method m) { + m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "fail") or + m.hasQualifiedName("org.junit", "Assert", "fail") or + m.hasQualifiedName("junit.framework", _, "fail") or + m.hasQualifiedName("org.testng", "Assert", "fail") } /** - * Holds if `m` is a non-overridable testing framework method that checks that its first argument - * is equal to `checkTrue` and throws otherwise. + * Holds if `mc` is a call to a method that checks that its argument `arg` is + * equal to `checkTrue` and throws otherwise. */ -private predicate condtionCheckMethodTestingFramework(Method m, int argument, boolean checkTrue) { - argument = 0 and - ( - m.getDeclaringType().hasQualifiedName("org.junit", "Assume") and - checkTrue = true and - m.hasName("assumeTrue") +predicate methodCallChecksBoolean(MethodCall mc, Expr arg, boolean checkTrue) { + exists(int pos | mc.getArgument(pos) = arg | + methodCheckTrue(mc.getMethod().getSourceDeclaration(), pos) and checkTrue = true or - m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assertions") and - ( - checkTrue = true and m.hasName("assertTrue") - or - checkTrue = false and m.hasName("assertFalse") - ) - or - m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assumptions") and - ( - checkTrue = true and m.hasName("assumeTrue") - or - checkTrue = false and m.hasName("assumeFalse") - ) + methodCheckFalse(mc.getMethod().getSourceDeclaration(), pos) and checkTrue = false ) - or - m.getDeclaringType().hasQualifiedName(["org.junit", "org.testng"], "Assert") and - m.getParameter(argument).getType() instanceof BooleanType and - ( - checkTrue = true and m.hasName("assertTrue") +} + +/** + * Holds if `mc` is a call to a method that checks that its argument `arg` is + * not null and throws otherwise. + */ +predicate methodCallChecksNotNull(MethodCall mc, Expr arg) { + exists(int pos | mc.getArgument(pos) = arg | + methodCheckNotNull(mc.getMethod().getSourceDeclaration(), pos) or - checkTrue = false and m.hasName("assertFalse") + methodCheckThat(mc.getMethod().getSourceDeclaration(), pos) and + mc.getAnArgument().(MethodCall).getMethod().getName() = "notNullValue" ) } /** + * Holds if `mc` is a call to a method that checks one of its arguments in some + * way and possibly throws. + */ +predicate methodCallChecksArgument(MethodCall mc) { + methodCallChecksBoolean(mc, _, _) or + methodCallChecksNotNull(mc, _) +} + +/** Holds if `mc` is a call to a method that unconditionally throws. */ +predicate methodCallUnconditionallyThrows(MethodCall mc) { + methodUnconditionallyThrows(mc.getMethod().getSourceDeclaration()) or + exists(BooleanLiteral b | methodCallChecksBoolean(mc, b, b.getBooleanValue().booleanNot())) +} + +/** + * DEPRECATED: Use `methodCallChecksBoolean` instead. + * + * Holds if `m` is a non-overridable method that checks that its zero-indexed `argument` + * is equal to `checkTrue` and throws otherwise. + */ +deprecated predicate conditionCheckMethodArgument(Method m, int argument, boolean checkTrue) { + methodCheckTrue(m, argument) and checkTrue = true + or + methodCheckFalse(m, argument) and checkTrue = false +} + +/** + * DEPRECATED: Use `methodCallChecksBoolean` instead. + * * Holds if `ma` is an access to a non-overridable method that checks that its * zero-indexed `argument` is equal to `checkTrue` and throws otherwise. */ -predicate conditionCheckArgument(MethodCall ma, int argument, boolean checkTrue) { +deprecated predicate conditionCheckArgument(MethodCall ma, int argument, boolean checkTrue) { conditionCheckMethodArgument(ma.getMethod().getSourceDeclaration(), argument, checkTrue) } diff --git a/java/ql/lib/semmle/code/java/dataflow/Nullness.qll b/java/ql/lib/semmle/code/java/dataflow/Nullness.qll index 756c5d1ae9f7..64423e9a6b99 100644 --- a/java/ql/lib/semmle/code/java/dataflow/Nullness.qll +++ b/java/ql/lib/semmle/code/java/dataflow/Nullness.qll @@ -42,7 +42,7 @@ private import RangeUtils private import IntegerGuards private import NullGuards private import semmle.code.java.Collections -private import semmle.code.java.frameworks.Assertions +private import semmle.code.java.controlflow.internal.Preconditions /** Gets an expression that may be `null`. */ Expr nullExpr() { @@ -140,20 +140,11 @@ private ControlFlowNode varDereference(SsaVariable v, VarAccess va) { * A `ControlFlowNode` that ensures that the SSA variable is not null in any * subsequent use, either by dereferencing it or by an assertion. */ -private ControlFlowNode ensureNotNull(SsaVariable v) { - result = varDereference(v, _) - or - exists(AssertTrueMethod m | result.asCall() = m.getACheck(directNullGuard(v, true, false))) - or - exists(AssertFalseMethod m | result.asCall() = m.getACheck(directNullGuard(v, false, false))) - or - exists(AssertNotNullMethod m | result.asCall() = m.getACheck(v.getAUse())) - or - exists(AssertThatMethod m, MethodCall ma | - result.asCall() = m.getACheck(v.getAUse()) and ma.getControlFlowNode() = result - | - ma.getAnArgument().(MethodCall).getMethod().getName() = "notNullValue" - ) +private ControlFlowNode ensureNotNull(SsaVariable v) { result = varDereference(v, _) } + +private predicate assertFail(BasicBlock bb, ControlFlowNode n) { + bb = n.getBasicBlock() and + methodCallUnconditionallyThrows(n.asExpr()) } /** diff --git a/java/ql/lib/semmle/code/java/frameworks/Assertions.qll b/java/ql/lib/semmle/code/java/frameworks/Assertions.qll index 9849be5f3603..49afcb7a3e18 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Assertions.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Assertions.qll @@ -1,4 +1,6 @@ /** + * DEPRECATED. + * * A library providing uniform access to various assertion frameworks. * * Currently supports `org.junit.Assert`, `junit.framework.*`, @@ -6,7 +8,7 @@ * and `java.util.Objects`. */ overlay[local?] -module; +deprecated module; import java diff --git a/java/ql/src/Violations of Best Practice/Dead Code/DeadLocals.qll b/java/ql/src/Violations of Best Practice/Dead Code/DeadLocals.qll index 26c5ed66a86d..5afab52c37e2 100644 --- a/java/ql/src/Violations of Best Practice/Dead Code/DeadLocals.qll +++ b/java/ql/src/Violations of Best Practice/Dead Code/DeadLocals.qll @@ -4,7 +4,7 @@ import java import semmle.code.java.dataflow.SSA -private import semmle.code.java.frameworks.Assertions +private import semmle.code.java.controlflow.internal.Preconditions private predicate emptyDecl(LocalVariableDeclExpr decl) { not exists(decl.getInit()) and @@ -22,7 +22,19 @@ predicate deadLocal(VariableUpdate upd) { /** * A dead variable update that is expected to be dead as indicated by an assertion. */ -predicate expectedDead(VariableUpdate upd) { assertFail(upd.getBasicBlock(), _) } +predicate expectedDead(VariableUpdate upd) { + exists(BasicBlock bb, ControlFlowNode n | + bb = upd.getBasicBlock() and + bb = n.getBasicBlock() + | + methodCallUnconditionallyThrows(n.asExpr()) + or + exists(AssertStmt a | + n.asExpr() = a.getExpr() and + a.getExpr().(BooleanLiteral).getBooleanValue() = false + ) + ) +} /** * A dead update that is overwritten by a live update. From b5c7bc1b331841f1353d68e3d006795e3b197c62 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 8 Sep 2025 11:38:58 +0200 Subject: [PATCH 2/3] Java: Accept test output. --- .../guards/guardspreconditions.expected | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/java/ql/test/library-tests/guards/guardspreconditions.expected b/java/ql/test/library-tests/guards/guardspreconditions.expected index 2d4597f0282b..042fc2a05bff 100644 --- a/java/ql/test/library-tests/guards/guardspreconditions.expected +++ b/java/ql/test/library-tests/guards/guardspreconditions.expected @@ -1,35 +1,19 @@ | Preconditions.java:8:9:8:31 | assertTrue(...) | exception | Preconditions.java:7:10:7:14 | Exceptional Exit | | Preconditions.java:8:9:8:31 | assertTrue(...) | no exception | Preconditions.java:9:9:9:18 | ; | -| Preconditions.java:13:9:13:32 | assertTrue(...) | exception | Preconditions.java:12:10:12:14 | Exceptional Exit | -| Preconditions.java:13:9:13:32 | assertTrue(...) | no exception | Preconditions.java:14:9:14:18 | ; | | Preconditions.java:18:9:18:33 | assertFalse(...) | exception | Preconditions.java:17:10:17:14 | Exceptional Exit | | Preconditions.java:18:9:18:33 | assertFalse(...) | no exception | Preconditions.java:19:9:19:18 | ; | -| Preconditions.java:23:9:23:32 | assertFalse(...) | exception | Preconditions.java:22:10:22:14 | Exceptional Exit | -| Preconditions.java:23:9:23:32 | assertFalse(...) | no exception | Preconditions.java:24:9:24:18 | ; | | Preconditions.java:28:9:28:41 | assertTrue(...) | exception | Preconditions.java:27:10:27:14 | Exceptional Exit | | Preconditions.java:28:9:28:41 | assertTrue(...) | no exception | Preconditions.java:29:9:29:18 | ; | -| Preconditions.java:33:9:33:42 | assertTrue(...) | exception | Preconditions.java:32:10:32:14 | Exceptional Exit | -| Preconditions.java:33:9:33:42 | assertTrue(...) | no exception | Preconditions.java:34:9:34:18 | ; | | Preconditions.java:38:9:38:43 | assertFalse(...) | exception | Preconditions.java:37:10:37:14 | Exceptional Exit | | Preconditions.java:38:9:38:43 | assertFalse(...) | no exception | Preconditions.java:39:9:39:18 | ; | -| Preconditions.java:43:9:43:42 | assertFalse(...) | exception | Preconditions.java:42:10:42:14 | Exceptional Exit | -| Preconditions.java:43:9:43:42 | assertFalse(...) | no exception | Preconditions.java:44:9:44:18 | ; | | Preconditions.java:48:9:48:35 | assertTrue(...) | exception | Preconditions.java:47:10:47:14 | Exceptional Exit | | Preconditions.java:48:9:48:35 | assertTrue(...) | no exception | Preconditions.java:49:9:49:18 | ; | -| Preconditions.java:53:9:53:36 | assertTrue(...) | exception | Preconditions.java:52:10:52:15 | Exceptional Exit | -| Preconditions.java:53:9:53:36 | assertTrue(...) | no exception | Preconditions.java:54:9:54:18 | ; | | Preconditions.java:58:9:58:37 | assertFalse(...) | exception | Preconditions.java:57:10:57:15 | Exceptional Exit | | Preconditions.java:58:9:58:37 | assertFalse(...) | no exception | Preconditions.java:59:9:59:18 | ; | -| Preconditions.java:63:9:63:36 | assertFalse(...) | exception | Preconditions.java:62:10:62:15 | Exceptional Exit | -| Preconditions.java:63:9:63:36 | assertFalse(...) | no exception | Preconditions.java:64:9:64:18 | ; | | Preconditions.java:68:9:68:45 | assertTrue(...) | exception | Preconditions.java:67:10:67:15 | Exceptional Exit | | Preconditions.java:68:9:68:45 | assertTrue(...) | no exception | Preconditions.java:69:9:69:18 | ; | -| Preconditions.java:73:9:73:46 | assertTrue(...) | exception | Preconditions.java:72:10:72:15 | Exceptional Exit | -| Preconditions.java:73:9:73:46 | assertTrue(...) | no exception | Preconditions.java:74:9:74:18 | ; | | Preconditions.java:78:9:78:47 | assertFalse(...) | exception | Preconditions.java:77:10:77:15 | Exceptional Exit | | Preconditions.java:78:9:78:47 | assertFalse(...) | no exception | Preconditions.java:79:9:79:18 | ; | -| Preconditions.java:83:9:83:46 | assertFalse(...) | exception | Preconditions.java:82:10:82:15 | Exceptional Exit | -| Preconditions.java:83:9:83:46 | assertFalse(...) | no exception | Preconditions.java:84:9:84:18 | ; | | Preconditions.java:88:9:88:15 | t(...) | exception | Preconditions.java:87:10:87:15 | Exceptional Exit | | Preconditions.java:88:9:88:15 | t(...) | no exception | Preconditions.java:89:9:89:18 | ; | | Preconditions.java:93:9:93:16 | t(...) | exception | Preconditions.java:92:10:92:15 | Exceptional Exit | From e7df1b220c5376108999f9f13dc0e808ea381f72 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 11 Sep 2025 10:00:53 +0200 Subject: [PATCH 3/3] Java: Add change note. --- java/ql/lib/change-notes/2025-09-11-assertions-cfg.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/lib/change-notes/2025-09-11-assertions-cfg.md diff --git a/java/ql/lib/change-notes/2025-09-11-assertions-cfg.md b/java/ql/lib/change-notes/2025-09-11-assertions-cfg.md new file mode 100644 index 000000000000..34ff19d685e4 --- /dev/null +++ b/java/ql/lib/change-notes/2025-09-11-assertions-cfg.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Improved support for various assertion libraries, in particular JUnit. This affects the control-flow graph slightly, and in turn affects several queries (mainly quality queries). Most queries should see improved precision (new true positives and fewer false positives), in particular `java/constant-comparison`, `java/index-out-of-bounds`, `java/dereferenced-value-may-be-null`, and `java/useless-null-check`. Some medium precision queries like `java/toctou-race-condition` and `java/unreleased-lock` may see mixed result changes (both slight improvements and slight regressions).