You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ah...@apache.org on 2022/03/16 00:08:00 UTC
[commons-rng] branch master updated: RNG-170: Ensure nextBytes is consistent with JDK range checks
This is an automated email from the ASF dual-hosted git repository.
aherbert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-rng.git
The following commit(s) were added to refs/heads/master by this push:
new c3c8021 RNG-170: Ensure nextBytes is consistent with JDK range checks
c3c8021 is described below
commit c3c802117da90523ecc0dd1507d895ac9c3f5f54
Author: Alex Herbert <ah...@apache.org>
AuthorDate: Wed Mar 16 00:06:47 2022 +0000
RNG-170: Ensure nextBytes is consistent with JDK range checks
Updated to match behaviour of System.arraycopy and JDK 9
Objects.checkFromIndexSize. This now allows:
nextBytes(new byte[0], 0, 0)
nextBytes(new byte[10], 10, 0)
Previously these would throw an exception.
---
.../commons/rng/core/source32/IntProvider.java | 41 ++++++++++++++++++--
.../commons/rng/core/source64/LongProvider.java | 41 ++++++++++++++++++--
.../apache/commons/rng/core/BaseProviderTest.java | 45 ++++++++++++++++++++++
.../rng/core/ProvidersCommonParametricTest.java | 19 ++++++++-
.../commons/rng/core/source32/IntProviderTest.java | 35 +++++++++++++++++
.../rng/core/source64/LongProviderTest.java | 35 +++++++++++++++++
src/changes/changes.xml | 5 +++
.../resources/spotbugs/spotbugs-exclude-filter.xml | 10 +++++
8 files changed, 224 insertions(+), 7 deletions(-)
diff --git a/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/IntProvider.java b/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/IntProvider.java
index 9a4ab7a..57d91e0 100644
--- a/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/IntProvider.java
+++ b/commons-rng-core/src/main/java/org/apache/commons/rng/core/source32/IntProvider.java
@@ -152,9 +152,7 @@ public abstract class IntProvider
public void nextBytes(byte[] bytes,
int start,
int len) {
- checkIndex(0, bytes.length - 1, start);
- checkIndex(0, bytes.length - start, len);
-
+ checkFromIndexSize(start, len, bytes.length);
nextBytesFill(this, bytes, start, len);
}
@@ -206,4 +204,41 @@ public abstract class IntProvider
}
}
}
+
+ /**
+ * Checks if the sub-range from fromIndex (inclusive) to fromIndex + size (exclusive) is
+ * within the bounds of range from 0 (inclusive) to length (exclusive).
+ *
+ * <p>This function provides the functionality of
+ * {@code java.utils.Objects.checkFromIndexSize} introduced in JDK 9. The
+ * <a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Objects.html#checkFromIndexSize(int,int,int)">Objects</a>
+ * javadoc has been reproduced for reference.
+ *
+ * <p>The sub-range is defined to be out of bounds if any of the following inequalities
+ * is true:
+ * <ul>
+ * <li>{@code fromIndex < 0}
+ * <li>{@code size < 0}
+ * <li>{@code fromIndex + size > length}, taking into account integer overflow
+ * <li>{@code length < 0}, which is implied from the former inequalities
+ * </ul>
+ *
+ * @param fromIndex the lower-bound (inclusive) of the sub-interval
+ * @param size the size of the sub-range
+ * @param length the upper-bound (exclusive) of the range
+ * @return the fromIndex
+ * @throws IndexOutOfBoundsException if the sub-range is out of bounds
+ */
+ private static int checkFromIndexSize(int fromIndex, int size, int length) {
+ // check for any negatives,
+ // or overflow safe length check given the values are all positive
+ // remaining = length - fromIndex
+ if ((fromIndex | size | length) < 0 || size > length - fromIndex) {
+ throw new IndexOutOfBoundsException(
+ // Note: %<d is 'relative indexing' to re-use the last argument
+ String.format("Range [%d, %<d + %d) out of bounds for length %d",
+ fromIndex, size, length));
+ }
+ return fromIndex;
+ }
}
diff --git a/commons-rng-core/src/main/java/org/apache/commons/rng/core/source64/LongProvider.java b/commons-rng-core/src/main/java/org/apache/commons/rng/core/source64/LongProvider.java
index 278c5bb..7660d32 100644
--- a/commons-rng-core/src/main/java/org/apache/commons/rng/core/source64/LongProvider.java
+++ b/commons-rng-core/src/main/java/org/apache/commons/rng/core/source64/LongProvider.java
@@ -184,9 +184,7 @@ public abstract class LongProvider
public void nextBytes(byte[] bytes,
int start,
int len) {
- checkIndex(0, bytes.length - 1, start);
- checkIndex(0, bytes.length - start, len);
-
+ checkFromIndexSize(start, len, bytes.length);
nextBytesFill(this, bytes, start, len);
}
@@ -242,4 +240,41 @@ public abstract class LongProvider
}
}
}
+
+ /**
+ * Checks if the sub-range from fromIndex (inclusive) to fromIndex + size (exclusive) is
+ * within the bounds of range from 0 (inclusive) to length (exclusive).
+ *
+ * <p>This function provides the functionality of
+ * {@code java.utils.Objects.checkFromIndexSize} introduced in JDK 9. The
+ * <a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Objects.html#checkFromIndexSize(int,int,int)">Objects</a>
+ * javadoc has been reproduced for reference.
+ *
+ * <p>The sub-range is defined to be out of bounds if any of the following inequalities
+ * is true:
+ * <ul>
+ * <li>{@code fromIndex < 0}
+ * <li>{@code size < 0}
+ * <li>{@code fromIndex + size > length}, taking into account integer overflow
+ * <li>{@code length < 0}, which is implied from the former inequalities
+ * </ul>
+ *
+ * @param fromIndex the lower-bound (inclusive) of the sub-interval
+ * @param size the size of the sub-range
+ * @param length the upper-bound (exclusive) of the range
+ * @return the fromIndex
+ * @throws IndexOutOfBoundsException if the sub-range is out of bounds
+ */
+ private static int checkFromIndexSize(int fromIndex, int size, int length) {
+ // check for any negatives,
+ // or overflow safe length check given the values are all positive
+ // remaining = length - fromIndex
+ if ((fromIndex | size | length) < 0 || size > length - fromIndex) {
+ throw new IndexOutOfBoundsException(
+ // Note: %<d is 'relative indexing' to re-use the last argument
+ String.format("Range [%d, %<d + %d) out of bounds for length %d",
+ fromIndex, size, length));
+ }
+ return fromIndex;
+ }
}
diff --git a/commons-rng-core/src/test/java/org/apache/commons/rng/core/BaseProviderTest.java b/commons-rng-core/src/test/java/org/apache/commons/rng/core/BaseProviderTest.java
index 207ed7f..1eeaac6 100644
--- a/commons-rng-core/src/test/java/org/apache/commons/rng/core/BaseProviderTest.java
+++ b/commons-rng-core/src/test/java/org/apache/commons/rng/core/BaseProviderTest.java
@@ -83,6 +83,51 @@ class BaseProviderTest {
}
/**
+ * Test the checkIndex method. This is not used by any RNG implementations
+ * as it has been superseded by the equivalent of JDK 9 Objects.checkFromIndexSize.
+ */
+ @Test
+ void testCheckIndex() {
+ final BaseProvider rng = new BaseProvider() {
+ @Override
+ public void nextBytes(byte[] bytes) { /* noop */ }
+ @Override
+ public void nextBytes(byte[] bytes, int start, int len) { /* noop */ }
+ @Override
+ public int nextInt() {
+ return 0;
+ }
+ @Override
+ public long nextLong() {
+ return 0;
+ }
+ @Override
+ public boolean nextBoolean() {
+ return false;
+ }
+ @Override
+ public float nextFloat() {
+ return 0;
+ }
+ @Override
+ public double nextDouble() {
+ return 0;
+ }
+ };
+ // Arguments are (min, max, index)
+ // index must be in [min, max]
+ rng.checkIndex(-10, 5, 0);
+ rng.checkIndex(-10, 5, -10);
+ rng.checkIndex(-10, 5, 5);
+ rng.checkIndex(Integer.MIN_VALUE, Integer.MAX_VALUE, Integer.MIN_VALUE);
+ rng.checkIndex(Integer.MIN_VALUE, Integer.MAX_VALUE, Integer.MAX_VALUE);
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> rng.checkIndex(-10, 5, -11));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> rng.checkIndex(-10, 5, 6));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> rng.checkIndex(-10, 5, Integer.MIN_VALUE));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> rng.checkIndex(-10, 5, Integer.MAX_VALUE));
+ }
+
+ /**
* Dummy class for checking the behaviorof the IntProvider. Tests:
* <ul>
* <li>an incomplete implementation</li>
diff --git a/commons-rng-core/src/test/java/org/apache/commons/rng/core/ProvidersCommonParametricTest.java b/commons-rng-core/src/test/java/org/apache/commons/rng/core/ProvidersCommonParametricTest.java
index f390d10..6935b90 100644
--- a/commons-rng-core/src/test/java/org/apache/commons/rng/core/ProvidersCommonParametricTest.java
+++ b/commons-rng-core/src/test/java/org/apache/commons/rng/core/ProvidersCommonParametricTest.java
@@ -56,14 +56,31 @@ class ProvidersCommonParametricTest {
@ParameterizedTest
@MethodSource("getList")
void testPreconditionNextBytes(UniformRandomProvider generator) {
+ Assertions.assertThrows(NullPointerException.class, () -> generator.nextBytes(null, 0, 0));
final int size = 10;
final int num = 1;
final byte[] buf = new byte[size];
Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(buf, -1, num));
- Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(buf, size, 0));
+ // Edge-case allowed by JDK range checks (see RNG-170)
+ generator.nextBytes(buf, size, 0);
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(buf, size, 1));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(buf, size, -1));
final int offset = 2;
Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(buf, offset, size - offset + 1));
Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(buf, offset, -1));
+ // offset + length overflows
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(buf, offset, Integer.MAX_VALUE));
+ // Should be OK
+ generator.nextBytes(buf, 0, size);
+ generator.nextBytes(buf, 0, size - offset);
+ generator.nextBytes(buf, offset, size - offset);
+ // Should be consistent with no length
+ final byte[] empty = {};
+ generator.nextBytes(empty);
+ generator.nextBytes(empty, 0, 0);
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(empty, 0, 1));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(empty, 0, -1));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> generator.nextBytes(empty, -1, 0));
}
// Uniformity tests
diff --git a/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/IntProviderTest.java b/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/IntProviderTest.java
index 4947a63..ef4a9fe 100644
--- a/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/IntProviderTest.java
+++ b/commons-rng-core/src/test/java/org/apache/commons/rng/core/source32/IntProviderTest.java
@@ -18,6 +18,8 @@ package org.apache.commons.rng.core.source32;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
/**
* The tests the caching of calls to {@link IntProvider#nextInt()} are used as
@@ -75,4 +77,37 @@ class IntProviderTest {
}
}
}
+
+ @ParameterizedTest
+ @CsvSource({
+ // OK
+ "10, 0, 10",
+ "10, 5, 5",
+ "10, 9, 1",
+ // Allowed edge cases
+ "0, 0, 0",
+ "10, 10, 0",
+ // Bad
+ "10, 0, 11",
+ "10, 10, 1",
+ "10, 10, 2147483647",
+ "10, 0, -1",
+ "10, 5, -1",
+ })
+ void testNextBytesIndices(int size, int start, int len) {
+ final FlipIntProvider rng = new FlipIntProvider(999);
+ final byte[] bytes = new byte[size];
+ // Be consistent with System.arraycopy
+ try {
+ System.arraycopy(bytes, start, bytes, start, len);
+ } catch (IndexOutOfBoundsException ex) {
+ // nextBytes should throw under the same conditions.
+ // Note: This is not ArrayIndexOutOfBoundException to be
+ // future compatible with Objects.checkFromIndexSize.
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () ->
+ rng.nextBytes(bytes, start, len));
+ return;
+ }
+ rng.nextBytes(bytes, start, len);
+ }
}
diff --git a/commons-rng-core/src/test/java/org/apache/commons/rng/core/source64/LongProviderTest.java b/commons-rng-core/src/test/java/org/apache/commons/rng/core/source64/LongProviderTest.java
index b66ce87..f799999 100644
--- a/commons-rng-core/src/test/java/org/apache/commons/rng/core/source64/LongProviderTest.java
+++ b/commons-rng-core/src/test/java/org/apache/commons/rng/core/source64/LongProviderTest.java
@@ -18,6 +18,8 @@ package org.apache.commons.rng.core.source64;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
/**
* The tests the caching of calls to {@link LongProvider#nextLong()} are used as
@@ -116,4 +118,37 @@ class LongProviderTest {
}
}
}
+
+ @ParameterizedTest
+ @CsvSource({
+ // OK
+ "10, 0, 10",
+ "10, 5, 5",
+ "10, 9, 1",
+ // Allowed edge cases
+ "0, 0, 0",
+ "10, 10, 0",
+ // Bad
+ "10, 0, 11",
+ "10, 10, 1",
+ "10, 10, 2147483647",
+ "10, 0, -1",
+ "10, 5, -1",
+ })
+ void testNextBytesIndices(int size, int start, int len) {
+ final FixedLongProvider rng = new FixedLongProvider(999);
+ final byte[] bytes = new byte[size];
+ // Be consistent with System.arraycopy
+ try {
+ System.arraycopy(bytes, start, bytes, start, len);
+ } catch (IndexOutOfBoundsException ex) {
+ // nextBytes should throw under the same conditions.
+ // Note: This is not ArrayIndexOutOfBoundException to be
+ // future compatible with Objects.checkFromIndexSize.
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () ->
+ rng.nextBytes(bytes, start, len));
+ return;
+ }
+ rng.nextBytes(bytes, start, len);
+ }
}
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index c37cf53..ee36c92 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -81,6 +81,11 @@ re-run tests that fail, and pass the build if they succeed
within the allotted number of reruns (the test will be marked
as 'flaky' in the report).
">
+ <action dev="aherbert" type="fix" issue="170">
+ Update implementations of "UniformRandomProvider.nextBytes" with a range
+ [start, start + length) to be consistent with the exception conditions of the
+ JDK array range checks.
+ </action>
<action dev="aherbert" type="add" issue="167">
New "TSampler" class to sample from Student's t-distribution.
</action>
diff --git a/src/main/resources/spotbugs/spotbugs-exclude-filter.xml b/src/main/resources/spotbugs/spotbugs-exclude-filter.xml
index a4a9e08..f50d6a4 100644
--- a/src/main/resources/spotbugs/spotbugs-exclude-filter.xml
+++ b/src/main/resources/spotbugs/spotbugs-exclude-filter.xml
@@ -107,4 +107,14 @@
<BugPattern name="FE_FLOATING_POINT_EQUALITY"/>
</Match>
+ <!-- False positives for range checks. The return value matches the argument. -->
+ <Match>
+ <Or>
+ <Class name="org.apache.commons.rng.core.source32.IntProvider"/>
+ <Class name="org.apache.commons.rng.core.source64.LongProvider"/>
+ </Or>
+ <Method name="nextBytes"/>
+ <BugPattern name="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
+ </Match>
+
</FindBugsFilter>