Boundary checks / synchronization / tests for MemoryMappedFile

Hardening MemoryMappedFile against boundary conditions and
edge cases. Many tests added.

Thread safety has been dropped entirely as part of this change:
expected usage is single threaded. It was previously possible
to unmap the file, but the BufferIterator would carry on
attempting to read from the address space.

Motivation: The ZoneInfoDB code will soon be modified to expect
exceptions if reads / writes are out of bounds and it's more
appropriate to make the exceptions part of the MemoryMappedFile
contract.

Bug: 31008728
Test: CtsLibcoreTestCases
Change-Id: I58cba7498c5fcd074054b13dbd6b313d6c1bfbe6

luni/src/main/java/libcore/io/BufferIterator.java

@@ -20,11 +20,12 @@
  * Iterates over big- or little-endian bytes. See {@link MemoryMappedFile#bigEndianIterator} and
  * {@link MemoryMappedFile#littleEndianIterator}.
  *
- * @hide don't make this public without adding bounds checking.
+ * @hide
  */
 public abstract class BufferIterator {
     /**
-     * Seeks to the absolute position {@code offset}, measured in bytes from the start.
+     * Seeks to the absolute position {@code offset}, measured in bytes from the start of the
+     * buffer.
      */
     public abstract void seek(int offset);
 
@@ -33,30 +34,45 @@ public abstract class BufferIterator {
      */
     public abstract void skip(int byteCount);
 
+    /**
+     * Returns the current position of the iterator within the buffer.
+     */
+    public abstract int pos();
+
     /**
      * Copies {@code byteCount} bytes from the current position into {@code dst}, starting at
      * {@code dstOffset}, and advances the current position {@code byteCount} bytes.
+     *
+     * @throws IndexOutOfBoundsException if the read / write would be outside of the buffer / array
      */
     public abstract void readByteArray(byte[] dst, int dstOffset, int byteCount);
 
     /**
      * Returns the byte at the current position, and advances the current position one byte.
+     *
+     * @throws IndexOutOfBoundsException if the read would be outside of the buffer
      */
     public abstract byte readByte();
 
     /**
      * Returns the 32-bit int at the current position, and advances the current position four bytes.
+     *
+     * @throws IndexOutOfBoundsException if the read would be outside of the buffer
      */
     public abstract int readInt();
 
     /**
      * Copies {@code intCount} 32-bit ints from the current position into {@code dst}, starting at
      * {@code dstOffset}, and advances the current position {@code 4 * intCount} bytes.
+     *
+     * @throws IndexOutOfBoundsException if the read / write would be outside of the buffer / array
      */
     public abstract void readIntArray(int[] dst, int dstOffset, int intCount);
 
     /**
      * Returns the 16-bit short at the current position, and advances the current position two bytes.
+     *
+     * @throws IndexOutOfBoundsException if the read would be outside of the buffer
      */
     public abstract short readShort();
 }

luni/src/main/java/libcore/io/MemoryMappedFile.java

@@ -28,31 +28,37 @@
 import static android.system.OsConstants.*;
 
 /**
- * A memory-mapped file. Use {@link #mmap} to map a file, {@link #close} to unmap a file,
+ * A memory-mapped file. Use {@link #mmapRO} to map a file, {@link #close} to unmap a file,
  * and either {@link #bigEndianIterator} or {@link #littleEndianIterator} to get a seekable
- * {@link BufferIterator} over the mapped data.
+ * {@link BufferIterator} over the mapped data. This class is not thread safe.
  */
 public final class MemoryMappedFile implements AutoCloseable {
-    private long address;
-    private final long size;
+    private boolean closed;
+    private final long address;
+    private final int size;
 
-    /**
-     * Use this if you've called {@code mmap} yourself.
-     */
+    /** Public for layoutlib only. */
     public MemoryMappedFile(long address, long size) {
         this.address = address;
-        this.size = size;
+        // For simplicity when bounds checking, only sizes up to Integer.MAX_VALUE are supported.
+        if (size < 0 || size > Integer.MAX_VALUE) {
+            throw new IllegalArgumentException("Unsupported file size=" + size);
+        }
+        this.size = (int) size;
     }
 
     /**
      * Use this to mmap the whole file read-only.
      */
     public static MemoryMappedFile mmapRO(String path) throws ErrnoException {
         FileDescriptor fd = Libcore.os.open(path, O_RDONLY, 0);
-        long size = Libcore.os.fstat(fd).st_size;
-        long address = Libcore.os.mmap(0L, size, PROT_READ, MAP_SHARED, fd, 0);
-        Libcore.os.close(fd);
-        return new MemoryMappedFile(address, size);
+        try {
+            long size = Libcore.os.fstat(fd).st_size;
+            long address = Libcore.os.mmap(0L, size, PROT_READ, MAP_SHARED, fd, 0);
+            return new MemoryMappedFile(address, size);
+        } finally {
+            Libcore.os.close(fd);
+        }
     }
 
     /**
@@ -63,31 +69,45 @@ public static MemoryMappedFile mmapRO(String path) throws ErrnoException {
      * Calling this method invalidates any iterators over this {@code MemoryMappedFile}. It is an
      * error to use such an iterator after calling {@code close}.
      */
-    public synchronized void close() throws ErrnoException {
-        if (address != 0) {
+    public void close() throws ErrnoException {
+        if (!closed) {
+            closed = true;
             Libcore.os.munmap(address, size);
-            address = 0;
         }
     }
 
+    public boolean isClosed() {
+        return closed;
+    }
+
     /**
      * Returns a new iterator that treats the mapped data as big-endian.
      */
     public BufferIterator bigEndianIterator() {
-        return new NioBufferIterator(address, (int) size, ByteOrder.nativeOrder() != ByteOrder.BIG_ENDIAN);
+        return new NioBufferIterator(
+                this, address, size, ByteOrder.nativeOrder() != ByteOrder.BIG_ENDIAN);
     }
 
     /**
      * Returns a new iterator that treats the mapped data as little-endian.
      */
     public BufferIterator littleEndianIterator() {
-        return new NioBufferIterator(address, (int) size, ByteOrder.nativeOrder() != ByteOrder.LITTLE_ENDIAN);
+        return new NioBufferIterator(
+                this, this.address, this.size, ByteOrder.nativeOrder() != ByteOrder.LITTLE_ENDIAN);
+    }
+
+    /** Throws {@link IllegalStateException} if the file is closed. */
+    void checkNotClosed() {
+        if (closed) {
+            throw new IllegalStateException("MemoryMappedFile is closed");
+        }
     }
 
     /**
      * Returns the size in bytes of the memory-mapped region.
      */
-    public long size() {
+    public int size() {
+        checkNotClosed();
         return size;
     }
 }

luni/src/main/java/libcore/io/NioBufferIterator.java

@@ -16,24 +16,37 @@
 
 package libcore.io;
 
-import libcore.io.Memory;
-
 /**
  * Iterates over big- or little-endian bytes on the native heap.
  * See {@link MemoryMappedFile#bigEndianIterator} and {@link MemoryMappedFile#littleEndianIterator}.
  *
- * @hide don't make this public without adding bounds checking.
+ * @hide
  */
 public final class NioBufferIterator extends BufferIterator {
+
+    private final MemoryMappedFile file;
     private final long address;
-    private final int size;
+    private final int length;
     private final boolean swap;
 
     private int position;
 
-    NioBufferIterator(long address, int size, boolean swap) {
+    NioBufferIterator(MemoryMappedFile file, long address, int length, boolean swap) {
+        file.checkNotClosed();
+
+        this.file = file;
         this.address = address;
-        this.size = size;
+
+        if (length < 0) {
+            throw new IllegalArgumentException("length < 0");
+        }
+        final long MAX_VALID_ADDRESS = -1;
+        if (Long.compareUnsigned(address, MAX_VALID_ADDRESS - length) > 0) {
+            throw new IllegalArgumentException(
+                    "length " + length + " would overflow 64-bit address space");
+        }
+        this.length = length;
+
         this.swap = swap;
     }
 
@@ -45,31 +58,78 @@ public void skip(int byteCount) {
         position += byteCount;
     }
 
+    @Override
+    public int pos() {
+        return position;
+    }
+
     public void readByteArray(byte[] dst, int dstOffset, int byteCount) {
+        checkDstBounds(dstOffset, dst.length, byteCount);
+        file.checkNotClosed();
+        checkReadBounds(position, length, byteCount);
         Memory.peekByteArray(address + position, dst, dstOffset, byteCount);
         position += byteCount;
     }
 
     public byte readByte() {
+        file.checkNotClosed();
+        checkReadBounds(position, length, 1);
         byte result = Memory.peekByte(address + position);
         ++position;
         return result;
     }
 
     public int readInt() {
+        file.checkNotClosed();
+        checkReadBounds(position, length, SizeOf.INT);
         int result = Memory.peekInt(address + position, swap);
         position += SizeOf.INT;
         return result;
     }
 
     public void readIntArray(int[] dst, int dstOffset, int intCount) {
+        checkDstBounds(dstOffset, dst.length, intCount);
+        file.checkNotClosed();
+        final int byteCount = SizeOf.INT * intCount;
+        checkReadBounds(position, length, byteCount);
         Memory.peekIntArray(address + position, dst, dstOffset, intCount, swap);
-        position += SizeOf.INT * intCount;
+        position += byteCount;
     }
 
     public short readShort() {
+        file.checkNotClosed();
+        checkReadBounds(position, length, SizeOf.SHORT);
         short result = Memory.peekShort(address + position, swap);
         position += SizeOf.SHORT;
         return result;
     }
+
+    private static void checkReadBounds(int position, int length, int byteCount) {
+        if (position < 0 || byteCount < 0) {
+            throw new IndexOutOfBoundsException(
+                    "Invalid read args: position=" + position + ", byteCount=" + byteCount);
+        }
+        // Use of int here relies on length being an int <= Integer.MAX_VALUE.
+        final int finalReadPos = position + byteCount;
+        if (finalReadPos < 0 || finalReadPos > length) {
+            throw new IndexOutOfBoundsException(
+                    "Read outside range: position=" + position + ", byteCount=" + byteCount
+                            + ", length=" + length);
+        }
+    }
+
+    private static void checkDstBounds(int dstOffset, int dstLength, int count) {
+        if (dstOffset < 0 || count < 0) {
+            throw new IndexOutOfBoundsException(
+                    "Invalid dst args: offset=" + dstLength + ", count=" + count);
+        }
+        // Use of int here relies on dstLength being an int <= Integer.MAX_VALUE, which it has to
+        // be because it's an array length.
+        final int targetPos = dstOffset + count;
+        if (targetPos < 0 || targetPos > dstLength) {
+            throw new IndexOutOfBoundsException(
+                    "Write outside range: dst.length=" + dstLength + ", offset="
+                            + dstOffset + ", count=" + count);
+        }
+    }
 }