Ensure pointers indirected from Memory and pointing into Memory retai…

…n originating object

The use case is this:

- native supplies the required size of a memory region
- a Memory object is created with the right memory size
- native fills the memory with a structure _and_ additional objects,
  that are held in that region, but are only referenced from the
  structure (one such example would be a Structure.ByReference)

As long as the resulting structure stays strongly referenced from Java,
all is good. When the toplevel structure goes ouf of scope, the memory
backing the strucure is not strongly referenced anymore and will be
freeed by GC.

This change fixes the issue by ensuring, that the pointers used in
substructures are SharedMemory objects, holding a strong references to
the original Memory object.

.gitignore

@@ -30,4 +30,5 @@ pom-jna-platform.xml.asc
 /contrib/platform/${build.generated.sources.dir}/
 /contrib/platform/${build}/
 /contrib/platform/nbproject/private/
-/nbproject/private/
+/nbproject/private/
+/native/.ccls-cache/

CHANGES.md

@@ -14,6 +14,7 @@ Bug Fixes
 ---------
 * [#1317](https://github.com/java-native-access/jna/pull/1317): Change the maven coordinates of the JPMS artifacts from classifier `jpms` to custom artifact ids `jna-jpms` and `jna-platform-jpms` - [@matthiasblaesing](https://github.com/matthiasblaesing).
 * [#1322](https://github.com/java-native-access/jna/pull/1322): Handle 0-length domain names in `c.s.j.p.win32.Advapi32Util#getAccountBySid` - [@dbwiddis](https://github.com/dbwiddis).
+* [#1326](https://github.com/java-native-access/jna/pull/1326): Ensure pointers indirected from Memory and pointing into Memory retain originating object - [@matthiasblaesing](https://github.com/matthiasblaesing).
 
 Important Changes
 -----------------

src/com/sun/jna/Memory.java

@@ -400,7 +400,7 @@ public void read(long bOff, short[] buf, int index, int length) {
      */
     @Override
     public void read(long bOff, char[] buf, int index, int length) {
-        boundsCheck(bOff, length * 2L);
+        boundsCheck(bOff, length * Native.WCHAR_SIZE);
         super.read(bOff, buf, index, length);
     }
 
@@ -460,6 +460,20 @@ public void read(long bOff, double[] buf, int index, int length) {
         super.read(bOff, buf, index, length);
     }
 
+    /**
+     * Indirect the native pointer to <code>malloc</code> space, a la
+     * <code>Pointer.read</code>.  But this method performs a bounds checks to
+     * ensure that the indirection does not cause memory outside the
+     * <code>malloc</code>ed space to be accessed.
+     *
+     * @see Pointer#read(long,Pointer[],int,int)
+     */
+    @Override
+    public void read(long bOff, Pointer[] buf, int index, int length) {
+        boundsCheck(bOff, length * Native.POINTER_SIZE);
+        super.read(bOff, buf, index, length);
+    }
+
     //////////////////////////////////////////////////////////////////////////
     // Raw write methods
     //////////////////////////////////////////////////////////////////////////
@@ -502,7 +516,7 @@ public void write(long bOff, short[] buf, int index, int length) {
      */
     @Override
     public void write(long bOff, char[] buf, int index, int length) {
-        boundsCheck(bOff, length * 2L);
+        boundsCheck(bOff, length * Native.WCHAR_SIZE);
         super.write(bOff, buf, index, length);
     }
 
@@ -562,6 +576,20 @@ public void write(long bOff, double[] buf, int index, int length) {
         super.write(bOff, buf, index, length);
     }
 
+    /**
+     * Indirect the native pointer to <code>malloc</code> space, a la
+     * <code>Pointer.write</code>.  But this method performs a bounds
+     * checks to ensure that the indirection does not cause memory outside the
+     * <code>malloc</code>ed space to be accessed.
+     *
+     * @see Pointer#write(long,Pointer[],int,int)
+     */
+    @Override
+    public void write(long bOff, Pointer[] buf, int index, int length) {
+        boundsCheck(bOff, length * Native.POINTER_SIZE);
+        super.write(bOff, buf, index, length);
+    }
+
     //////////////////////////////////////////////////////////////////////////
     // Java type read methods
     //////////////////////////////////////////////////////////////////////////
@@ -590,7 +618,7 @@ public byte getByte(long offset) {
      */
     @Override
     public char getChar(long offset) {
-        boundsCheck(offset, 1);
+        boundsCheck(offset, Native.WCHAR_SIZE);
         return super.getChar(offset);
     }
 
@@ -675,7 +703,7 @@ public double getDouble(long offset) {
     @Override
     public Pointer getPointer(long offset) {
         boundsCheck(offset, Native.POINTER_SIZE);
-        return super.getPointer(offset);
+        return shareReferenceIfInBounds(super.getPointer(offset));
     }
 
     /**
@@ -862,4 +890,26 @@ protected static long malloc(long size) {
     public String dump() {
         return dump(0, (int)size());
     }
+
+    /**
+     * Check whether the supplied Pointer object points into the memory region
+     * backed by this memory object. The intention is to prevent premature GC
+     * of the Memory object.
+     *
+     * @param target Pointer to check
+     * @return {@code target} if target does not point into the region covered
+     * by this memory object, a newly {@code SharedMemory} object, if the pointer
+     * points to memory backed by this Memory object.
+     */
+    private Pointer shareReferenceIfInBounds(Pointer target) {
+        if(target == null) {
+            return null;
+        }
+        long offset = target.peer - this.peer;
+        if (offset >= 0 && offset < this.size) {
+            return this.share(offset);
+        } else {
+            return target;
+        }
+    }
 }

test/com/sun/jna/MemoryTest.java

@@ -23,11 +23,19 @@
  */
 package com.sun.jna;
 
+
 import java.lang.ref.Reference;
 import java.lang.ref.WeakReference;
+import java.lang.reflect.Array;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.nio.ByteBuffer;
-
 import junit.framework.TestCase;
+import org.junit.Assert;
+
+import static com.sun.jna.Native.POINTER_SIZE;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.not;
 
 public class MemoryTest extends TestCase {
 
@@ -199,6 +207,166 @@ public void testRemoveAllocatedMemory() {
         assertEquals(0, Memory.integrityCheck());
     }
 
+    public void testGetSharedMemory() {
+        Memory mem1 = new Memory(512);
+        Memory mem2 = new Memory(512);
+
+        // Get pointers into the two memory objects
+        Pointer stringStart1 = mem1.share(128);
+        Pointer stringStart2 = mem2.share(128);
+
+        mem1.setPointer(10 * POINTER_SIZE, stringStart1);
+        mem1.setPointer(11 * POINTER_SIZE, stringStart2);
+
+        // The pointer in mem1 at offset 10 * POINTER_SIZE points into the
+        // memory region of mem1, while the pointer at offset 11 * POINTER_SIZE
+        // points to the second memory region
+
+        // It is expected, that resolution of the first pointer results in
+        // an instance of SharedMemory (a subclass of Memory, that retains a
+        // reference on the originating Memory object)
+        Assert.assertThat(mem1.getPointer(10 * POINTER_SIZE), instanceOf(Pointer.class));
+        Assert.assertThat(mem1.getPointer(10 * POINTER_SIZE), instanceOf(Memory.class));
+        // The second pointer lies outside of memory 1, so it must not be a
+        // Memory object, but a raw pointer
+        Assert.assertThat(mem1.getPointer(11 * POINTER_SIZE), instanceOf(Pointer.class));
+        Assert.assertThat(mem1.getPointer(11 * POINTER_SIZE), not(instanceOf(Memory.class)));
+
+        // It is expected, that Memory#read called for pointers shows the same
+        // behaviour as direct calls to getPointer calls with the corresponding
+        // offsets
+        Pointer[] pointers = new Pointer[2];
+        mem1.read(10 * POINTER_SIZE, pointers, 0, 2);
+
+        Assert.assertThat(pointers[0], instanceOf(Pointer.class));
+        Assert.assertThat(pointers[0], instanceOf(Memory.class));
+        Assert.assertThat(pointers[1], instanceOf(Pointer.class));
+        Assert.assertThat(pointers[1], not(instanceOf(Memory.class)));
+    }
+
+    public void testBoundsChecking() throws NoSuchMethodException, IllegalAccessException, IllegalArgumentException, InvocationTargetException {
+        // Test the bounds checking of the Memory#read invocations
+        testBoundsCheckArray(byte.class, 1);
+        testBoundsCheckArray(char.class, Native.WCHAR_SIZE);
+        testBoundsCheckArray(short.class, 2);
+        testBoundsCheckArray(int.class, 4);
+        testBoundsCheckArray(long.class, 8);
+        testBoundsCheckArray(float.class, 4);
+        testBoundsCheckArray(double.class, 8);
+        testBoundsCheckArray(Pointer.class, Native.POINTER_SIZE);
+        // Test the bounds checking of the Memory#get* / Memory#set* methods
+        testBoundsCheckSingleValue(byte.class, "Byte", 1, (byte) 42);
+        testBoundsCheckSingleValue(char.class, "Char", Native.WCHAR_SIZE, 'x');
+        testBoundsCheckSingleValue(short.class, "Short", 2, (short) 42);
+        testBoundsCheckSingleValue(int.class, "Int", 4, 42);
+        testBoundsCheckSingleValue(long.class, "Long", 8, 42L);
+        testBoundsCheckSingleValue(float.class, "Float", 4, 42f);
+        testBoundsCheckSingleValue(double.class, "Double", 8, 42d);
+        testBoundsCheckSingleValue(Pointer.class, "Pointer", Native.POINTER_SIZE, Pointer.createConstant(42L));
+    }
+
+    private void testBoundsCheckSingleValue(Class clazz, String name, int elementSize, Object value) throws NoSuchMethodException, IllegalAccessException, IllegalArgumentException, InvocationTargetException {
+        Method readMethod = Memory.class.getMethod("get" + name, new Class[]{long.class});
+        Method writeMethod = Memory.class.getMethod("set" + name, new Class[]{long.class, clazz});
+
+        Memory mem = new Memory(elementSize * 10);
+        readMethod.invoke(mem, elementSize * 0);
+        readMethod.invoke(mem, elementSize * 8);
+        try {
+            readMethod.invoke(mem, elementSize * -1);
+            fail("Negative offset was read");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            readMethod.invoke(mem, elementSize * 10 - (elementSize / 2));
+            fail("Value lies half outside the memory location");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            readMethod.invoke(mem, elementSize * 20);
+            fail("Read outsize allocated memory");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+
+        writeMethod.invoke(mem, elementSize * 0, value);
+        writeMethod.invoke(mem, elementSize * 8, value);
+        try {
+            writeMethod.invoke(mem, elementSize * -1, value);
+            fail("Negative offset was read");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            writeMethod.invoke(mem, elementSize * 10 - (elementSize / 2), value);
+            fail("Value lies half outside the memory location");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            writeMethod.invoke(mem, elementSize * 20, value);
+            fail("Write outsize allocated memory");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+    }
+
+    private void testBoundsCheckArray(Class componentClass, int elementSize) throws NoSuchMethodException, IllegalAccessException, IllegalArgumentException, InvocationTargetException {
+        Object javaArray = Array.newInstance(componentClass, 2);
+        Method readMethod = Memory.class.getMethod("read", new Class[]{long.class, javaArray.getClass(), int.class, int.class});
+        Method writeMethod = Memory.class.getMethod("write", new Class[]{long.class, javaArray.getClass(), int.class, int.class});
+
+        Memory mem = new Memory(elementSize * 10);
+
+        readMethod.invoke(mem, elementSize * 0, javaArray, 0, 2);
+        readMethod.invoke(mem, elementSize * 8, javaArray, 0, 2);
+        try {
+            readMethod.invoke(mem, elementSize * -1, javaArray, 0, 2);
+            fail("Negative offset was read");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            readMethod.invoke(mem, elementSize * 9, javaArray, 0, 2);
+            fail("Half of the array contents layed outside the allocated area");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            readMethod.invoke(mem, elementSize * 20, javaArray, 0, 2);
+            fail("Array contents layed completely outside the allocated area");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+
+        writeMethod.invoke(mem, 0, javaArray, 0, 2);
+        writeMethod.invoke(mem, elementSize * 8, javaArray, 0, 2);
+        try {
+            writeMethod.invoke(mem, elementSize * -1, javaArray, 0, 2);
+            fail("Negative offset was read");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            writeMethod.invoke(mem, elementSize * 9, javaArray, 0, 2);
+            fail("Half of the array contents layed outside the allocated area");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+        try {
+            writeMethod.invoke(mem, elementSize * 20, javaArray, 0, 2);
+            fail("Array contents layed completely outside the allocated area");
+        } catch (InvocationTargetException ex) {
+            checkCauseInstance(ex, IndexOutOfBoundsException.class);
+        }
+    }
+
+    private void checkCauseInstance(Exception ex, Class<? extends Exception> expectedClazz) {
+        Assert.assertThat(ex.getCause(), instanceOf(expectedClazz));
+    }
+
     public static void main(String[] args) {
         junit.textui.TestRunner.run(MemoryTest.class);
     }