diff --git a/change-notes/1.19/analysis-java.md b/change-notes/1.19/analysis-java.md index 3d5e09b5746..f2d0d5adf97 100644 --- a/change-notes/1.19/analysis-java.md +++ b/change-notes/1.19/analysis-java.md @@ -13,6 +13,7 @@ |----------------------------|------------------------|------------------------------------------------------------------| | Array index out of bounds (`java/index-out-of-bounds`) | Fewer false positive results | False positives involving arrays with a length evenly divisible by 3 or some greater number and an index being increased with a similar stride length are no longer reported. | | Unreachable catch clause (`java/unreachable-catch-clause`) | Fewer false positive results | This rule now accounts for calls to generic methods that throw generic exceptions. | +| Useless comparison test (`java/constant-comparison`) | Fewer false positive results | Constant comparisons guarding `java.util.ConcurrentModificationException` are no longer reported, as they are intended to always be false in the absence of API misuse. | ## Changes to QL libraries diff --git a/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql b/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql index 818ba676241..d6c1e205668 100644 --- a/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql +++ b/java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql @@ -167,6 +167,15 @@ predicate overFlowTest(ComparisonExpr comp) { comp.getGreaterOperand().(IntegerLiteral).getIntValue() = 0 } +predicate concurrentModificationTest(BinaryExpr test) { + exists(IfStmt ifstmt, ThrowStmt throw, RefType exc | + ifstmt.getCondition() = test and + (ifstmt.getThen() = throw or ifstmt.getThen().(SingletonBlock).getStmt() = throw) and + throw.getExpr().(ClassInstanceExpr).getConstructedType() = exc and + exc.hasQualifiedName("java.util", "ConcurrentModificationException") + ) +} + /** * Holds if `test` and `guard` are equality tests of the same integral variable v with constants `c1` and `c2`. */ @@ -202,13 +211,13 @@ where ) else if constCondSimple(test, _) - then ( - constCondSimple(test, testIsTrue) and reason = "" and reasonElem = test // dummy reason element - ) else + then constCondSimple(test, testIsTrue) and reason = "" and reasonElem = test // dummy reason element + else exists(CondReason r | constCond(test, testIsTrue, r) and reason = ", because of $@" and reasonElem = r.getCond() ) ) and not overFlowTest(test) and + not concurrentModificationTest(test) and not exists(AssertStmt assert | assert.getExpr() = test.getParent*()) select test, "Test is always " + testIsTrue + reason + ".", reasonElem, "this condition" diff --git a/java/ql/test/query-tests/UselessComparisonTest/B.java b/java/ql/test/query-tests/UselessComparisonTest/B.java new file mode 100644 index 00000000000..c9c2b1cd69c --- /dev/null +++ b/java/ql/test/query-tests/UselessComparisonTest/B.java @@ -0,0 +1,22 @@ +import java.util.*; +import java.util.function.*; + +public class B { + int modcount = 0; + + int[] arr; + + public void modify(IntUnaryOperator func) { + int mc = modcount; + for (int i = 0; i < arr.length; i++) { + arr[i] = func.applyAsInt(arr[i]); + } + // Always false unless there is a call path from func.applyAsInt(..) to + // this method, but should not be reported, as checks guarding + // ConcurrentModificationExceptions are expected to always be false in the + // absence of API misuse. + if (mc != modcount) + throw new ConcurrentModificationException(); + modcount++; + } +}