vigna / fastutil

fastutil extends the Java™ Collections Framework by providing type-specific maps, sets, lists and queues.
Apache License 2.0
1.78k stars 196 forks source link

Arrays.ensureOffsetLength does not handle integer overflow #274

Closed aherbert closed 2 years ago

aherbert commented 2 years ago

This does not throw an exception as expected:

  @Test
  void testEnsureOffSetLength() {
    Assertions.assertThrows(ArrayIndexOutOfBoundsException.class,
        () -> Arrays.ensureOffsetLength(42, Integer.MAX_VALUE, 10));
  }

The issue is integer overflow of offset + length. Since the ensureOffsetLength method has already checked the offset and length are not negative then the condition if (offset + length > arrayLength) can be changed to if (length > arrayLength - offset):

  @Test
  void testEnsureOffSetLength2() {
    ArrayIndexOutOfBoundsException ex =
        Assertions.assertThrows(ArrayIndexOutOfBoundsException.class,
        () -> ensureOffsetLength(42, Integer.MAX_VALUE, 10));
    Assertions.assertTrue(ex.getMessage().contains("Last index"));
    Assertions.assertTrue(ex.getMessage().contains(Long.toString(Integer.MAX_VALUE + 10L)));
  }

  public static void ensureOffsetLength(final int arrayLength, final int offset, final int length) {
    // When Java 9 becomes the minimum, use Objects#checkFromIndexSize​, as that can be an intrinsic
    if (offset < 0) throw new ArrayIndexOutOfBoundsException("Offset (" + offset + ") is negative");
    if (length < 0) throw new IllegalArgumentException("Length (" + length + ") is negative");
    if (length > arrayLength - offset) throw new ArrayIndexOutOfBoundsException("Last index (" + ((long)offset + length) + ") is greater than array length (" + arrayLength + ")");
  }

Note: This bug may not have any effect in downstream code. This condition will occur for any reading/writing of very large arrays. In that case the method will go ahead and the ArrayIndexOutOfBoundsException will be created when the method attempts to read/write from an out of bounds index. If a bulk read/write is performed using System.arraycopy then this will generate the exception and no change to the backing arrays will occur. If the method is used and then elements accessed then the exception will be raised after some items could have been modified:

  @Test
  void testArrayIncrement() {
    int[] expected = new int[10];
    int[] actual = expected.clone();
    int offset = 5;
    int length = Integer.MAX_VALUE;
    Assertions.assertThrows(ArrayIndexOutOfBoundsException.class, () -> {
      Arrays.ensureOffsetLength(actual.length, offset, length);
      for (int i = 0; i < length; i++) {
        actual[i + offset]++;
      }
    });
    Assertions.assertArrayEquals(expected, actual);
  }

This test will pass with the modification to ensureOffsetLength.

vigna commented 2 years ago

You're totally right. We are waiting to switch to Java 9 so to use the stuff in java.util.Objects, but in the mean time I'm fixing this.