From 3d30a98b23ae6bd17db8c44efaae2955f308d13e Mon Sep 17 00:00:00 2001
From: Francesco Nigro <nigro.fra@gmail.com>
Date: Mon, 16 Apr 2018 13:20:22 +0200
Subject: [PATCH] ARTEMIS-1810 JDBCSequentialFileFactoryDriver should check <=0
 read length

In order to avoid out of bounds reads to happen, the reading of the file
should avoid those readings to hit the DMBS and just return the expected
value.
---
 .../file/JDBCSequentialFileFactoryDriver.java | 22 +++++--
 .../file/JDBCSequentialFileFactoryTest.java   | 60 +++++++++++++++++++
 2 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/file/JDBCSequentialFileFactoryDriver.java b/artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/file/JDBCSequentialFileFactoryDriver.java
index fdd7c764a4..14ad25e825 100644
--- a/artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/file/JDBCSequentialFileFactoryDriver.java
+++ b/artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/file/JDBCSequentialFileFactoryDriver.java
@@ -294,17 +294,27 @@ public class JDBCSequentialFileFactoryDriver extends AbstractJDBCDriver {
             if (rs.next()) {
                final Blob blob = rs.getBlob(1);
                if (blob != null) {
-                  readLength = (int) calculateReadLength(blob.length(), bytes.remaining(), file.position());
-                  byte[] data = blob.getBytes(file.position() + 1, readLength);
-                  bytes.put(data);
+                  final long blobLength = blob.length();
+                  final int bytesRemaining = bytes.remaining();
+                  final long filePosition = file.position();
+                  readLength = (int) calculateReadLength(blobLength, bytesRemaining, filePosition);
+                  if (logger.isDebugEnabled()) {
+                     logger.debugf("trying read %d bytes: blobLength = %d bytesRemaining = %d filePosition = %d",
+                                   readLength, blobLength, bytesRemaining, filePosition);
+                  }
+                  if (readLength < 0) {
+                     readLength = -1;
+                  } else if (readLength > 0) {
+                     byte[] data = blob.getBytes(file.position() + 1, readLength);
+                     bytes.put(data);
+                  }
                }
             }
             connection.commit();
             return readLength;
-         } catch (Throwable e) {
-            throw e;
-         } finally {
+         } catch (SQLException e) {
             connection.rollback();
+            throw e;
          }
       }
    }
diff --git a/artemis-jdbc-store/src/test/java/org/apache/activemq/artemis/jdbc/file/JDBCSequentialFileFactoryTest.java b/artemis-jdbc-store/src/test/java/org/apache/activemq/artemis/jdbc/file/JDBCSequentialFileFactoryTest.java
index d763709df0..d567f84eb9 100644
--- a/artemis-jdbc-store/src/test/java/org/apache/activemq/artemis/jdbc/file/JDBCSequentialFileFactoryTest.java
+++ b/artemis-jdbc-store/src/test/java/org/apache/activemq/artemis/jdbc/file/JDBCSequentialFileFactoryTest.java
@@ -96,6 +96,66 @@ public class JDBCSequentialFileFactoryTest {
       assertTrue(factory.isStarted());
    }
 
+   @Test
+   public void testReadZeroBytesOnEmptyFile() throws Exception {
+      JDBCSequentialFile file = (JDBCSequentialFile) factory.createSequentialFile("test.txt");
+      file.open();
+      try {
+         final ByteBuffer readBuffer = ByteBuffer.allocate(0);
+         final int bytes = file.read(readBuffer);
+         assertEquals(0, bytes);
+      } finally {
+         file.close();
+      }
+   }
+
+   @Test
+   public void testReadZeroBytesOnNotEmptyFile() throws Exception {
+      final int fileLength = 8;
+      JDBCSequentialFile file = (JDBCSequentialFile) factory.createSequentialFile("test.txt");
+      file.open();
+      try {
+         file.writeDirect(ByteBuffer.allocate(fileLength), true);
+         assertEquals(fileLength, file.size());
+         final ByteBuffer readBuffer = ByteBuffer.allocate(0);
+         final int bytes = file.read(readBuffer);
+         assertEquals(0, bytes);
+      } finally {
+         file.close();
+      }
+   }
+
+   @Test
+   public void testReadOutOfBoundsOnEmptyFile() throws Exception {
+      JDBCSequentialFile file = (JDBCSequentialFile) factory.createSequentialFile("test.txt");
+      file.open();
+      try {
+         final ByteBuffer readBuffer = ByteBuffer.allocate(1);
+         file.position(1);
+         final int bytes = file.read(readBuffer);
+         assertTrue("bytes read should be < 0", bytes < 0);
+      } finally {
+         file.close();
+      }
+   }
+
+   @Test
+   public void testReadOutOfBoundsOnNotEmptyFile() throws Exception {
+      final int fileLength = 8;
+      JDBCSequentialFile file = (JDBCSequentialFile) factory.createSequentialFile("test.txt");
+      file.open();
+      try {
+         file.writeDirect(ByteBuffer.allocate(fileLength), true);
+         assertEquals(fileLength, file.size());
+         file.position(fileLength + 1);
+         final ByteBuffer readBuffer = ByteBuffer.allocate(fileLength);
+         final int bytes = file.read(readBuffer);
+         assertTrue("bytes read should be < 0", bytes < 0);
+      } finally {
+         file.close();
+      }
+   }
+
    @Test
    public void testCreateFiles() throws Exception {
       int noFiles = 100;