Repository: amzn/ion-java
Pull request #261 (2019-09-25 00:20:31)

Fixes a bug in IonReaderBinaryRawX that caused the reader to prematurely finish reading large streams in certain cases.
*Description of changes:*

The binary reader uses `Integer.MIN_VALUE` to mean [`NO_LIMIT`](https://github.com/amzn/ion-java/blob/master/src/com/amazon/ion/impl/IonReaderBinaryRawX.java#L54) for its [`_local_remaining`](https://github.com/amzn/ion-java/blob/master/src/com/amazon/ion/impl/IonReaderBinaryRawX.java#L65) value, which keeps track of the remaining number of bytes in the current value. Between values at the top level, this should always be set to `NO_LIMIT`. No arithmetic should ever be performed on the `_local_remaining` when it is set to `NO_LIMIT`.

This PR fixes a bug that violated this by performing an unconditional subtraction of the current value's length from `_local_remaining` within `stringValue()`, `decimalValue()`, and `timestampValue()`. This caused `_local_remaining` to overflow to a very large value immediately. For every top level value subsequently read, the length of that value would be subtracted from `_local_remaining` until eventually `_local_remaining` prematurely reached 0 around the time the stream reached `Integer.MAX_VALUE` in length. This caused the reader to erroneously determine that there were fewer bytes remaining than were needed to complete the current value. The reader would then finish early before reading the entire stream.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


codecov[bot]: # [Codecov](https://codecov.io/gh/amzn/ion-java/pull/261?src=pr&el=h1) Report
> Merging [#261](https://codecov.io/gh/amzn/ion-java/pull/261?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-java/commit/1c6f20e18f79004b6a1b8bc003202af32de6c27f?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-java/pull/261/graphs/tree.svg?width=650&token=M93Y3e3kKn&height=150&src=pr)](https://codecov.io/gh/amzn/ion-java/pull/261?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master     #261      +/-   ##
============================================
+ Coverage     63.85%   63.86%   +0.01%     
- Complexity     4798     4802       +4     
============================================
  Files           136      136              
  Lines         20998    21004       +6     
  Branches       3821     3824       +3     
============================================
+ Hits          13408    13415       +7     
  Misses         6246     6246              
+ Partials       1344     1343       -1
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-java/pull/261?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [src/com/amazon/ion/impl/IonReaderBinaryRawX.java](https://codecov.io/gh/amzn/ion-java/pull/261/diff?src=pr&el=tree#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvSW9uUmVhZGVyQmluYXJ5UmF3WC5qYXZh) | `77.51% <100%> (+0.21%)` | `167 <0> (+3)` | :arrow_up: |
| [src/com/amazon/ion/impl/BlockedBuffer.java](https://codecov.io/gh/amzn/ion-java/pull/261/diff?src=pr&el=tree#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvQmxvY2tlZEJ1ZmZlci5qYXZh) | `51.45% <0%> (+0.12%)` | `80% <0%> (+1%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-java/pull/261?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-java/pull/261?src=pr&el=footer). Last update [1c6f20e...ba5a9ac](https://codecov.io/gh/amzn/ion-java/pull/261?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/com/amazon/ion/impl/IonReaderBinaryRawX.java b/src/com/amazon/ion/impl/IonReaderBinaryRawX.java
index 1ef4ffde..c5ece958 100644
--- a/src/com/amazon/ion/impl/IonReaderBinaryRawX.java
+++ b/src/com/amazon/ion/impl/IonReaderBinaryRawX.java
@@ -1064,7 +1064,10 @@ abstract class IonReaderBinaryRawX
         }
         else {
             // otherwise we to it the hard way ....
-            int  save_limit = _local_remaining - len;
+            int save_limit = NO_LIMIT;
+            if (_local_remaining != NO_LIMIT) {
+                save_limit = _local_remaining - len;
+            }
             _local_remaining = len;
             int  exponent = readVarInt();
             BigInteger value;
@@ -1114,7 +1117,10 @@ abstract class IonReaderBinaryRawX
 
         int         year = 0, month = 0, day = 0, hour = 0, minute = 0, second = 0;
         BigDecimal  frac = null;
-        int         save_limit = _local_remaining - len;
+        int         save_limit = NO_LIMIT;
+        if (_local_remaining != NO_LIMIT) {
+            save_limit = _local_remaining - len;
+        }
         _local_remaining = len;  // > 0
 
         // first up is the offset, which requires a special int reader
@@ -1182,7 +1188,10 @@ abstract class IonReaderBinaryRawX
         // the char array is way faster than using string buffer
         char[] chars = new char[len];
         int    c, ii = 0;
-        int    save_limit = _local_remaining - len;
+        int    save_limit = NO_LIMIT;
+        if (_local_remaining != NO_LIMIT) {
+            save_limit = _local_remaining - len;
+        }
         _local_remaining = len;
         while (!isEOF()) {
             c = readUnicodeScalar();


Test patch:
diff --git a/test/AllTests.java b/test/AllTests.java
index 326611b6..0e433b5b 100644
--- a/test/AllTests.java
+++ b/test/AllTests.java
@@ -48,6 +48,7 @@ import com.amazon.ion.NonEquivsTest;
 import com.amazon.ion.NopPaddingTest;
 import com.amazon.ion.NullTest;
 import com.amazon.ion.RawValueSpanReaderBasicTest;
+import com.amazon.ion.impl.IonReaderBinaryRawLargeStreamTest;
 import com.amazon.ion.impl.RawValueSpanReaderTest;
 import com.amazon.ion.RoundTripTest;
 import com.amazon.ion.SexpTest;
@@ -206,6 +207,7 @@ import org.junit.runners.Suite;
     IonReaderToIonValueTest.class,
     BinaryReaderWrappedValueLengthTest.class,
     IonReaderBuilderTest.class,
+    IonReaderBinaryRawLargeStreamTest.class,
 
     // experimental binary writer tests
     PooledBlockAllocatorProviderTest.class,
diff --git a/test/com/amazon/ion/impl/IonReaderBinaryRawLargeStreamTest.java b/test/com/amazon/ion/impl/IonReaderBinaryRawLargeStreamTest.java
new file mode 100644
index 00000000..9ed96510
--- /dev/null
+++ b/test/com/amazon/ion/impl/IonReaderBinaryRawLargeStreamTest.java
@@ -0,0 +1,73 @@
+package com.amazon.ion.impl;
+
+import com.amazon.ion.IonReader;
+import com.amazon.ion.IonType;
+import com.amazon.ion.IonWriter;
+import com.amazon.ion.Timestamp;
+import com.amazon.ion.system.IonBinaryWriterBuilder;
+import com.amazon.ion.system.IonReaderBuilder;
+import com.amazon.ion.util.RepeatInputStream;
+import org.junit.Test;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.InputStream;
+import java.io.SequenceInputStream;
+import java.math.BigDecimal;
+
+import static com.amazon.ion.impl._Private_IonConstants.BINARY_VERSION_MARKER_1_0;
+import static org.junit.Assert.assertEquals;
+
+public class IonReaderBinaryRawLargeStreamTest {
+
+    // NOTE: this test takes several seconds to complete.
+    @Test
+    public void testReadLargeScalarStream() throws Exception {
+        ByteArrayOutputStream out = new ByteArrayOutputStream();
+        IonWriter writer = IonBinaryWriterBuilder.standard().build(out);
+        final Timestamp timestamp = Timestamp.forDay(2000, 1, 1);
+        writer.writeString("foo");
+        writer.writeDecimal(BigDecimal.TEN);
+        writer.writeTimestamp(timestamp);
+        writer.close();
+        byte[] dataWithIvm = out.toByteArray();
+        // Strip the IVM, as this needs to be one continuous stream to avoid resetting the reader's internals.
+        byte[] data = new byte[dataWithIvm.length - BINARY_VERSION_MARKER_1_0.length];
+        System.arraycopy(dataWithIvm, BINARY_VERSION_MARKER_1_0.length, data, 0, data.length);
+        // The binary reader uses Integer.MIN_VALUE to mean NO_LIMIT for its _local_remaining value, which keeps track
+        // of the remaining number of bytes in the current value. Between values at the top level, this should always be
+        // NO_LIMIT. No arithmetic should ever be performed on the value when it is set to NO_LIMIT. If bugs exist that
+        // violate this, then between top level values _local_remaining will never again be NO_LIMIT, meaning that
+        // arithmetic will continue to be performed on it. Eventually, due to integer overflow, the value will roll over
+        // into a small enough positive value that the reader will erroneously determine that there are fewer bytes
+        // remaining than are needed to complete the current value. The reader will then finish early before reading the
+        // entire stream. The bug that prompted this test to be written involved an unconditional subtraction of the
+        // current value's length as declared in its header from the current value of _local_remaining within
+        // stringValue(), decimalValue(), and timestampValue(). This caused _local_remaining to overflow to a very
+        // large value immediately. For every top level value subsequently read, the length of that value would be
+        // subtracted from _local_remaining until eventually _local_remaining prematurely reached 0 around the time
+        // the stream reached Integer.MAX_VALUE in length.
+        // Repeat the batch a sufficient number of times to exceed a total stream length of Integer.MAX_VALUE, plus
+        // a few more to make sure batches continue to be read correctly.
+        final int totalNumberOfBatches = (Integer.MAX_VALUE / data.length) + 7;
+        InputStream inputStream = new SequenceInputStream(
+            new ByteArrayInputStream(BINARY_VERSION_MARKER_1_0),
+            new RepeatInputStream(data, totalNumberOfBatches - 1) // This will provide the data 'totalNumberOfBatches' times
+        );
+        IonReader reader = IonReaderBuilder.standard().build(inputStream);
+        reader.next();
+        assertEquals("foo", reader.stringValue());
+        reader.next();
+        assertEquals(BigDecimal.TEN, reader.decimalValue());
+        reader.next();
+        assertEquals(timestamp, reader.timestampValue());
+        int batchesRead = 1;
+        while (reader.next() != null) {
+            assertEquals(IonType.STRING, reader.getType());
+            assertEquals(IonType.DECIMAL, reader.next());
+            assertEquals(IonType.TIMESTAMP, reader.next());
+            batchesRead++;
+        }
+        assertEquals(totalNumberOfBatches, batchesRead);
+    }
+}
diff --git a/test/com/amazon/ion/util/RepeatInputStream.java b/test/com/amazon/ion/util/RepeatInputStream.java
index f5bae1c2..c9e254af 100644
--- a/test/com/amazon/ion/util/RepeatInputStream.java
+++ b/test/com/amazon/ion/util/RepeatInputStream.java
@@ -33,7 +33,9 @@ public final class RepeatInputStream
 
     /**
      * @param bytes the data to be repeated.
-     * @param times the number of times to repeat the data.
+     * @param times the number of times to repeat the data. NOTE: this is the number of times the bytes will be
+     *              repeated after the first time they are read. For example, providing a byte array with a single
+     *              byte 'a' and times=1 will allow 'a' to be read twice before the stream signals EOF.
      */
     public RepeatInputStream(byte[] bytes, long times)
     {