Skip to content

Commit 7edd10e

Browse files
minborgminborg
committedJan 8, 2024
8321786: SegmentAllocator:allocateFrom(ValueLayout, MemorySegment,ValueLayout,long,long) spec mismatch in exception scenario
Reviewed-by: mcimadamore
·
jdk-25+17jdk-23-ga
1 parent d75d876 commit 7edd10e

13 files changed

+218
-39
lines changed
 

‎src/java.base/share/classes/java/lang/foreign/MemoryLayout.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1016,7 +1016,7 @@ static PaddingLayout paddingLayout(long byteSize) {
10161016
* @throws IllegalArgumentException if {@code elementLayout.byteSize() % elementLayout.byteAlignment() != 0}
10171017
*/
10181018
static SequenceLayout sequenceLayout(long elementCount, MemoryLayout elementLayout) {
1019-
MemoryLayoutUtil.requireNonNegative(elementCount);
1019+
Utils.checkNonNegativeArgument(elementCount, "elementCount");
10201020
Objects.requireNonNull(elementLayout);
10211021
Utils.checkElementAlignment(elementLayout,
10221022
"Element layout size is not multiple of alignment");

‎src/java.base/share/classes/java/lang/foreign/MemorySegment.java

+38-4
Large diffs are not rendered by default.

‎src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java

+4-2
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import jdk.internal.foreign.ArenaImpl;
3434
import jdk.internal.foreign.SlicingAllocator;
3535
import jdk.internal.foreign.StringSupport;
36+
import jdk.internal.foreign.Utils;
3637
import jdk.internal.vm.annotation.ForceInline;
3738

3839
/**
@@ -390,9 +391,10 @@ default MemorySegment allocateFrom(AddressLayout layout, MemorySegment value) {
390391
* with {@code source} is not {@linkplain MemorySegment.Scope#isAlive() alive}
391392
* @throws WrongThreadException if this method is called from a thread {@code T},
392393
* such that {@code source.isAccessibleBy(T) == false}
393-
* @throws IndexOutOfBoundsException if {@code elementCount * sourceElementLayout.byteSize()} overflows
394+
* @throws IllegalArgumentException if {@code elementCount * sourceElementLayout.byteSize()} overflows
395+
* @throws IllegalArgumentException if {@code elementCount < 0}
394396
* @throws IndexOutOfBoundsException if {@code sourceOffset > source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
395-
* @throws IndexOutOfBoundsException if either {@code sourceOffset} or {@code elementCount} are {@code < 0}
397+
* @throws IndexOutOfBoundsException if {@code sourceOffset < 0}
396398
*/
397399
@ForceInline
398400
default MemorySegment allocateFrom(ValueLayout elementLayout,

‎src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java

+3-5
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,7 @@ public final MemorySegment reinterpret(Arena arena, Consumer<MemorySegment> clea
153153

154154
public MemorySegment reinterpretInternal(Class<?> callerClass, long newSize, Scope scope, Consumer<MemorySegment> cleanup) {
155155
Reflection.ensureNativeAccess(callerClass, MemorySegment.class, "reinterpret");
156-
if (newSize < 0) {
157-
throw new IllegalArgumentException("newSize < 0");
158-
}
156+
Utils.checkNonNegativeArgument(newSize, "newSize");
159157
if (!isNative()) throw new UnsupportedOperationException("Not a native segment");
160158
Runnable action = cleanup != null ?
161159
() -> cleanup.accept(SegmentFactories.makeNativeSegmentUnchecked(address(), newSize)) :
@@ -594,6 +592,7 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcElementLayout,
594592
MemorySegment dstSegment, ValueLayout dstElementLayout, long dstOffset,
595593
long elementCount) {
596594

595+
Utils.checkNonNegativeIndex(elementCount, "elementCount");
597596
AbstractMemorySegmentImpl srcImpl = (AbstractMemorySegmentImpl)srcSegment;
598597
AbstractMemorySegmentImpl dstImpl = (AbstractMemorySegmentImpl)dstSegment;
599598
if (srcElementLayout.byteSize() != dstElementLayout.byteSize()) {
@@ -625,7 +624,7 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcElementLayout,
625624
public static void copy(MemorySegment srcSegment, ValueLayout srcLayout, long srcOffset,
626625
Object dstArray, int dstIndex,
627626
int elementCount) {
628-
627+
Utils.checkNonNegativeIndex(elementCount, "elementCount");
629628
var dstInfo = Utils.BaseAndScale.of(dstArray);
630629
if (dstArray.getClass().componentType() != srcLayout.carrier()) {
631630
throw new IllegalArgumentException("Incompatible value layout: " + srcLayout);
@@ -652,7 +651,6 @@ public static void copy(MemorySegment srcSegment, ValueLayout srcLayout, long sr
652651
public static void copy(Object srcArray, int srcIndex,
653652
MemorySegment dstSegment, ValueLayout dstLayout, long dstOffset,
654653
int elementCount) {
655-
656654
var srcInfo = Utils.BaseAndScale.of(srcArray);
657655
if (srcArray.getClass().componentType() != dstLayout.carrier()) {
658656
throw new IllegalArgumentException("Incompatible value layout: " + dstLayout);

‎src/java.base/share/classes/jdk/internal/foreign/Utils.java

+16-5
Original file line numberDiff line numberDiff line change
@@ -200,11 +200,8 @@ public static long pointeeByteAlign(AddressLayout addressLayout) {
200200
}
201201

202202
public static void checkAllocationSizeAndAlign(long byteSize, long byteAlignment) {
203-
// size should be >= 0
204-
if (byteSize < 0) {
205-
throw new IllegalArgumentException("Invalid allocation size : " + byteSize);
206-
}
207-
203+
// byteSize should be >= 0
204+
Utils.checkNonNegativeArgument(byteSize, "allocation size");
208205
checkAlign(byteAlignment);
209206
}
210207

@@ -216,6 +213,20 @@ public static void checkAlign(long byteAlignment) {
216213
}
217214
}
218215

216+
@ForceInline
217+
public static void checkNonNegativeArgument(long value, String name) {
218+
if (value < 0) {
219+
throw new IllegalArgumentException("The provided " + name + " is negative: " + value);
220+
}
221+
}
222+
223+
@ForceInline
224+
public static void checkNonNegativeIndex(long value, String name) {
225+
if (value < 0) {
226+
throw new IndexOutOfBoundsException("The provided " + name + " is negative: " + value);
227+
}
228+
}
229+
219230
private static long computePadding(long offset, long align) {
220231
boolean isAligned = offset == 0 || offset % align == 0;
221232
if (isAligned) {

‎src/java.base/share/classes/jdk/internal/foreign/layout/AbstractLayout.java

+2-7
Original file line numberDiff line numberDiff line change
@@ -151,13 +151,8 @@ private static long requirePowerOfTwoAndGreaterOrEqualToOne(long value) {
151151
}
152152

153153
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");
161156
return Math.addExact(offset, Math.multiplyExact(byteSize(), index));
162157
}
163158

‎src/java.base/share/classes/jdk/internal/foreign/layout/MemoryLayoutUtil.java

-7
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,6 @@ public final class MemoryLayoutUtil {
3030
private MemoryLayoutUtil() {
3131
}
3232

33-
public static long requireNonNegative(long value) {
34-
if (value < 0) {
35-
throw new IllegalArgumentException("The provided value was negative: " + value);
36-
}
37-
return value;
38-
}
39-
4033
public static long requireByteSizeValid(long byteSize, boolean allowZero) {
4134
if ((byteSize == 0 && !allowZero) || byteSize < 0) {
4235
throw new IllegalArgumentException("Invalid byte size: " + byteSize);

‎test/jdk/java/foreign/TestLayouts.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -371,13 +371,13 @@ public void testVarHandleCaching() {
371371
}
372372

373373
@Test(expectedExceptions=IllegalArgumentException.class,
374-
expectedExceptionsMessageRegExp=".*Negative offset.*")
374+
expectedExceptionsMessageRegExp=".*offset is negative.*")
375375
public void testScaleNegativeOffset() {
376376
JAVA_INT.scale(-1, 0);
377377
}
378378

379379
@Test(expectedExceptions=IllegalArgumentException.class,
380-
expectedExceptionsMessageRegExp=".*Negative index.*")
380+
expectedExceptionsMessageRegExp=".*index is negative.*")
381381
public void testScaleNegativeIndex() {
382382
JAVA_INT.scale(0, -1);
383383
}

‎test/jdk/java/foreign/TestMemoryAccessInstance.java

+7
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,13 @@ public <X, L extends ValueLayout> void badAccessOverflowInIndexedAccess(String t
164164
}
165165
}
166166

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+
167174
static final ByteOrder NE = ByteOrder.nativeOrder();
168175

169176
@DataProvider(name = "segmentAccessors")

‎test/jdk/java/foreign/TestScopedOperations.java

+3
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
*/
2828

2929
import java.lang.foreign.Arena;
30+
import java.lang.foreign.MemoryLayout;
3031
import java.lang.foreign.MemorySegment;
3132
import java.lang.foreign.ValueLayout;
3233

@@ -135,6 +136,8 @@ public <Z> void testOpOutsideConfinement(String name, ScopedOperation<Z> scopedO
135136
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_FLOAT, new float[]{0}), "Arena::allocateFrom/float");
136137
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_LONG, new long[]{0}), "Arena::allocateFrom/long");
137138
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_DOUBLE, new double[]{0}), "Arena::allocateFrom/double");
139+
var source = MemorySegment.ofArray(new byte[]{});
140+
ScopedOperation.ofScope(a -> a.allocateFrom(ValueLayout.JAVA_INT, source, JAVA_BYTE, 0, 1), "Arena::allocateFrom/5arg");
138141
};
139142

140143
@DataProvider(name = "scopedOperations")

‎test/jdk/java/foreign/TestSegmentAllocators.java

+77
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@
4444
import java.nio.ShortBuffer;
4545
import java.util.ArrayList;
4646
import java.util.List;
47+
import java.util.concurrent.CompletableFuture;
48+
import java.util.concurrent.ExecutionException;
4749
import java.util.concurrent.atomic.AtomicInteger;
4850
import java.util.function.BiFunction;
4951
import java.util.function.Function;
@@ -189,6 +191,81 @@ public void testAllocatorAllocateFromHeapSegment() {
189191
}
190192
}
191193

194+
// Invariant checking tests for the SegmentAllocator method:
195+
// MemorySegment allocateFrom(ValueLayout elementLayout,
196+
// MemorySegment source,
197+
// ValueLayout sourceElementLayout,
198+
// long sourceOffset,
199+
// long elementCount) {
200+
@Test
201+
public void testAllocatorAllocateFromArguments() {
202+
try (Arena arena = Arena.ofConfined()) {
203+
var sourceElements = 2;
204+
var source = arena.allocate(ValueLayout.JAVA_LONG, sourceElements);
205+
var elementLayout = ValueLayout.JAVA_INT;
206+
var sourceElementLayout = ValueLayout.JAVA_INT;
207+
208+
// IllegalArgumentException if {@code elementLayout.byteSize() != sourceElementLayout.byteSize()}
209+
assertThrows(IllegalArgumentException.class, () ->
210+
arena.allocateFrom(elementLayout, source, ValueLayout.JAVA_BYTE, 0, 1)
211+
);
212+
213+
// IllegalArgumentException if source segment/offset
214+
// are <a href="MemorySegment.html#segment-alignment">incompatible with the alignment constraint</a>
215+
// in the source element layout
216+
assertThrows(IllegalArgumentException.class, () ->
217+
arena.allocateFrom(elementLayout, source.asSlice(1), sourceElementLayout, 0, 1)
218+
);
219+
assertThrows(IllegalArgumentException.class, () ->
220+
arena.allocateFrom(elementLayout, source, sourceElementLayout, 1, 1)
221+
);
222+
223+
// IllegalArgumentException if {@code elementLayout.byteAlignment() > elementLayout.byteSize()}
224+
assertThrows(IllegalArgumentException.class, () ->
225+
arena.allocateFrom(elementLayout.withByteAlignment(elementLayout.byteAlignment() * 2), source, sourceElementLayout, 1, 1)
226+
);
227+
228+
// IllegalStateException if the {@linkplain MemorySegment#scope() scope} associated
229+
// with {@code source} is not {@linkplain MemorySegment.Scope#isAlive() alive}
230+
// This is tested in TestScopedOperations
231+
232+
// WrongThreadException if this method is called from a thread {@code T},
233+
// such that {@code source.isAccessibleBy(T) == false}
234+
CompletableFuture<Arena> future = CompletableFuture.supplyAsync(Arena::ofConfined);
235+
try {
236+
Arena otherThreadArena = future.get();
237+
assertThrows(WrongThreadException.class, () ->
238+
otherThreadArena.allocateFrom(elementLayout, source, sourceElementLayout, 0, 1)
239+
);
240+
} catch (ExecutionException | InterruptedException e) {
241+
fail("Unable to create arena", e);
242+
}
243+
244+
// IllegalArgumentException if {@code elementCount * sourceElementLayout.byteSize()} overflows
245+
assertThrows(IllegalArgumentException.class, () ->
246+
arena.allocateFrom(elementLayout, source, sourceElementLayout, 0, Long.MAX_VALUE)
247+
);
248+
249+
// IndexOutOfBoundsException if {@code sourceOffset > source.byteSize() - (elementCount * sourceElementLayout.byteSize())}
250+
assertThrows(IndexOutOfBoundsException.class, () ->
251+
arena.allocateFrom(elementLayout, source, sourceElementLayout, source.byteSize() - (1 * sourceElementLayout.byteAlignment()) + elementLayout.byteSize(), 1)
252+
);
253+
254+
// IndexOutOfBoundsException if {@code sourceOffset < 0}
255+
assertThrows(IndexOutOfBoundsException.class, () ->
256+
arena.allocateFrom(elementLayout, source, sourceElementLayout, -elementLayout.byteSize(), 1)
257+
);
258+
259+
// IllegalArgumentException if {@code elementCount < 0}
260+
assertThrows(IllegalArgumentException.class, () ->
261+
arena.allocateFrom(elementLayout, source, sourceElementLayout, 0, -1)
262+
);
263+
264+
265+
}
266+
}
267+
268+
192269
@Test
193270
public void testArrayAllocateDelegation() {
194271
AtomicInteger calls = new AtomicInteger();

‎test/jdk/java/foreign/TestSegmentCopy.java

+60
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,66 @@ public void testHyperAlignedDst() {
145145
MemorySegment.copy(segment, JAVA_BYTE.withByteAlignment(2), 0, segment, 0, 4);
146146
}
147147

148+
@Test
149+
public void testCopy5ArgWithNegativeValues() {
150+
MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
151+
MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
152+
assertThrows(IndexOutOfBoundsException.class, () ->
153+
MemorySegment.copy(src, -1, dst, 0, 4)
154+
);
155+
assertThrows(IndexOutOfBoundsException.class, () ->
156+
MemorySegment.copy(src, 0, dst, -1, 4)
157+
);
158+
assertThrows(IndexOutOfBoundsException.class, () ->
159+
MemorySegment.copy(src, 0, dst, 0, -1)
160+
);
161+
}
162+
163+
@Test
164+
public void testCopy7ArgWithNegativeValues() {
165+
MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
166+
MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
167+
assertThrows(IndexOutOfBoundsException.class, () ->
168+
MemorySegment.copy(src, JAVA_BYTE, -1, dst, JAVA_BYTE, 0, 4)
169+
);
170+
assertThrows(IndexOutOfBoundsException.class, () ->
171+
MemorySegment.copy(src, JAVA_BYTE, 0, dst, JAVA_BYTE, -1, 4)
172+
);
173+
assertThrows(IndexOutOfBoundsException.class, () ->
174+
MemorySegment.copy(src, JAVA_BYTE, 0, dst, JAVA_BYTE, 0, -1)
175+
);
176+
}
177+
178+
@Test
179+
public void testCopyFromArrayWithNegativeValues() {
180+
MemorySegment src = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
181+
byte[] dst = new byte[] {1, 2, 3, 4};
182+
assertThrows(IndexOutOfBoundsException.class, () ->
183+
MemorySegment.copy(src, JAVA_BYTE, -1, dst, 0, 4)
184+
);
185+
assertThrows(IndexOutOfBoundsException.class, () ->
186+
MemorySegment.copy(src, JAVA_BYTE, 0, dst, -1, 4)
187+
);
188+
assertThrows(IndexOutOfBoundsException.class, () ->
189+
MemorySegment.copy(src, JAVA_BYTE, 0, dst, 0, -1)
190+
);
191+
}
192+
193+
@Test
194+
public void testCopyToArrayWithNegativeValues() {
195+
byte[] src = new byte[] {1, 2, 3, 4};
196+
MemorySegment dst = MemorySegment.ofArray(new byte[] {1, 2, 3, 4});
197+
assertThrows(IndexOutOfBoundsException.class, () ->
198+
MemorySegment.copy(src, -1, dst, JAVA_BYTE, 0, 4)
199+
);
200+
assertThrows(IndexOutOfBoundsException.class, () ->
201+
MemorySegment.copy(src, 0, dst, JAVA_BYTE, -1, 4)
202+
);
203+
assertThrows(IndexOutOfBoundsException.class, () ->
204+
MemorySegment.copy(src, 0, dst, JAVA_BYTE, 0, -1)
205+
);
206+
}
207+
148208
enum Type {
149209
// Byte
150210
BYTE(byte.class, JAVA_BYTE, i -> (byte)i),

‎test/jdk/java/foreign/TestSegments.java

+5-6
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import java.util.function.Supplier;
4444

4545
import static java.lang.foreign.ValueLayout.JAVA_INT;
46+
import static java.lang.foreign.ValueLayout.JAVA_LONG;
4647
import static org.testng.Assert.*;
4748

4849
public class TestSegments {
@@ -55,14 +56,13 @@ public void testBadAllocateAlign(long size, long align) {
5556
@Test
5657
public void testZeroLengthNativeSegment() {
5758
try (Arena arena = Arena.ofConfined()) {
58-
Arena session = arena;
59-
var segment = session.allocate(0, 1);
59+
var segment = arena.allocate(0, 1);
6060
assertEquals(segment.byteSize(), 0);
6161
MemoryLayout seq = MemoryLayout.sequenceLayout(0, JAVA_INT);
62-
segment = session.allocate(seq);
62+
segment = arena.allocate(seq);
6363
assertEquals(segment.byteSize(), 0);
6464
assertEquals(segment.address() % seq.byteAlignment(), 0);
65-
segment = session.allocate(0, 4);
65+
segment = arena.allocate(0, 4);
6666
assertEquals(segment.byteSize(), 0);
6767
assertEquals(segment.address() % 4, 0);
6868
MemorySegment rawAddress = MemorySegment.ofAddress(segment.address());
@@ -133,8 +133,7 @@ public void testDerivedScopes(Supplier<MemorySegment> segmentSupplier) {
133133
@Test
134134
public void testEqualsOffHeap() {
135135
try (Arena arena = Arena.ofConfined()) {
136-
Arena scope1 = arena;
137-
MemorySegment segment = scope1.allocate(100, 1);
136+
MemorySegment segment = arena.allocate(100, 1);
138137
assertEquals(segment, segment.asReadOnly());
139138
assertEquals(segment, segment.asSlice(0, 100));
140139
assertNotEquals(segment, segment.asSlice(10, 90));

0 commit comments

Comments
 (0)
Please sign in to comment.