Fix integer addition overflow.

An integer overflow can occur when extending the index, preventing guard statements from catching indices outside the expected buffer range and resulting in OutOfMemory exceptions from array allocation.
drewnoakes · Mar 31, 2022 · 0440e26 · 0440e26
1 parent e9733f0
commit 0440e26
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 11 deletions.
diff --git a/Source/com/drew/lang/SequentialByteArrayReader.java b/Source/com/drew/lang/SequentialByteArrayReader.java
@@ -70,7 +70,7 @@ public byte getByte() throws IOException
     @Override
     public byte[] getBytes(int count) throws IOException
     {
-        if (_index + count > _bytes.length) {
+        if ((long)_index + count > _bytes.length) {
             throw new EOFException("End of data reached.");
         }
 
@@ -84,7 +84,7 @@ public byte[] getBytes(int count) throws IOException
     @Override
     public void getBytes(@NotNull byte[] buffer, int offset, int count) throws IOException
     {
-        if (_index + count > _bytes.length) {
+        if ((long)_index + count > _bytes.length) {
             throw new EOFException("End of data reached.");
         }
 
@@ -113,13 +113,13 @@ public boolean trySkip(long n) throws IOException
             throw new IllegalArgumentException("n must be zero or greater.");
         }
 
-        _index += n;
-
-        if (_index > _bytes.length) {
+        if (_index + n > _bytes.length)  {
             _index = _bytes.length;
             return false;
         }
 
+        _index += n;
+
         return true;
     }
 

diff --git a/Source/com/drew/lang/StreamReader.java b/Source/com/drew/lang/StreamReader.java
@@ -68,9 +68,14 @@ public byte getByte() throws IOException
     @Override
     public byte[] getBytes(int count) throws IOException
     {
-        byte[] bytes = new byte[count];
-        getBytes(bytes, 0, count);
-        return bytes;
+        try {
+            byte[] bytes = new byte[count];
+            getBytes(bytes, 0, count);
+            return bytes;
+        } catch (OutOfMemoryError e) {
+            throw new EOFException("End of data reached.");
+        }
+
     }
 
     @Override

diff --git a/Tests/com/drew/lang/SequentialAccessTestBase.java b/Tests/com/drew/lang/SequentialAccessTestBase.java
@@ -40,7 +40,7 @@ public abstract class SequentialAccessTestBase
     @Test
     public void testDefaultEndianness()
     {
-        assertEquals(true, createReader(new byte[1]).isMotorolaByteOrder());
+        assertTrue(createReader(new byte[1]).isMotorolaByteOrder());
     }
 
     @Test
@@ -269,13 +269,20 @@ public void testGetBytes() throws IOException
     @Test
     public void testOverflowBoundsCalculation()
     {
-        SequentialReader reader = createReader(new byte[10]);
-
         try {
+            SequentialReader reader = createReader(new byte[10]);
             reader.getBytes(15);
         } catch (IOException e) {
             assertEquals("End of data reached.", e.getMessage());
         }
+
+        try {
+            SequentialReader reader = createReader(new byte[10]);
+            reader.getBytes(5);
+            reader.getBytes(Integer.MAX_VALUE);
+        } catch (IOException e) {
+            assertEquals("End of data reached.", e.getMessage());
+        }
     }
 
     @Test
@@ -325,6 +332,13 @@ public void testSkipEOF() throws Exception
             reader.skip(1);
             fail("Expecting exception");
         } catch (EOFException ignored) {}
+
+        try {
+            reader = createReader(new byte[100]);
+            reader.skip(50);
+            reader.skip(Integer.MAX_VALUE);
+            fail("Expecting exception");
+        } catch (EOFException ignored) {}
     }
 
     @Test
@@ -336,5 +350,9 @@ public void testTrySkipEOF() throws Exception
         assertTrue(reader.trySkip(1));
         assertTrue(reader.trySkip(1));
         assertFalse(reader.trySkip(1));
+
+        reader = createReader(new byte[100]);
+        reader.getBytes(50);
+        assertFalse(reader.trySkip(Integer.MAX_VALUE));
     }
 }