8310053: VarHandle and slice handle derived from layout are lacking a…

…lignment check

Reviewed-by: mcimadamore
Backport-of: e022e876543b65b531027662326f35b497861f33

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

@@ -380,6 +380,10 @@ default MethodHandle byteOffsetHandle(PathElement... elements) {
      * 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.
      * <p>
+     * The base address must be <a href="MemorySegment.html#segment-alignment">aligned</a> 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.
+     * <p>
      * Multiple paths can be chained, with <a href=#deref-path-elements>dereference path elements</a>.
      * 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 @@ default VarHandle varHandle(PathElement... elements) {
      * long size = select(elements).byteSize();
      * MemorySegment slice = segment.asSlice(offset, size);
      * }
+     * <p>
+     * The segment to be sliced must be <a href="MemorySegment.html#segment-alignment">aligned</a> 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.

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 VarHandle dereferenceHandle(boolean adapt) {
             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 MethodHandle offsetHandle() {
         return mh;
     }
 
+    private MemoryLayout rootLayout() {
+        return enclosing != null ? enclosing.rootLayout() : this.layout;
+    }
+
     public MethodHandle sliceHandle() {
-        MethodHandle offsetHandle = offsetHandle(); // byte offset
+        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
 
-        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
+        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;
     }

test/jdk/java/foreign/TestDereferencePath.java

@@ -143,4 +143,16 @@ void testBadDerefInSlice() {
     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
+        }
+    }
 }

test/jdk/java/foreign/TestLayoutPaths.java

@@ -31,17 +31,18 @@
 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;
 
 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 void testByteOffsetHandleBadRange() {
         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 void testSliceHandle(MemoryLayout layout, PathElement[] pathElements, lon
     }
 
 }
-