8321786: SegmentAllocator:allocateFrom(ValueLayout, MemorySegment,ValueLayout,long,long) spec mismatch in exception scenario

minborg · minborg · commit 7edd10e5fa71 · 2024-01-08T08:20:07.000Z
Reviewed-by: mcimadamore
diff --git a/src/java.base/share/classes/java/lang/foreign/MemoryLayout.java b/src/java.base/share/classes/java/lang/foreign/MemoryLayout.java
@@ -1016,7 +1016,7 @@ static PaddingLayout paddingLayout(long byteSize) {
      * @throws IllegalArgumentException if {@code elementLayout.byteSize() % elementLayout.byteAlignment() != 0}
      */
     static SequenceLayout sequenceLayout(long elementCount, MemoryLayout elementLayout) {
-        MemoryLayoutUtil.requireNonNegative(elementCount);
+        Utils.checkNonNegativeArgument(elementCount, "elementCount");
         Objects.requireNonNull(elementLayout);
         Utils.checkElementAlignment(elementLayout,
                 "Element layout size is not multiple of alignment");
diff --git a/src/java.base/share/classes/java/lang/foreign/MemorySegment.java b/src/java.base/share/classes/java/lang/foreign/MemorySegment.java
diff --git a/src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java b/src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java
@@ -33,6 +33,7 @@
 import jdk.internal.foreign.ArenaImpl;
 import jdk.internal.foreign.SlicingAllocator;
 import jdk.internal.foreign.StringSupport;
+import jdk.internal.foreign.Utils;
 import jdk.internal.vm.annotation.ForceInline;
 
 /**
@@ -390,9 +391,10 @@ default MemorySegment allocateFrom(AddressLayout layout, MemorySegment value) {
      *         with {@code source} is not {@linkplain MemorySegment.Scope#isAlive() alive}
      * @throws WrongThreadException if this method is called from a thread {@code T},
      *         such that {@code source.isAccessibleBy(T) == false}
-     * @throws IndexOutOfBoundsException if {@code elementCount * sourceElementLayout.byteSize()} overflows
+     * @throws IllegalArgumentException if {@code elementCount * sourceElementLayout.byteSize()} overflows
+     * @throws IllegalArgumentException if {@code elementCount < 0}
      * @throws IndexOutOfBoundsException if {@code sourceOffset > source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
-     * @throws IndexOutOfBoundsException if either {@code sourceOffset} or {@code elementCount} are {@code < 0}
+     * @throws IndexOutOfBoundsException if {@code sourceOffset < 0}
      */
     @ForceInline
     default MemorySegment allocateFrom(ValueLayout elementLayout,
diff --git a/src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java b/src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
@@ -153,9 +153,7 @@ public final MemorySegment reinterpret(Arena arena, Consumer<MemorySegment> clea
 
     public MemorySegment reinterpretInternal(Class<?> callerClass, long newSize, Scope scope, Consumer<MemorySegment> cleanup) {
         Reflection.ensureNativeAccess(callerClass, MemorySegment.class, "reinterpret");
-        if (newSize < 0) {
-            throw new IllegalArgumentException("newSize < 0");
-        }
+        Utils.checkNonNegativeArgument(newSize, "newSize");
         if (!isNative()) throw new UnsupportedOperationException("Not a native segment");
         Runnable action = cleanup != null ?
                 () -> cleanup.accept(SegmentFactories.makeNativeSegmentUnchecked(address(), newSize)) :
@@ -594,6 +592,7 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcElementLayout,
                             MemorySegment dstSegment, ValueLayout dstElementLayout, long dstOffset,
                             long elementCount) {
 
+        Utils.checkNonNegativeIndex(elementCount, "elementCount");
         AbstractMemorySegmentImpl srcImpl = (AbstractMemorySegmentImpl)srcSegment;
         AbstractMemorySegmentImpl dstImpl = (AbstractMemorySegmentImpl)dstSegment;
         if (srcElementLayout.byteSize() != dstElementLayout.byteSize()) {
@@ -625,7 +624,7 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcElementLayout,
     public static void copy(MemorySegment srcSegment, ValueLayout srcLayout, long srcOffset,
                             Object dstArray, int dstIndex,
                             int elementCount) {
-
+        Utils.checkNonNegativeIndex(elementCount, "elementCount");
         var dstInfo = Utils.BaseAndScale.of(dstArray);
         if (dstArray.getClass().componentType() != srcLayout.carrier()) {
             throw new IllegalArgumentException("Incompatible value layout: " + srcLayout);
@@ -652,7 +651,6 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcLayout, long sr
     public static void copy(Object srcArray, int srcIndex,
                             MemorySegment dstSegment, ValueLayout dstLayout, long dstOffset,
                             int elementCount) {
-
         var srcInfo = Utils.BaseAndScale.of(srcArray);
         if (srcArray.getClass().componentType() != dstLayout.carrier()) {
             throw new IllegalArgumentException("Incompatible value layout: " + dstLayout);
diff --git a/src/java.base/share/classes/jdk/internal/foreign/Utils.java b/src/java.base/share/classes/jdk/internal/foreign/Utils.java
@@ -200,11 +200,8 @@ public static long pointeeByteAlign(AddressLayout addressLayout) {
     }
 
     public static void checkAllocationSizeAndAlign(long byteSize, long byteAlignment) {
-        // size should be >= 0
-        if (byteSize < 0) {
-            throw new IllegalArgumentException("Invalid allocation size : " + byteSize);
-        }
-
+        // byteSize should be >= 0
+        Utils.checkNonNegativeArgument(byteSize, "allocation size");
         checkAlign(byteAlignment);
     }
 
@@ -216,6 +213,20 @@ public static void checkAlign(long byteAlignment) {
         }
     }
 
+    @ForceInline
+    public static void checkNonNegativeArgument(long value, String name) {
+        if (value < 0) {
+            throw new IllegalArgumentException("The provided " + name + " is negative: " + value);
+        }
+    }
+
+    @ForceInline
+    public static void checkNonNegativeIndex(long value, String name) {
+        if (value < 0) {
+            throw new IndexOutOfBoundsException("The provided " + name + " is negative: " + value);
+        }
+    }
+
     private static long computePadding(long offset, long align) {
         boolean isAligned = offset == 0 || offset % align == 0;
         if (isAligned) {
diff --git a/src/java.base/share/classes/jdk/internal/foreign/layout/AbstractLayout.java b/src/java.base/share/classes/jdk/internal/foreign/layout/AbstractLayout.java
@@ -151,13 +151,8 @@ private static long requirePowerOfTwoAndGreaterOrEqualToOne(long value) {
     }
 
     public long scale(long offset, long index) {
-        if (offset < 0) {
-            throw new IllegalArgumentException("Negative offset: " + offset);
-        }
-        if (index < 0) {
-            throw new IllegalArgumentException("Negative index: " + index);
-        }
-
+        Utils.checkNonNegativeArgument(offset, "offset");
+        Utils.checkNonNegativeArgument(index, "index");
         return Math.addExact(offset, Math.multiplyExact(byteSize(), index));
     }
 
diff --git a/src/java.base/share/classes/jdk/internal/foreign/layout/MemoryLayoutUtil.java b/src/java.base/share/classes/jdk/internal/foreign/layout/MemoryLayoutUtil.java
@@ -30,13 +30,6 @@ public final class MemoryLayoutUtil {
     private MemoryLayoutUtil() {
     }
 
-    public static long requireNonNegative(long value) {
-        if (value < 0) {
-            throw new IllegalArgumentException("The provided value was negative: " + value);
-        }
-        return value;
-    }
-
     public static long requireByteSizeValid(long byteSize, boolean allowZero) {
         if ((byteSize == 0 && !allowZero) || byteSize < 0) {
             throw new IllegalArgumentException("Invalid byte size: " + byteSize);
diff --git a/test/jdk/java/foreign/TestLayouts.java b/test/jdk/java/foreign/TestLayouts.java
@@ -371,13 +371,13 @@ public void testVarHandleCaching() {
     }
 
     @Test(expectedExceptions=IllegalArgumentException.class,
-        expectedExceptionsMessageRegExp=".*Negative offset.*")
+        expectedExceptionsMessageRegExp=".*offset is negative.*")
     public void testScaleNegativeOffset() {
         JAVA_INT.scale(-1, 0);
     }
 
     @Test(expectedExceptions=IllegalArgumentException.class,
-        expectedExceptionsMessageRegExp=".*Negative index.*")
+        expectedExceptionsMessageRegExp=".*index is negative.*")
     public void testScaleNegativeIndex() {
         JAVA_INT.scale(0, -1);
     }
diff --git a/test/jdk/java/foreign/TestMemoryAccessInstance.java b/test/jdk/java/foreign/TestMemoryAccessInstance.java
@@ -164,6 +164,13 @@ public <X, L extends ValueLayout> void badAccessOverflowInIndexedAccess(String t
         }
     }
 
+    @Test(dataProvider = "segmentAccessors")
+    public <X, L extends ValueLayout> void negativeOffset(String testName, Accessor<X, L> accessor) {
+        MemorySegment segment = MemorySegment.ofArray(new byte[100]);
+        assertThrows(IndexOutOfBoundsException.class, () -> accessor.get(segment, -ValueLayout.JAVA_LONG.byteSize()));
+        assertThrows(IndexOutOfBoundsException.class, () -> accessor.set(segment, -ValueLayout.JAVA_LONG.byteSize(), accessor.value));
+    }
+
     static final ByteOrder NE = ByteOrder.nativeOrder();
 
     @DataProvider(name = "segmentAccessors")
diff --git a/test/jdk/java/foreign/TestScopedOperations.java b/test/jdk/java/foreign/TestScopedOperations.java
@@ -27,6 +27,7 @@
  */
 
 import java.lang.foreign.Arena;
+import java.lang.foreign.MemoryLayout;
 import java.lang.foreign.MemorySegment;
 import java.lang.foreign.ValueLayout;
 
@@ -135,6 +136,8 @@ public <Z> void testOpOutsideConfinement(String name, ScopedOperation<Z> scopedO
         ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_FLOAT, new float[]{0}), "Arena::allocateFrom/float");
         ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_LONG, new long[]{0}), "Arena::allocateFrom/long");
         ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_DOUBLE, new double[]{0}), "Arena::allocateFrom/double");
+        var source = MemorySegment.ofArray(new byte[]{});
+        ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_INT, source, JAVA_BYTE, 0, 1), "Arena::allocateFrom/5arg");
     };
 
     @DataProvider(name = "scopedOperations")
diff --git a/test/jdk/java/foreign/TestSegmentAllocators.java b/test/jdk/java/foreign/TestSegmentAllocators.java
@@ -44,6 +44,8 @@
 import java.nio.ShortBuffer;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.BiFunction;
 import java.util.function.Function;
@@ -189,6 +191,81 @@ public void testAllocatorAllocateFromHeapSegment() {
         }
     }
 
+    // Invariant checking tests for the SegmentAllocator method:
+    // MemorySegment allocateFrom(ValueLayout elementLayout,
+    //                            MemorySegment source,
+    //                            ValueLayout sourceElementLayout,
+    //                            long sourceOffset,
+    //                            long elementCount) {
+    @Test
+    public void testAllocatorAllocateFromArguments() {
+        try (Arena arena = Arena.ofConfined()) {
+            var sourceElements = 2;
+            var source = arena.allocate(ValueLayout.JAVA_LONG, sourceElements);
+            var elementLayout = ValueLayout.JAVA_INT;
+            var sourceElementLayout = ValueLayout.JAVA_INT;
+
+            // IllegalArgumentException if {@code elementLayout.byteSize() != sourceElementLayout.byteSize()}
+            assertThrows(IllegalArgumentException.class, () ->
+                    arena.allocateFrom(elementLayout, source, ValueLayout.JAVA_BYTE, 0, 1)
+            );
+
+            // IllegalArgumentException if source segment/offset
+            // are <a href="MemorySegment.html#segment-alignment">incompatible with the alignment constraint</a>
+            // in the source element layout
+            assertThrows(IllegalArgumentException.class, () ->
+                    arena.allocateFrom(elementLayout, source.asSlice(1), sourceElementLayout, 0, 1)
+            );
+            assertThrows(IllegalArgumentException.class, () ->
+                    arena.allocateFrom(elementLayout, source, sourceElementLayout, 1, 1)
+            );
+
+            // IllegalArgumentException if {@code elementLayout.byteAlignment() > elementLayout.byteSize()}
+            assertThrows(IllegalArgumentException.class, () ->
+                    arena.allocateFrom(elementLayout.withByteAlignment(elementLayout.byteAlignment() * 2), source, sourceElementLayout, 1, 1)
+            );
+
+            // IllegalStateException if the {@linkplain MemorySegment#scope() scope} associated
+            // with {@code source} is not {@linkplain MemorySegment.Scope#isAlive() alive}
+            // This is tested in TestScopedOperations
+
+            // WrongThreadException if this method is called from a thread {@code T},
+            // such that {@code source.isAccessibleBy(T) == false}
+            CompletableFuture<Arena> future = CompletableFuture.supplyAsync(Arena::ofConfined);
+            try {
+                Arena otherThreadArena = future.get();
+                assertThrows(WrongThreadException.class, () ->
+                        otherThreadArena.allocateFrom(elementLayout, source, sourceElementLayout, 0, 1)
+                );
+            } catch (ExecutionException | InterruptedException e) {
+                fail("Unable to create arena", e);
+            }
+
+            // IllegalArgumentException if {@code elementCount * sourceElementLayout.byteSize()} overflows
+            assertThrows(IllegalArgumentException.class, () ->
+                    arena.allocateFrom(elementLayout, source, sourceElementLayout, 0, Long.MAX_VALUE)
+            );
+
+            // IndexOutOfBoundsException if {@code sourceOffset > source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
+            assertThrows(IndexOutOfBoundsException.class, () ->
+                    arena.allocateFrom(elementLayout, source, sourceElementLayout, source.byteSize() - (1 * sourceElementLayout.byteAlignment()) + elementLayout.byteSize(), 1)
+            );
+
+            // IndexOutOfBoundsException if {@code sourceOffset < 0}
+            assertThrows(IndexOutOfBoundsException.class, () ->
+                    arena.allocateFrom(elementLayout, source, sourceElementLayout, -elementLayout.byteSize(), 1)
+            );
+
+            // IllegalArgumentException if {@code elementCount < 0}
+            assertThrows(IllegalArgumentException.class, () ->
+                    arena.allocateFrom(elementLayout, source, sourceElementLayout, 0, -1)
+            );
+
+
+        }
+    }
+
+
     @Test
     public void testArrayAllocateDelegation() {
         AtomicInteger calls = new AtomicInteger();
diff --git a/test/jdk/java/foreign/TestSegmentCopy.java b/test/jdk/java/foreign/TestSegmentCopy.java
@@ -145,6 +145,66 @@ public void testHyperAlignedDst() {
         MemorySegment.copy(segment, JAVA_BYTE.withByteAlignment(2), 0, segment, 0, 4);
     }
 
+    @Test
+    public void testCopy5ArgWithNegativeValues() {
+        MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
+        MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, -1, dst, 0, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, 0, dst, -1, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, 0, dst, 0, -1)
+        );
+    }
+
+    @Test
+    public void testCopy7ArgWithNegativeValues() {
+        MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
+        MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, JAVA_BYTE, -1, dst, JAVA_BYTE, 0, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, JAVA_BYTE, 0, dst, JAVA_BYTE, -1, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, JAVA_BYTE, 0, dst, JAVA_BYTE, 0, -1)
+        );
+    }
+
+    @Test
+    public void testCopyFromArrayWithNegativeValues() {
+        MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
+        byte[] dst = new byte[] {1, 2, 3, 4};
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, JAVA_BYTE, -1, dst, 0, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, JAVA_BYTE, 0, dst, -1, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, JAVA_BYTE, 0, dst, 0, -1)
+        );
+    }
+
+    @Test
+    public void testCopyToArrayWithNegativeValues() {
+        byte[] src = new byte[] {1, 2, 3, 4};
+        MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, -1, dst, JAVA_BYTE, 0, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, 0, dst, JAVA_BYTE, -1, 4)
+        );
+        assertThrows(IndexOutOfBoundsException.class, () ->
+                MemorySegment.copy(src, 0, dst, JAVA_BYTE, 0, -1)
+        );
+    }
+
     enum Type {
         // Byte
         BYTE(byte.class, JAVA_BYTE, i -> (byte)i),
diff --git a/test/jdk/java/foreign/TestSegments.java b/test/jdk/java/foreign/TestSegments.java
@@ -43,6 +43,7 @@
 import java.util.function.Supplier;
 
 import static java.lang.foreign.ValueLayout.JAVA_INT;
+import static java.lang.foreign.ValueLayout.JAVA_LONG;
 import static org.testng.Assert.*;
 
 public class TestSegments {
@@ -55,14 +56,13 @@ public void testBadAllocateAlign(long size, long align) {
     @Test
     public void testZeroLengthNativeSegment() {
         try (Arena arena = Arena.ofConfined()) {
-            Arena session = arena;
-            var segment = session.allocate(0, 1);
+            var segment = arena.allocate(0, 1);
             assertEquals(segment.byteSize(), 0);
             MemoryLayout seq = MemoryLayout.sequenceLayout(0, JAVA_INT);
-            segment = session.allocate(seq);
+            segment = arena.allocate(seq);
             assertEquals(segment.byteSize(), 0);
             assertEquals(segment.address() % seq.byteAlignment(), 0);
-            segment = session.allocate(0, 4);
+            segment = arena.allocate(0, 4);
             assertEquals(segment.byteSize(), 0);
             assertEquals(segment.address() % 4, 0);
             MemorySegment rawAddress = MemorySegment.ofAddress(segment.address());
@@ -133,8 +133,7 @@ public void testDerivedScopes(Supplier<MemorySegment> segmentSupplier) {
     @Test
     public void testEqualsOffHeap() {
         try (Arena arena = Arena.ofConfined()) {
-            Arena scope1 = arena;
-            MemorySegment segment = scope1.allocate(100, 1);
+            MemorySegment segment = arena.allocate(100, 1);
             assertEquals(segment, segment.asReadOnly());
             assertEquals(segment, segment.asSlice(0, 100));
             assertNotEquals(segment, segment.asSlice(10, 90));

Original file line number	Diff line number	Diff line change
`@@ -151,13 +151,8 @@ private static long requirePowerOfTwoAndGreaterOrEqualToOne(long value) {`
`151`	`151`	`}`
`152`	`152`
`153`	`153`	`public long scale(long offset, long index) {`
`154`		`- if (offset < 0) {`
`155`		`- throw new IllegalArgumentException("Negative offset: " + offset);`
`156`		`- }`
`157`		`- if (index < 0) {`
`158`		`- throw new IllegalArgumentException("Negative index: " + index);`
`159`		`- }`
`160`		`-`
	`154`	`+ Utils.checkNonNegativeArgument(offset, "offset");`
	`155`	`+ Utils.checkNonNegativeArgument(index, "index");`
`161`	`156`	`return Math.addExact(offset, Math.multiplyExact(byteSize(), index));`
`162`	`157`	`}`
`163`	`158`
Original file line number	Diff line number	Diff line change
`@@ -371,13 +371,13 @@ public void testVarHandleCaching() {`
`371`	`371`	`}`
`372`	`372`
`373`	`373`	`@Test(expectedExceptions=IllegalArgumentException.class,`
`374`		`- expectedExceptionsMessageRegExp=".Negative offset.")`
	`374`	`+ expectedExceptionsMessageRegExp=".offset is negative.")`
`375`	`375`	`public void testScaleNegativeOffset() {`
`376`	`376`	`JAVA_INT.scale(-1, 0);`
`377`	`377`	`}`
`378`	`378`
`379`	`379`	`@Test(expectedExceptions=IllegalArgumentException.class,`
`380`		`- expectedExceptionsMessageRegExp=".Negative index.")`
	`380`	`+ expectedExceptionsMessageRegExp=".index is negative.")`
`381`	`381`	`public void testScaleNegativeIndex() {`
`382`	`382`	`JAVA_INT.scale(0, -1);`
`383`	`383`	`}`
Original file line number	Diff line number	Diff line change
`@@ -164,6 +164,13 @@ public <X, L extends ValueLayout> void badAccessOverflowInIndexedAccess(String t`
`164`	`164`	`}`
`165`	`165`	`}`
`166`	`166`
	`167`	`+ @Test(dataProvider = "segmentAccessors")`
	`168`	`+ public <X, L extends ValueLayout> void negativeOffset(String testName, Accessor<X, L> accessor) {`
	`169`	`+ MemorySegment segment = MemorySegment.ofArray(new byte[100]);`
	`170`	`+ assertThrows(IndexOutOfBoundsException.class, () -> accessor.get(segment, -ValueLayout.JAVA_LONG.byteSize()));`
	`171`	`+ assertThrows(IndexOutOfBoundsException.class, () -> accessor.set(segment, -ValueLayout.JAVA_LONG.byteSize(), accessor.value));`
	`172`	`+ }`
	`173`	`+`
`167`	`174`	`static final ByteOrder NE = ByteOrder.nativeOrder();`
`168`	`175`
`169`	`176`	`@DataProvider(name = "segmentAccessors")`