Make buffers read-only for all paths

This also moves the responsibility of reading from the file to the
BufferHolder. This seems to make the responsibilities a bit clearer,
although I also method on the buffers could work.

Author: oschwald

src/main/java/com/maxmind/db/Buffer.java

@@ -1,7 +1,5 @@
 package com.maxmind.db;
 
-import java.io.IOException;
-import java.nio.channels.FileChannel;
 import java.nio.charset.CharacterCodingException;
 import java.nio.charset.CharsetDecoder;
 
@@ -12,6 +10,9 @@
  *
  * <p>This interface is designed to provide a long-based API while
  * remaining compatible with the limitations of underlying storage.
+ *
+ * <p>All underlying {@link java.nio.ByteBuffer}s are read-only to prevent
+ * accidental modification of shared data.
  */
 interface Buffer {
     /**
@@ -96,16 +97,6 @@ interface Buffer {
      */
     Buffer duplicate();
 
-    /**
-     * Reads data from the given channel into this buffer starting at the
-     * current position.
-     *
-     * @param channel the file channel
-     * @return the number of bytes read
-     * @throws IOException if an I/O error occurs
-     */
-    long readFrom(FileChannel channel) throws IOException;
-
     /**
      * Decodes the buffer's content into a string using the given decoder.
      *

src/main/java/com/maxmind/db/BufferHolder.java

@@ -23,18 +23,48 @@ final class BufferHolder {
              FileChannel channel = file.getChannel()) {
             long size = channel.size();
             if (mode == FileMode.MEMORY) {
-                Buffer buf;
                 if (size <= chunkSize) {
-                    buf = new SingleBuffer(size);
+                    // Allocate, read, and make read-only
+                    ByteBuffer buffer = ByteBuffer.allocate((int) size);
+                    if (channel.read(buffer) != size) {
+                        throw new IOException("Unable to read "
+                                + database.getName()
+                                + " into memory. Unexpected end of stream.");
+                    }
+                    buffer.flip();
+                    this.buffer = new SingleBuffer(buffer);
                 } else {
-                    buf = new MultiBuffer(size);
-                }
-                if (buf.readFrom(channel) != buf.capacity()) {
-                    throw new IOException("Unable to read "
-                            + database.getName()
-                            + " into memory. Unexpected end of stream.");
+                    // Allocate chunks, read, and make read-only
+                    int fullChunks = (int) (size / chunkSize);
+                    int remainder = (int) (size % chunkSize);
+                    int totalChunks = fullChunks + (remainder > 0 ? 1 : 0);
+                    ByteBuffer[] buffers = new ByteBuffer[totalChunks];
+
+                    for (int i = 0; i < fullChunks; i++) {
+                        buffers[i] = ByteBuffer.allocate(chunkSize);
+                    }
+                    if (remainder > 0) {
+                        buffers[totalChunks - 1] = ByteBuffer.allocate(remainder);
+                    }
+
+                    long totalRead = 0;
+                    for (ByteBuffer buffer : buffers) {
+                        int read = channel.read(buffer);
+                        if (read == -1) {
+                            break;
+                        }
+                        totalRead += read;
+                        buffer.flip();
+                    }
+
+                    if (totalRead != size) {
+                        throw new IOException("Unable to read "
+                                + database.getName()
+                                + " into memory. Unexpected end of stream.");
+                    }
+
+                    this.buffer = new MultiBuffer(buffers, chunkSize);
                 }
-                this.buffer = buf;
             } else {
                 if (size <= chunkSize) {
                     this.buffer = SingleBuffer.mapFromChannel(channel);

src/main/java/com/maxmind/db/MultiBuffer.java

@@ -17,6 +17,9 @@
  * a single logical position and limit across them.
  *
  * <p>Use this when working with databases/files that may exceed 2GB.
+ *
+ * <p>All underlying {@link ByteBuffer}s are read-only to prevent accidental
+ * modification of shared data.
  */
 class MultiBuffer implements Buffer {
 
@@ -30,16 +33,6 @@ class MultiBuffer implements Buffer {
     private long position = 0;
     private long limit;
 
-    /**
-     * Creates a new {@code MultiBuffer} with the given capacity, backed by
-     * heap-allocated {@link ByteBuffer}s.
-     *
-     * @param capacity the total capacity in bytes
-     */
-    public MultiBuffer(long capacity) {
-        this(capacity, DEFAULT_CHUNK_SIZE);
-    }
-
     /**
      * Creates a new {@code MultiBuffer} backed by the given
      * {@link ByteBuffer}s.
@@ -64,43 +57,19 @@ public MultiBuffer(long capacity) {
                     + " is smaller than expected chunk size");
         }
 
-        this.buffers = buffers.clone();
-        long capacity = 0;
-        for (ByteBuffer buffer : buffers) {
-            capacity += buffer.capacity();
+        // Make all buffers read-only
+        this.buffers = new ByteBuffer[buffers.length];
+        for (int i = 0; i < buffers.length; i++) {
+            this.buffers[i] = buffers[i].asReadOnlyBuffer();
         }
-        this.capacity = capacity;
-        this.limit = capacity;
-        this.chunkSize = chunkSize;
-    }
 
-    /**
-     * Creates a new {@code MultiBuffer} with the given capacity, backed by
-     * heap-allocated {@link ByteBuffer}s with the given chunk size.
-     *
-     * @param capacity the total capacity in bytes
-     * @param chunkSize the size of each buffer chunk
-     */
-    MultiBuffer(long capacity, int chunkSize) {
-        if (capacity <= 0) {
-            throw new IllegalArgumentException("Capacity must be positive");
+        long capacity = 0;
+        for (ByteBuffer buffer : this.buffers) {
+            capacity += buffer.capacity();
         }
         this.capacity = capacity;
         this.limit = capacity;
         this.chunkSize = chunkSize;
-
-        int fullChunks = (int) (capacity / chunkSize);
-        int remainder = (int) (capacity % chunkSize);
-        int totalChunks = fullChunks + (remainder > 0 ? 1 : 0);
-
-        this.buffers = new ByteBuffer[totalChunks];
-
-        for (int i = 0; i < fullChunks; i++) {
-            buffers[i] = ByteBuffer.allocate(chunkSize);
-        }
-        if (remainder > 0) {
-            buffers[totalChunks - 1] = ByteBuffer.allocate(remainder);
-        }
     }
 
     /** {@inheritDoc} */
@@ -240,41 +209,6 @@ public Buffer duplicate() {
         return copy;
     }
 
-    /** {@inheritDoc} */
-    @Override
-    public long readFrom(FileChannel channel) throws IOException {
-        return this.readFrom(channel, DEFAULT_CHUNK_SIZE);
-    }
-
-    /**
-     * Reads data from the given channel into this buffer starting at the
-     * current position.
-     *
-     * @param channel the file channel
-     * @param chunkSize the chunk size to use for positioning reads
-     * @return the number of bytes read
-     * @throws IOException if an I/O error occurs
-     */
-    long readFrom(FileChannel channel, int chunkSize) throws IOException {
-        long totalRead = 0;
-        long pos = position;
-        for (int i = (int) (pos / chunkSize); i < buffers.length; i++) {
-            ByteBuffer buf = buffers[i];
-            buf.position((int) (pos % chunkSize));
-            int read = channel.read(buf);
-            if (read == -1) {
-                break;
-            }
-            totalRead += read;
-            pos += read;
-            if (pos >= limit) {
-                break;
-            }
-        }
-        position = pos;
-        return totalRead;
-    }
-
     /** {@inheritDoc} */
     @Override
     public String decode(CharsetDecoder decoder)

src/main/java/com/maxmind/db/SingleBuffer.java

@@ -12,34 +12,22 @@
  *
  * <p>This implementation is limited to capacities up to
  * {@link Integer#MAX_VALUE}, as {@link ByteBuffer} cannot exceed that size.
+ *
+ * <p>The underlying {@link ByteBuffer} is read-only to prevent accidental
+ * modification of shared data.
  */
 class SingleBuffer implements Buffer {
 
     private final ByteBuffer buffer;
 
-    /**
-     * Creates a new {@code SingleBuffer} with the given capacity.
-     *
-     * @param capacity the capacity in bytes (must be <= Integer.MAX_VALUE)
-     * @throws IllegalArgumentException if the capacity exceeds
-     *                                  {@link Integer#MAX_VALUE}
-     */
-    public SingleBuffer(long capacity) {
-        if (capacity > Integer.MAX_VALUE) {
-            throw new IllegalArgumentException(
-                "SingleBuffer cannot exceed Integer.MAX_VALUE capacity"
-            );
-        }
-        this.buffer = ByteBuffer.allocate((int) capacity);
-    }
-
     /**
      * Creates a new {@code SingleBuffer} wrapping the given {@link ByteBuffer}.
+     * The buffer is made read-only.
      *
      * @param buffer the underlying buffer
      */
-    private SingleBuffer(ByteBuffer buffer) {
-        this.buffer = buffer;
+    SingleBuffer(ByteBuffer buffer) {
+        this.buffer = buffer.asReadOnlyBuffer();
     }
 
     /** {@inheritDoc} */
@@ -111,12 +99,6 @@ public SingleBuffer duplicate() {
         return new SingleBuffer(this.buffer.duplicate());
     }
 
-    /** {@inheritDoc} */
-    @Override
-    public long readFrom(FileChannel channel) throws IOException {
-        return channel.read(buffer);
-    }
-
     /** {@inheritDoc} */
     @Override
     public String decode(CharsetDecoder decoder)
@@ -145,6 +127,6 @@ public static SingleBuffer wrap(byte[] array) {
     public static SingleBuffer mapFromChannel(FileChannel channel)
         throws IOException {
         ByteBuffer buffer = channel.map(MapMode.READ_ONLY, 0, channel.size());
-        return new SingleBuffer(buffer.asReadOnlyBuffer());
+        return new SingleBuffer(buffer);
     }
 }