diff --git a/its/ruling/src/test/resources/guava/java-S9354.json b/its/ruling/src/test/resources/guava/java-S9354.json index 5bd8aea52dc..aaba47fcf61 100644 --- a/its/ruling/src/test/resources/guava/java-S9354.json +++ b/its/ruling/src/test/resources/guava/java-S9354.json @@ -2,39 +2,7 @@ "com.google.guava:guava:src/com/google/common/collect/ExplicitOrdering.java": [ 41 ], -"com.google.guava:guava:src/com/google/common/primitives/Booleans.java": [ -297 -], -"com.google.guava:guava:src/com/google/common/primitives/Chars.java": [ -414 -], -"com.google.guava:guava:src/com/google/common/primitives/Doubles.java": [ -401 -], -"com.google.guava:guava:src/com/google/common/primitives/Floats.java": [ -397 -], -"com.google.guava:guava:src/com/google/common/primitives/Ints.java": [ -462 -], -"com.google.guava:guava:src/com/google/common/primitives/Longs.java": [ -498 -], -"com.google.guava:guava:src/com/google/common/primitives/Shorts.java": [ -461 -], -"com.google.guava:guava:src/com/google/common/primitives/SignedBytes.java": [ -202 -], "com.google.guava:guava:src/com/google/common/primitives/UnsignedBytes.java": [ -409, -420, -436 -], -"com.google.guava:guava:src/com/google/common/primitives/UnsignedInts.java": [ -176 -], -"com.google.guava:guava:src/com/google/common/primitives/UnsignedLongs.java": [ -177 +409 ] } diff --git a/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java index dc7a7f46c3d..071ac30cddc 100644 --- a/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java @@ -3,6 +3,7 @@ import java.io.File; import java.util.Comparator; import java.util.List; +import java.util.Map; import java.util.function.IntSupplier; class IntegerSubtractionInComparisonCheckSample { @@ -225,6 +226,181 @@ public int compareTo(BoxedShortCompareTo other) { } } + static class ArrayLengthComparator implements Comparator { + @Override + public int compare(int[] left, int[] right) { + return left.length - right.length; // Compliant - both operands are array lengths, bounded in [0, Integer.MAX_VALUE] + } + } + + static class StringLengthComparator implements Comparator { + @Override + public int compare(String left, String right) { + return left.length() - right.length(); // Compliant - both operands are String.length(), bounded in [0, Integer.MAX_VALUE] + } + } + + static class CollectionSizeComparator implements Comparator> { + @Override + public int compare(List left, List right) { + return left.size() - right.size(); // Compliant - both operands are Collection.size(), bounded in [0, Integer.MAX_VALUE] + } + } + + static class MapSizeComparator implements Comparator> { + @Override + public int compare(Map left, Map right) { + return left.size() - right.size(); // Compliant - both operands are Map.size(), bounded in [0, Integer.MAX_VALUE] + } + } + + enum Suit { + CLUBS, DIAMONDS, HEARTS, SPADES + } + + static class OrdinalComparator implements Comparator { + @Override + public int compare(Suit left, Suit right) { + return left.ordinal() - right.ordinal(); // Compliant - both operands are Enum.ordinal(), bounded in [0, Integer.MAX_VALUE] + } + } + + static class BitCountComparator implements Comparator { + @Override + public int compare(Integer left, Integer right) { + return Integer.bitCount(left) - Integer.bitCount(right); // Compliant - both operands are bounded in [0, 32] + } + } + + static class LongBitCountComparator implements Comparator { + @Override + public int compare(Long left, Long right) { + return Long.bitCount(left) - Long.bitCount(right); // Compliant - both operands are bounded in [0, 64] + } + } + + static class MaskedByteComparator implements Comparator { + @Override + public int compare(byte[] left, byte[] right) { + int a = left[0] & 0xff; + int b = right[0] & 0xff; + return a - b; // Compliant - both operands are masked to [0, 255] + } + } + + static class DirectMaskedByteComparator implements Comparator { + @Override + public int compare(byte[] left, byte[] right) { + return (left[0] & 0xff) - (right[0] & 0xff); // Compliant - both operands are masked to [0, 255] + } + } + + static class IndirectLengthComparator implements Comparator { + @Override + public int compare(String left, String right) { + int len1 = left.length(); + int len2 = right.length(); + return len1 - len2; // Compliant - len1 and len2 are single-write locals holding String.length() + } + } + + static class LiteralConstantComparator implements Comparator { + @Override + public int compare(Object left, Object right) { + return 0 - 1; // Compliant - both operands are compile-time constants + } + } + + static class MixedBoundedAndUnboundedComparator implements Comparable { + private int value; + + @Override + public int compareTo(MixedBoundedAndUnboundedComparator other) { + // Noncompliant@+1 {{Subtracting numeric values in compareTo can overflow; use Integer.compare instead.}} + return this.value - other.getClass().getName().length(); + } + } + + static class MixedUnboundedAndBoundedComparator implements Comparator { + @Override + public int compare(String left, String right) { + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return left.length() - right.hashCode(); + } + } + + static class BoundedButUnsafeComparator implements Comparator { + @Override + public int compare(byte[] left, byte[] right) { + int a = left[0] & 0xff; + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return a - Integer.MIN_VALUE; + } + } + + static class SignBitLiteralComparator implements Comparator { + @Override + public int compare(String left, String right) { + // 0x80000000 is the int literal for Integer.MIN_VALUE; left.length() is bounded but this still overflows. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return left.length() - 0x80000000; + } + } + + static class MaskedByUnresolvedMaskComparator implements Comparator { + @Override + public int compare(Integer left, Integer right) { + int mask = computeMask(left); + // mask does not resolve to a constant, so the mask range - and therefore this subtraction - is unknown. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return (left & mask) - 0; + } + + private static int computeMask(int seed) { + return seed > 0 ? 0xff : 0x0f; + } + } + + static class DeeplyChainedLocalsComparator implements Comparator { + @Override + public int compare(String left, String right) { + int a = left.length(); + int b = a; + int c = b; + int d = c; + int e = d; + // The single-write resolution depth cap keeps this conservative: e is not traced all the way back to + // left.length(), so it is treated as unbounded even though it provably isn't. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return e - right.length(); + } + } + + static class IncrementedCounterComparator implements Comparator { + @Override + public int compare(String[] left, String[] right) { + int leftCount = 0; + for (String s : left) { + leftCount++; + } + int rightCount = 0; + for (String s : right) { + rightCount++; + } + // leftCount and rightCount are mutated with ++, not a single assignment, so they are not resolved to a + // bounded range even though both loops only ever increment their counter. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return leftCount - rightCount; + } + } + + static class UnknownRangeComparator implements Comparable { + @Override + public int compareTo(UnknownRangeComparator other) { + return this.hashCode() - other.hashCode(); // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Integer.compare instead.}} + } + } + int subtract(int a, int b) { return a - b; // Compliant - not in a comparison method } diff --git a/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java b/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java index 951742668a8..86eefdb8692 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java @@ -18,6 +18,7 @@ import java.util.List; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.BoundedIntegerRange; import org.sonar.java.checks.helpers.ComparisonMethodUtils; import org.sonar.java.model.ExpressionUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; @@ -103,6 +104,11 @@ private static String replacementFor(BinaryExpressionTree tree) { return "Long.compare"; } if (isInt(left) || isInt(right)) { + if (BoundedIntegerRange.subtractionCannotOverflow(tree.leftOperand(), tree.rightOperand())) { + // Both operands have a provable range whose difference is guaranteed to fit in an int, + // e.g. lengths, sizes, ordinals, or masked values: no overflow is possible. + return null; + } return "Integer.compare"; } return null; diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java new file mode 100644 index 00000000000..8686624f1a0 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java @@ -0,0 +1,183 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks.helpers; + +import javax.annotation.CheckForNull; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; + +/** + * Computes a best-effort [lo, hi] bound for an integer-typed expression, used to prove that + * subtracting two such expressions cannot produce a 32-bit overflow. + *

+ * This is intentionally conservative: it only recognizes a small set of JDK APIs and operations + * with a documented or structurally guaranteed range. Any expression it cannot classify is + * treated as unbounded, which keeps every unrecognized case reported as before. + */ +public final class BoundedIntegerRange { + + private static final int MAX_SINGLE_WRITE_DEPTH = 3; + + private static final Range NON_NEGATIVE_INT = new Range(0, Integer.MAX_VALUE); + + private static final MethodMatchers CHAR_SEQUENCE_LENGTH = MethodMatchers.create() + .ofSubTypes("java.lang.CharSequence").names("length").addWithoutParametersMatcher().build(); + + private static final MethodMatchers COLLECTION_SIZE = MethodMatchers.create() + .ofSubTypes("java.util.Collection").names("size").addWithoutParametersMatcher().build(); + + private static final MethodMatchers MAP_SIZE = MethodMatchers.create() + .ofSubTypes("java.util.Map").names("size").addWithoutParametersMatcher().build(); + + private static final MethodMatchers ENUM_ORDINAL = MethodMatchers.create() + .ofSubTypes("java.lang.Enum").names("ordinal").addWithoutParametersMatcher().build(); + + private static final MethodMatchers BIT_COUNT_32 = MethodMatchers.create() + .ofTypes("java.lang.Integer").names("bitCount", "numberOfLeadingZeros", "numberOfTrailingZeros").addParametersMatcher("int").build(); + + private static final MethodMatchers BIT_COUNT_64 = MethodMatchers.create() + .ofTypes("java.lang.Long").names("bitCount", "numberOfLeadingZeros", "numberOfTrailingZeros").addParametersMatcher("long").build(); + + private BoundedIntegerRange() { + } + + /** + * @return true when both operands have a provable range whose difference is guaranteed to fit in an int, + * i.e. cannot overflow regardless of the actual runtime values. + */ + public static boolean subtractionCannotOverflow(ExpressionTree left, ExpressionTree right) { + Range leftRange = rangeOf(left, 0); + if (leftRange == null) { + return false; + } + Range rightRange = rangeOf(right, 0); + return rightRange != null && leftRange.fitsIntSubtraction(rightRange); + } + + @CheckForNull + private static Range rangeOf(ExpressionTree expression, int depth) { + if (depth > MAX_SINGLE_WRITE_DEPTH) { + return null; + } + ExpressionTree tree = ExpressionUtils.skipParentheses(expression); + Range constant = constantRange(tree); + if (constant != null) { + return constant; + } + if (tree.is(Tree.Kind.MEMBER_SELECT)) { + return arrayLengthRange((MemberSelectExpressionTree) tree); + } + if (tree.is(Tree.Kind.METHOD_INVOCATION)) { + return methodInvocationRange((MethodInvocationTree) tree); + } + if (tree.is(Tree.Kind.AND)) { + return bitwiseAndRange((BinaryExpressionTree) tree, depth); + } + if (tree.is(Tree.Kind.IDENTIFIER)) { + return identifierRange((IdentifierTree) tree, depth); + } + return null; + } + + @CheckForNull + private static Range constantRange(ExpressionTree tree) { + // Resolve through the compiler's own constant folding rather than re-parsing literal text, so the + // value respects the expression's static type: an int-typed 0x80000000 is -2147483648, not +2147483648. + Integer intValue = tree.asConstant(Integer.class).orElse(null); + if (intValue != null) { + return new Range(intValue, intValue); + } + Long longValue = tree.asConstant(Long.class).orElse(null); + if (longValue != null) { + return new Range(longValue, longValue); + } + return null; + } + + @CheckForNull + private static Range arrayLengthRange(MemberSelectExpressionTree memberSelect) { + Type ownerType = memberSelect.expression().symbolType(); + String memberName = memberSelect.identifier().name(); + if (ownerType.isArray() && "length".equals(memberName)) { + return NON_NEGATIVE_INT; + } + return null; + } + + @CheckForNull + private static Range methodInvocationRange(MethodInvocationTree invocation) { + if (CHAR_SEQUENCE_LENGTH.matches(invocation) || COLLECTION_SIZE.matches(invocation) + || MAP_SIZE.matches(invocation) || ENUM_ORDINAL.matches(invocation)) { + return NON_NEGATIVE_INT; + } + if (BIT_COUNT_32.matches(invocation)) { + return new Range(0, 32); + } + if (BIT_COUNT_64.matches(invocation)) { + return new Range(0, 64); + } + return null; + } + + @CheckForNull + private static Range bitwiseAndRange(BinaryExpressionTree and, int depth) { + Long nonNegativeConstant = nonNegativeConstant(and.leftOperand(), depth); + if (nonNegativeConstant == null) { + nonNegativeConstant = nonNegativeConstant(and.rightOperand(), depth); + } + if (nonNegativeConstant == null) { + return null; + } + return new Range(0, nonNegativeConstant); + } + + @CheckForNull + private static Long nonNegativeConstant(ExpressionTree expression, int depth) { + Range range = rangeOf(expression, depth + 1); + if (range != null && range.lo() == range.hi() && range.lo() >= 0) { + return range.lo(); + } + return null; + } + + @CheckForNull + private static Range identifierRange(IdentifierTree identifier, int depth) { + Symbol symbol = identifier.symbol(); + if (symbol.isUnknown() || !ExpressionsHelper.isNotReassigned(symbol)) { + // getSingleWriteUsage only looks at AssignmentExpressionTree reassignments, so a variable + // mutated by ++/-- would otherwise look like a single-write local pinned to its initializer. + return null; + } + ExpressionTree singleWriteUsage = ExpressionsHelper.getSingleWriteUsage(symbol); + return singleWriteUsage == null ? null : rangeOf(singleWriteUsage, depth + 1); + } + + private record Range(long lo, long hi) { + boolean fitsIntSubtraction(Range other) { + return hi - other.lo() <= Integer.MAX_VALUE && lo - other.hi() >= Integer.MIN_VALUE; + } + } + +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java b/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java new file mode 100644 index 00000000000..5da4ad2b733 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java @@ -0,0 +1,53 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks.helpers; + +import java.lang.reflect.Constructor; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.StatementTree; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Most of {@link BoundedIntegerRange}'s behaviour is exercised end-to-end through + * {@code IntegerSubtractionInComparisonCheckSample}, matching how the rest of this codebase tests checks. This class + * only covers the one scenario that cannot be expressed as a real {@code compare()}/{@code compareTo()} method body: + * {@link BoundedIntegerRange#subtractionCannotOverflow} is only ever called by the check on {@code int}-typed + * operands, so its {@code long} constant handling has no reachable call site and needs a direct test. + */ +class BoundedIntegerRangeTest { + + @Test + void private_constructor() throws Exception { + Constructor constructor = BoundedIntegerRange.class.getDeclaredConstructor(); + assertThat(constructor.isAccessible()).isFalse(); + constructor.setAccessible(true); + constructor.newInstance(); + } + + @Test + void long_constant_operands_cannot_overflow() { + List statements = JParserTestUtils.methodBody(JParserTestUtils.newCode("void m() { long x = 5L - 3L; }")); + ExpressionTree initializer = JParserTestUtils.initializerFromVariableDeclarationStatement(statements.get(0)); + BinaryExpressionTree binary = (BinaryExpressionTree) initializer; + assertThat(BoundedIntegerRange.subtractionCannotOverflow(binary.leftOperand(), binary.rightOperand())).isTrue(); + } + +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.html index 9bc09eb5407..fe4a0cf95ef 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.html @@ -20,6 +20,9 @@

Why is this an issue?

Use Integer.compare() for int values and Long.compare() for long values. Those methods compare without computing a difference that can overflow.

This rule does not flag floating-point subtraction in ordering methods. See {rule:java:S9148}.

+

This rule also does not flag subtraction of two values with a provably bounded range, such as array lengths, String.length(), +Collection.size(), Enum.ordinal(), or a value masked with a non-negative bitwise AND constant. Their difference cannot +overflow, so the subtraction is safe.

How to fix it

Replace the subtraction with Integer.compare() or Long.compare(), matching the operand type.

Code examples