From e022e876543b65b531027662326f35b497861f33 Mon Sep 17 00:00:00 2001 From: Jorn Vernee Date: Wed, 21 Jun 2023 00:03:13 +0000 Subject: [PATCH] 8310053: VarHandle and slice handle derived from layout are lacking alignment check Reviewed-by: mcimadamore --- .../java/lang/foreign/MemoryLayout.java | 8 +++ .../jdk/internal/foreign/LayoutPath.java | 59 ++++++++++++++----- .../jdk/java/foreign/TestDereferencePath.java | 12 ++++ test/jdk/java/foreign/TestLayoutPaths.java | 32 +++++++++- 4 files changed, 94 insertions(+), 17 deletions(-) diff --git a/src/java.base/share/classes/java/lang/foreign/MemoryLayout.java b/src/java.base/share/classes/java/lang/foreign/MemoryLayout.java index 10f57aa436a..41093a1cc32 100644 --- a/src/java.base/share/classes/java/lang/foreign/MemoryLayout.java +++ b/src/java.base/share/classes/java/lang/foreign/MemoryLayout.java @@ -380,6 +380,10 @@ public sealed interface MemoryLayout permits SequenceLayout, GroupLayout, Paddin * Additionally, the provided dynamic values must conform to bounds which are derived from the layout path, that is, * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link IndexOutOfBoundsException} is thrown. *

+ * The base address must be aligned according to the {@linkplain + * #byteAlignment() alignment constraint} of the root layout (this layout). Note that this can be more strict + * (but not less) than the alignment constraint of the selected value layout. + *

* Multiple paths can be chained, with dereference path elements. * A dereference path element constructs a fresh native memory segment whose base address is the address value * read obtained by accessing a memory segment at the offset determined by the layout path elements immediately preceding @@ -436,6 +440,10 @@ public sealed interface MemoryLayout permits SequenceLayout, GroupLayout, Paddin * long size = select(elements).byteSize(); * MemorySegment slice = segment.asSlice(offset, size); * } + *

+ * The segment to be sliced must be aligned according to the + * {@linkplain #byteAlignment() alignment constraint} of the root layout (this layout). Note that this can be more + * strict (but not less) than the alignment constraint of the selected value layout. * * @apiNote The returned method handle can be used to obtain a memory segment slice, similarly to {@link MemorySegment#asSlice(long, long)}, * but more flexibly, as some indices can be specified when invoking the method handle. diff --git a/src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java b/src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java index b893a759d56..f368a8b7f23 100644 --- a/src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java +++ b/src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java @@ -58,6 +58,8 @@ public class LayoutPath { private static final MethodHandle MH_ADD_SCALED_OFFSET; private static final MethodHandle MH_SLICE; + private static final MethodHandle MH_SLICE_LAYOUT; + private static final MethodHandle MH_CHECK_ALIGN; private static final MethodHandle MH_SEGMENT_RESIZE; static { @@ -67,6 +69,10 @@ public class LayoutPath { MethodType.methodType(long.class, long.class, long.class, long.class, long.class)); MH_SLICE = lookup.findVirtual(MemorySegment.class, "asSlice", MethodType.methodType(MemorySegment.class, long.class, long.class)); + MH_SLICE_LAYOUT = lookup.findVirtual(MemorySegment.class, "asSlice", + MethodType.methodType(MemorySegment.class, long.class, MemoryLayout.class)); + MH_CHECK_ALIGN = lookup.findStatic(LayoutPath.class, "checkAlign", + MethodType.methodType(MemorySegment.class, MemorySegment.class, MemoryLayout.class)); MH_SEGMENT_RESIZE = lookup.findStatic(LayoutPath.class, "resizeSegment", MethodType.methodType(MemorySegment.class, MemorySegment.class, MemoryLayout.class)); } catch (Throwable ex) { @@ -193,17 +199,19 @@ public class LayoutPath { throw new IllegalArgumentException("Path does not select a value layout"); } - VarHandle handle = Utils.makeSegmentViewVarHandle(valueLayout); - for (int i = strides.length - 1; i >= 0; i--) { - MethodHandle collector = MethodHandles.insertArguments(MH_ADD_SCALED_OFFSET, 2, - strides[i], - bounds[i]); - // (J, ...) -> J to (J, J, ...) -> J - // i.e. new coord is prefixed. Last coord will correspond to innermost layout - handle = MethodHandles.collectCoordinates(handle, 1, collector); + // If we have an enclosing layout, drop the alignment check for the accessed element, + // we check the root layout instead + ValueLayout accessedLayout = enclosing != null ? valueLayout.withByteAlignment(1) : valueLayout; + VarHandle handle = Utils.makeSegmentViewVarHandle(accessedLayout); + handle = MethodHandles.collectCoordinates(handle, 1, offsetHandle()); + + // we only have to check the alignment of the root layout for the first dereference we do, + // as each dereference checks the alignment of the target address when constructing its segment + // (see Utils::longToAddress) + if (derefAdapters.length == 0 && enclosing != null) { + MethodHandle checkHandle = MethodHandles.insertArguments(MH_CHECK_ALIGN, 1, rootLayout()); + handle = MethodHandles.filterCoordinates(handle, 0, checkHandle); } - handle = MethodHandles.insertCoordinates(handle, 1, - offset); if (adapt) { for (int i = derefAdapters.length; i > 0; i--) { @@ -231,16 +239,37 @@ public class LayoutPath { return mh; } - public MethodHandle sliceHandle() { - MethodHandle offsetHandle = offsetHandle(); // byte offset + private MemoryLayout rootLayout() { + return enclosing != null ? enclosing.rootLayout() : this.layout; + } - MethodHandle sliceHandle = MH_SLICE; // (MS, long, long) -> MS - sliceHandle = MethodHandles.insertArguments(sliceHandle, 2, layout.byteSize()); // (MS, long) -> MS - sliceHandle = MethodHandles.collectArguments(sliceHandle, 1, offsetHandle); // (MS, ...) -> MS + public MethodHandle sliceHandle() { + MethodHandle sliceHandle; + if (enclosing != null) { + // drop the alignment check for the accessed element, we check the root layout instead + sliceHandle = MH_SLICE; // (MS, long, long) -> MS + sliceHandle = MethodHandles.insertArguments(sliceHandle, 2, layout.byteSize()); // (MS, long) -> MS + } else { + sliceHandle = MH_SLICE_LAYOUT; // (MS, long, MemoryLayout) -> MS + sliceHandle = MethodHandles.insertArguments(sliceHandle, 2, layout); // (MS, long) -> MS + } + sliceHandle = MethodHandles.collectArguments(sliceHandle, 1, offsetHandle()); // (MS, ...) -> MS + + if (enclosing != null) { + MethodHandle checkHandle = MethodHandles.insertArguments(MH_CHECK_ALIGN, 1, rootLayout()); + sliceHandle = MethodHandles.filterArguments(sliceHandle, 0, checkHandle); + } return sliceHandle; } + private static MemorySegment checkAlign(MemorySegment segment, MemoryLayout constraint) { + if (!((AbstractMemorySegmentImpl) segment).isAlignedForElement(0, constraint)) { + throw new IllegalArgumentException("Target offset incompatible with alignment constraints: " + constraint.byteAlignment()); + } + return segment; + } + public MemoryLayout layout() { return layout; } diff --git a/test/jdk/java/foreign/TestDereferencePath.java b/test/jdk/java/foreign/TestDereferencePath.java index 7962322081e..1c129032053 100644 --- a/test/jdk/java/foreign/TestDereferencePath.java +++ b/test/jdk/java/foreign/TestDereferencePath.java @@ -143,4 +143,16 @@ public class TestDereferencePath { void badDerefAddressNoTarget() { A_MULTI_NO_TARGET.varHandle(PathElement.groupElement("bs"), PathElement.dereferenceElement()); } + + @Test(expectedExceptions = IllegalArgumentException.class) + void badDerefMisAligned() { + MemoryLayout struct = MemoryLayout.structLayout( + ValueLayout.ADDRESS.withTargetLayout(ValueLayout.JAVA_INT).withName("x")); + + try (Arena arena = Arena.ofConfined()) { + MemorySegment segment = arena.allocate(struct.byteSize() + 1).asSlice(1); + VarHandle vhX = struct.varHandle(PathElement.groupElement("x"), PathElement.dereferenceElement()); + vhX.set(segment, 42); // should throw + } + } } diff --git a/test/jdk/java/foreign/TestLayoutPaths.java b/test/jdk/java/foreign/TestLayoutPaths.java index ef6a68f5701..ceac0b2ce02 100644 --- a/test/jdk/java/foreign/TestLayoutPaths.java +++ b/test/jdk/java/foreign/TestLayoutPaths.java @@ -31,10 +31,10 @@ import java.lang.foreign.*; import java.lang.foreign.MemoryLayout.PathElement; -import org.testng.SkipException; import org.testng.annotations.*; import java.lang.invoke.MethodHandle; +import java.lang.invoke.VarHandle; import java.util.ArrayList; import java.util.List; import java.util.function.IntFunction; @@ -42,6 +42,7 @@ import java.util.function.IntFunction; import static java.lang.foreign.MemoryLayout.PathElement.groupElement; import static java.lang.foreign.MemoryLayout.PathElement.sequenceElement; import static java.lang.foreign.ValueLayout.JAVA_INT; +import static java.lang.foreign.ValueLayout.JAVA_SHORT; import static org.testng.Assert.*; public class TestLayoutPaths { @@ -134,6 +135,34 @@ public class TestLayoutPaths { seq.byteOffsetHandle(sequenceElement(5, 1)); // invalid range (starting position is outside the sequence) } + @Test + public void testBadAlignmentOfRoot() throws Throwable { + MemoryLayout struct = MemoryLayout.structLayout( + JAVA_INT, + JAVA_SHORT.withName("x")); + assertEquals(struct.byteAlignment(), 4); + + try (Arena arena = Arena.ofConfined()) { + MemorySegment seg = arena.allocate(struct.byteSize() + 2, struct.byteAlignment()).asSlice(2); + assertEquals(seg.address() % JAVA_SHORT.byteAlignment(), 0); // should be aligned + assertNotEquals(seg.address() % struct.byteAlignment(), 0); // should not be aligned + + String expectedMessage = "Target offset incompatible with alignment constraints: " + struct.byteAlignment(); + + VarHandle vhX = struct.varHandle(groupElement("x")); + IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> { + vhX.set(seg, (short) 42); + }); + assertEquals(iae.getMessage(), expectedMessage); + + MethodHandle sliceX = struct.sliceHandle(groupElement("x")); + iae = expectThrows(IllegalArgumentException.class, () -> { + MemorySegment slice = (MemorySegment) sliceX.invokeExact(seg); + }); + assertEquals(iae.getMessage(), expectedMessage); + } + } + @Test public void testBadSequencePathInOffset() { SequenceLayout seq = MemoryLayout.sequenceLayout(10, JAVA_INT); @@ -338,4 +367,3 @@ public class TestLayoutPaths { } } -