[1/2] activemq-artemis git commit: ARTEMIS-1471 Needs Bounds Checking on writes for MappedSequentialFile

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[1/2] activemq-artemis git commit: ARTEMIS-1471 Needs Bounds Checking on writes for MappedSequentialFile

clebertsuconic-2
Repository: activemq-artemis
Updated Branches:
  refs/heads/master f7ec00b84 -> 0d38250ff


ARTEMIS-1471 Needs Bounds Checking on writes for MappedSequentialFile

The MappedSequentialFile relies on the assumption that any writers
won't exceed the maximum capacity of the file, leaving the JVM to crash otherwise.
This commit adds proper bounds checking on write operations (and position changes too)
in order to provide recoverable effects if such scenario should occour.
In addition are provided minor fixes on Mapped and Nio SequentialFile::fill behaviour
to match the original contract.


Project: http://git-wip-us.apache.org/repos/asf/activemq-artemis/repo
Commit: http://git-wip-us.apache.org/repos/asf/activemq-artemis/commit/3950169c
Tree: http://git-wip-us.apache.org/repos/asf/activemq-artemis/tree/3950169c
Diff: http://git-wip-us.apache.org/repos/asf/activemq-artemis/diff/3950169c

Branch: refs/heads/master
Commit: 3950169c21d97538708320ef29a7f5bab279a50f
Parents: f7ec00b
Author: Francesco Nigro <[hidden email]>
Authored: Thu Oct 19 20:17:34 2017 +0200
Committer: Clebert Suconic <[hidden email]>
Committed: Mon Nov 6 13:52:57 2017 -0500

----------------------------------------------------------------------
 .../artemis/core/io/mapped/MappedFile.java      | 144 ++++---------------
 .../core/io/mapped/MappedSequentialFile.java    |   4 +-
 .../artemis/core/io/nio/NIOSequentialFile.java  |   2 +-
 .../MappedSequentialFileFactoryTest.java        |   2 +-
 4 files changed, 30 insertions(+), 122 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/3950169c/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedFile.java
----------------------------------------------------------------------
diff --git a/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedFile.java b/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedFile.java
index 782233d..05f8f25 100644
--- a/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedFile.java
+++ b/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedFile.java
@@ -80,71 +80,20 @@ final class MappedFile implements AutoCloseable {
       this.buffer.force();
    }
 
-   /**
-    * Reads a sequence of bytes from this file into the given buffer.
-    * <p>
-    * <p> Bytes are read starting at this file's specified position.
-    */
-   public int read(int position, ByteBuf dst, int dstStart, int dstLength) throws IOException {
-      final long srcAddress = this.address + position;
-      if (dst.hasMemoryAddress()) {
-         final long dstAddress = dst.memoryAddress() + dstStart;
-         PlatformDependent.copyMemory(srcAddress, dstAddress, dstLength);
-      } else if (dst.hasArray()) {
-         final byte[] dstArray = dst.array();
-         PlatformDependent.copyMemory(srcAddress, dstArray, dstStart, dstLength);
-      } else {
-         throw new IllegalArgumentException("unsupported byte buffer");
-      }
-      position += dstLength;
-      if (position > this.length) {
-         this.length = position;
+   private void checkCapacity(int requiredCapacity) {
+      if (requiredCapacity < 0 || requiredCapacity > buffer.capacity()) {
+         throw new IllegalStateException("requiredCapacity must be >0 and <= " + buffer.capacity());
       }
-      return dstLength;
    }
 
    /**
-    * Reads a sequence of bytes from this file into the given buffer.
-    * <p>
-    * <p> Bytes are read starting at this file's specified position.
+    * It is raw because it doesn't validate capacity through {@link #checkCapacity(int)}.
     */
-   public int read(int position, ByteBuffer dst, int dstStart, int dstLength) throws IOException {
-      final long srcAddress = this.address + position;
-      if (dst.isDirect()) {
-         final long dstAddress = PlatformDependent.directBufferAddress(dst) + dstStart;
-         PlatformDependent.copyMemory(srcAddress, dstAddress, dstLength);
-      } else {
-         final byte[] dstArray = dst.array();
-         PlatformDependent.copyMemory(srcAddress, dstArray, dstStart, dstLength);
-      }
-      position += dstLength;
+   private void rawMovePositionAndLength(int position) {
+      this.position = position;
       if (position > this.length) {
          this.length = position;
       }
-      return dstLength;
-   }
-
-   /**
-    * Reads a sequence of bytes from this file into the given buffer.
-    * <p>
-    * <p> Bytes are read starting at this file's current position, and
-    * then the position is updated with the number of bytes actually read.
-    */
-   public int read(ByteBuf dst, int dstStart, int dstLength) throws IOException {
-      final int remaining = this.length - this.position;
-      final int read = Math.min(remaining, dstLength);
-      final long srcAddress = this.address + position;
-      if (dst.hasMemoryAddress()) {
-         final long dstAddress = dst.memoryAddress() + dstStart;
-         PlatformDependent.copyMemory(srcAddress, dstAddress, read);
-      } else if (dst.hasArray()) {
-         final byte[] dstArray = dst.array();
-         PlatformDependent.copyMemory(srcAddress, dstArray, dstStart, read);
-      } else {
-         throw new IllegalArgumentException("unsupported byte buffer");
-      }
-      position += read;
-      return read;
    }
 
    /**
@@ -156,7 +105,7 @@ final class MappedFile implements AutoCloseable {
    public int read(ByteBuffer dst, int dstStart, int dstLength) throws IOException {
       final int remaining = this.length - this.position;
       final int read = Math.min(remaining, dstLength);
-      final long srcAddress = this.address + position;
+      final long srcAddress = this.address + this.position;
       if (dst.isDirect()) {
          final long dstAddress = PlatformDependent.directBufferAddress(dst) + dstStart;
          PlatformDependent.copyMemory(srcAddress, dstAddress, read);
@@ -164,7 +113,7 @@ final class MappedFile implements AutoCloseable {
          final byte[] dstArray = dst.array();
          PlatformDependent.copyMemory(srcAddress, dstArray, dstStart, read);
       }
-      position += read;
+      this.position += read;
       return read;
    }
 
@@ -175,13 +124,12 @@ final class MappedFile implements AutoCloseable {
     */
    public void write(EncodingSupport encodingSupport) throws IOException {
       final int encodedSize = encodingSupport.getEncodeSize();
+      final int nextPosition = this.position + encodedSize;
+      checkCapacity(nextPosition);
       this.byteBufWrapper.setIndex(this.position, this.position);
       encodingSupport.encode(this.channelBufferWrapper);
-      position += encodedSize;
-      assert (byteBufWrapper.writerIndex() == position);
-      if (position > this.length) {
-         this.length = position;
-      }
+      rawMovePositionAndLength(nextPosition);
+      assert (byteBufWrapper.writerIndex() == this.position);
    }
 
    /**
@@ -190,7 +138,9 @@ final class MappedFile implements AutoCloseable {
     * <p> Bytes are written starting at this file's current position,
     */
    public void write(ByteBuf src, int srcStart, int srcLength) throws IOException {
-      final long destAddress = this.address + position;
+      final int nextPosition = this.position + srcLength;
+      checkCapacity(nextPosition);
+      final long destAddress = this.address + this.position;
       if (src.hasMemoryAddress()) {
          final long srcAddress = src.memoryAddress() + srcStart;
          PlatformDependent.copyMemory(srcAddress, destAddress, srcLength);
@@ -200,10 +150,7 @@ final class MappedFile implements AutoCloseable {
       } else {
          throw new IllegalArgumentException("unsupported byte buffer");
       }
-      position += srcLength;
-      if (position > this.length) {
-         this.length = position;
-      }
+      rawMovePositionAndLength(nextPosition);
    }
 
    /**
@@ -212,7 +159,9 @@ final class MappedFile implements AutoCloseable {
     * <p> Bytes are written starting at this file's current position,
     */
    public void write(ByteBuffer src, int srcStart, int srcLength) throws IOException {
-      final long destAddress = this.address + position;
+      final int nextPosition = this.position + srcLength;
+      checkCapacity(nextPosition);
+      final long destAddress = this.address + this.position;
       if (src.isDirect()) {
          final long srcAddress = PlatformDependent.directBufferAddress(src) + srcStart;
          PlatformDependent.copyMemory(srcAddress, destAddress, srcLength);
@@ -220,52 +169,7 @@ final class MappedFile implements AutoCloseable {
          final byte[] srcArray = src.array();
          PlatformDependent.copyMemory(srcArray, srcStart, destAddress, srcLength);
       }
-      position += srcLength;
-      if (position > this.length) {
-         this.length = position;
-      }
-   }
-
-   /**
-    * Writes a sequence of bytes to this file from the given buffer.
-    * <p>
-    * <p> Bytes are written starting at this file's specified position,
-    */
-   public void write(int position, ByteBuf src, int srcStart, int srcLength) throws IOException {
-      final long destAddress = this.address + position;
-      if (src.hasMemoryAddress()) {
-         final long srcAddress = src.memoryAddress() + srcStart;
-         PlatformDependent.copyMemory(srcAddress, destAddress, srcLength);
-      } else if (src.hasArray()) {
-         final byte[] srcArray = src.array();
-         PlatformDependent.copyMemory(srcArray, srcStart, destAddress, srcLength);
-      } else {
-         throw new IllegalArgumentException("unsupported byte buffer");
-      }
-      position += srcLength;
-      if (position > this.length) {
-         this.length = position;
-      }
-   }
-
-   /**
-    * Writes a sequence of bytes to this file from the given buffer.
-    * <p>
-    * <p> Bytes are written starting at this file's specified position,
-    */
-   public void write(int position, ByteBuffer src, int srcStart, int srcLength) throws IOException {
-      final long destAddress = this.address + position;
-      if (src.isDirect()) {
-         final long srcAddress = PlatformDependent.directBufferAddress(src) + srcStart;
-         PlatformDependent.copyMemory(srcAddress, destAddress, srcLength);
-      } else {
-         final byte[] srcArray = src.array();
-         PlatformDependent.copyMemory(srcArray, srcStart, destAddress, srcLength);
-      }
-      position += srcLength;
-      if (position > this.length) {
-         this.length = position;
-      }
+      rawMovePositionAndLength(nextPosition);
    }
 
    /**
@@ -274,6 +178,7 @@ final class MappedFile implements AutoCloseable {
     * <p> Bytes are written starting at this file's current position,
     */
    public void zeros(int position, final int count) throws IOException {
+      checkCapacity(position + count);
       //zeroes memory in reverse direction in OS_PAGE_SIZE batches
       //to gain sympathy by the page cache LRU policy
       final long start = this.address + position;
@@ -301,7 +206,7 @@ final class MappedFile implements AutoCloseable {
       if (toZeros > 0) {
          PlatformDependent.setMemory(start, toZeros, (byte) 0);
       }
-
+      //do not move this.position: only this.length can be changed
       position += count;
       if (position > this.length) {
          this.length = position;
@@ -309,15 +214,16 @@ final class MappedFile implements AutoCloseable {
    }
 
    public int position() {
-      return position;
+      return this.position;
    }
 
    public void position(int position) {
+      checkCapacity(position);
       this.position = position;
    }
 
    public long length() {
-      return length;
+      return this.length;
    }
 
    @Override

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/3950169c/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedSequentialFile.java
----------------------------------------------------------------------
diff --git a/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedSequentialFile.java b/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedSequentialFile.java
index 7619d1a..aec4723 100644
--- a/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedSequentialFile.java
+++ b/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/mapped/MappedSequentialFile.java
@@ -135,10 +135,12 @@ final class MappedSequentialFile implements SequentialFile {
    public void fill(int size) throws IOException {
       checkIsOpen();
       //the fill will give a big performance hit when done in parallel of other writings!
-      this.mappedFile.zeros(this.mappedFile.position(), size);
+      this.mappedFile.zeros(0, size);
       if (factory.isDatasync()) {
          this.mappedFile.force();
       }
+      //set the position to 0 to match the fill contract
+      this.mappedFile.position(0);
    }
 
    @Override

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/3950169c/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java
----------------------------------------------------------------------
diff --git a/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java b/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java
index d1e333e..891bd5c 100644
--- a/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java
+++ b/artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/nio/NIOSequentialFile.java
@@ -112,7 +112,7 @@ public final class NIOSequentialFile extends AbstractSequentialFile {
             }
             //set the position to 0 to match the fill contract
             channel.position(0);
-            fileSize = size;
+            fileSize = channel.size();
          } finally {
             //return it to the factory
             this.factory.releaseBuffer(zeroPage);

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/3950169c/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/journal/MappedSequentialFileFactoryTest.java
----------------------------------------------------------------------
diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/journal/MappedSequentialFileFactoryTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/journal/MappedSequentialFileFactoryTest.java
index 937435e..4b9d321 100644
--- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/journal/MappedSequentialFileFactoryTest.java
+++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/journal/MappedSequentialFileFactoryTest.java
@@ -123,7 +123,7 @@ public class MappedSequentialFileFactoryTest extends SequentialFileFactoryTestBa
                SequentialFile file = factory.createSequentialFile("file.txt");
                file.open();
                Thread.currentThread().interrupt();
-               file.fill(1024);
+               file.fill(fakeEncoding.getEncodeSize());
                file.close();
 
             } catch (Exception e) {

Reply | Threaded
Open this post in threaded view
|

[2/2] activemq-artemis git commit: This closes #1600

clebertsuconic-2
This closes #1600


Project: http://git-wip-us.apache.org/repos/asf/activemq-artemis/repo
Commit: http://git-wip-us.apache.org/repos/asf/activemq-artemis/commit/0d38250f
Tree: http://git-wip-us.apache.org/repos/asf/activemq-artemis/tree/0d38250f
Diff: http://git-wip-us.apache.org/repos/asf/activemq-artemis/diff/0d38250f

Branch: refs/heads/master
Commit: 0d38250ff2ca4260a9cd81290748d8ecb4af127f
Parents: f7ec00b 3950169
Author: Clebert Suconic <[hidden email]>
Authored: Mon Nov 6 13:52:58 2017 -0500
Committer: Clebert Suconic <[hidden email]>
Committed: Mon Nov 6 13:52:58 2017 -0500

----------------------------------------------------------------------
 .../artemis/core/io/mapped/MappedFile.java      | 144 ++++---------------
 .../core/io/mapped/MappedSequentialFile.java    |   4 +-
 .../artemis/core/io/nio/NIOSequentialFile.java  |   2 +-
 .../MappedSequentialFileFactoryTest.java        |   2 +-
 4 files changed, 30 insertions(+), 122 deletions(-)
----------------------------------------------------------------------