diff --git a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java index e38eb9b687..813d8e0100 100644 --- a/src/main/java/com/amazon/ion/impl/IonCursorBinary.java +++ b/src/main/java/com/amazon/ion/impl/IonCursorBinary.java @@ -733,12 +733,9 @@ private boolean slowSeek(long numberOfBytes) { private long uncheckedReadVarUInt_1_0(byte currentByte) { long result = currentByte & LOWER_SEVEN_BITS_BITMASK; do { - // Note: if the varUInt is malformed such that it extends beyond the declared length of the value *and* - // beyond the end of the buffer, this will result in IndexOutOfBoundsException because only the declared - // value length has been filled. Preventing this is simple: if (peekIndex >= limit) throw - // new IonException(); However, we choose not to perform that check here because it is not worth sacrificing - // performance in this inner-loop code in order to throw one type of exception over another in case of - // malformed data. + if (peekIndex >= limit) { + throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); + } currentByte = buffer[(int) (peekIndex++)]; result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK); } while (currentByte >= 0); @@ -778,6 +775,9 @@ private long slowReadVarUInt_1_0() { private boolean uncheckedReadAnnotationWrapperHeader_1_0(IonTypeID valueTid) { long endIndex; if (valueTid.variableLength) { + if (peekIndex >= limit) { + throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); + } byte b = buffer[(int) peekIndex++]; if (b < 0) { endIndex = (b & LOWER_SEVEN_BITS_BITMASK); @@ -790,7 +790,7 @@ private boolean uncheckedReadAnnotationWrapperHeader_1_0(IonTypeID valueTid) { endIndex += peekIndex; setMarker(endIndex, valueMarker); if (endIndex > limit) { - return true; + throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); } byte b = buffer[(int) peekIndex++]; int annotationsLength; @@ -886,6 +886,9 @@ private void prohibitEmptyOrderedStruct_1_0(Marker marker) { private long calculateEndIndex_1_0(IonTypeID valueTid, boolean isAnnotated) { long endIndex; if (valueTid.variableLength) { + if (peekIndex >= limit) { + throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); + } byte b = buffer[(int) peekIndex++]; if (b < 0) { endIndex = (b & LOWER_SEVEN_BITS_BITMASK); diff --git a/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java b/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java index 71c4f9447e..39813ffd10 100644 --- a/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java +++ b/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java @@ -157,34 +157,15 @@ int readVarUInt_1_0() { int currentByte = 0; int result = 0; while ((currentByte & HIGHEST_BIT_BITMASK) == 0) { + if (peekIndex >= limit) { + throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); + } currentByte = buffer[(int)(peekIndex++)]; result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK); } return result; } - /** - * Reads a 2+ byte VarUInt, given the first byte. When called, `peekIndex` must point at the second byte in the - * VarUInt. When this method returns, `peekIndex` will point at the first byte that follows the VarUInt. - * NOTE: the VarUInt must fit in an `int`. - * @param currentByte the first byte in the VarUInt. - * @return the value. - */ - int readVarUInt_1_0(byte currentByte) { - int result = currentByte & LOWER_SEVEN_BITS_BITMASK; - do { - // Note: if the varUInt is malformed such that it extends beyond the declared length of the value *and* - // beyond the end of the buffer, this will result in IndexOutOfBoundsException because only the declared - // value length has been filled. Preventing this is simple: if (peekIndex >= valueMarker.endIndex) throw - // new IonException(); However, we choose not to perform that check here because it is not worth sacrificing - // performance in this inner-loop code in order to throw one type of exception over another in case of - // malformed data. - currentByte = buffer[(int) (peekIndex++)]; - result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK); - } while (currentByte >= 0); - return result; - } - /** * Reads a 2+ byte VarInt, given the first byte. When called, `peekIndex` must point at the second byte in the * VarInt. at `peekIndex`.When this method returns, `peekIndex` will point at the first byte that follows the @@ -197,12 +178,9 @@ private int readVarInt_1_0(int firstByte) { int sign = (currentByte & VAR_INT_SIGN_BITMASK) == 0 ? 1 : -1; int result = currentByte & LOWER_SIX_BITS_BITMASK; while ((currentByte & HIGHEST_BIT_BITMASK) == 0) { - // Note: if the varInt is malformed such that it extends beyond the declared length of the value *and* - // beyond the end of the buffer, this will result in IndexOutOfBoundsException because only the declared - // value length has been filled. Preventing this is simple: if (peekIndex >= valueMarker.endIndex) throw - // new IonException(); However, we choose not to perform that check here because it is not worth sacrificing - // performance in this inner-loop code in order to throw one type of exception over another in case of - // malformed data. + if (peekIndex >= limit) { + throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); + } currentByte = buffer[(int)(peekIndex++)]; result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK); } @@ -518,11 +496,13 @@ public boolean isNullValue() { } /** - * Performs any logic necessary to prepare a scalar value for parsing. In general nothing needs to be done, but - * subclasses may wish to provide different behavior, such as ensuring that the value is present in the buffer. + * Performs any logic necessary to prepare a scalar value for parsing. Subclasses may wish to provide additional + * logic, such as ensuring that the value is present in the buffer. */ void prepareScalar() { - // May be overridden. + if (valueMarker.endIndex > limit) { + throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); + } } @Override diff --git a/src/main/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinary.java b/src/main/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinary.java index 25a9383f8e..e46118bc0f 100644 --- a/src/main/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinary.java +++ b/src/main/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinary.java @@ -208,11 +208,12 @@ public IonType getType() { void prepareScalar() { if (!isValueIncomplete) { if (!isSlowMode || event == IonCursor.Event.VALUE_READY) { - // Nothing to do. + super.prepareScalar(); return; } if (isFillRequired) { if (fillValue() == Event.VALUE_READY) { + super.prepareScalar(); return; } if (event == Event.NEEDS_INSTRUCTION) { diff --git a/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java index e0b80fb23b..b28e9bd5f9 100644 --- a/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java +++ b/src/test/java/com/amazon/ion/impl/IonCursorBinaryTest.java @@ -4,6 +4,8 @@ package com.amazon.ion.impl; import com.amazon.ion.IonCursor; +import com.amazon.ion.IonException; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -26,6 +28,7 @@ import static com.amazon.ion.impl.IonCursorTestUtilities.scalar; import static com.amazon.ion.impl.IonCursorTestUtilities.startContainer; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; public class IonCursorBinaryTest { @@ -299,4 +302,99 @@ public void fillContainerThenSkip(boolean constructFromBytes) { endStream() ); } + + @Test + public void expectMalformedListHeaderToFailCleanly() { + // The following test is expected to fail because the VarUInt length would extend beyond the end of the buffer. + // Since a byte array is provided as the input source, no additional input can be provided, so this must be + // treated as an error. + IonCursorBinary cursor = initializeCursor( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0xBE, // List with VarUInt length + 0x01 // Non-terminal VarUInt byte + ); + assertThrows(IonException.class, cursor::nextValue); + cursor.close(); + } + + @Test + public void expectMissingListLengthToFailCleanly() { + // The following test is expected to fail because the VarUInt length is missing. + // Since a byte array is provided as the input source, no additional input can be provided, so this must be + // treated as an error. + IonCursorBinary cursor = initializeCursor( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0xBE // List with VarUInt length, no length follows + ); + assertThrows(IonException.class, cursor::nextValue); + cursor.close(); + } + + @Test + public void expectMalformedStructFieldNameToFailCleanly() { + IonCursorBinary cursor = initializeCursor( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0xD2, // Struct length 2 + 0x01, // Non-terminal VarUInt byte + 0x01, // Non-terminal VarUInt byte + 0x01 // Non-terminal VarUInt byte + ); + cursor.nextValue(); + cursor.stepIntoContainer(); + assertThrows(IonException.class, cursor::nextValue); + cursor.close(); + } + + @Test + public void expectMissingAnnotationWrapperLengthToFailCleanly() { + IonCursorBinary cursor = initializeCursor( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0xEE // Annotation wrapper with VarUInt length, no length follows + ); + assertThrows(IonException.class, cursor::nextValue); + cursor.close(); + } + + @Test + public void expectMalformedAnnotationWrapperHeaderToFailCleanly() { + IonCursorBinary cursor = initializeCursor( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0xEE, // Annotation wrapper with VarUInt length + 0x01 // Non-terminal VarUInt byte + ); + assertThrows(IonException.class, cursor::nextValue); + cursor.close(); + } + + @Test + public void expectMalformedAnnotationWrapperAnnotationLengthToFailCleanly() { + IonCursorBinary cursor = initializeCursor( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0xEE, // Annotation wrapper with VarUInt length + 0x83, // VarUInt length 3 + 0x01, // Non-terminal VarUInt byte + 0x01, // Non-terminal VarUInt byte + 0x01 // Non-terminal VarUInt byte + ); + assertThrows(IonException.class, cursor::nextValue); + cursor.close(); + } + + @Test + public void expectMissingAnnotationWrapperAnnotationLengthToFailCleanly() { + IonCursorBinary cursor = initializeCursor( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0xEE, // Annotation wrapper with VarUInt length + 0x83 // VarUInt length 3, no annotation length follows + ); + assertThrows(IonException.class, cursor::nextValue); + cursor.close(); + } } diff --git a/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java index cbc0c9a74b..3de1ab1da0 100644 --- a/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java +++ b/src/test/java/com/amazon/ion/impl/IonReaderContinuableCoreBinaryTest.java @@ -3,7 +3,9 @@ package com.amazon.ion.impl; +import com.amazon.ion.IonException; import com.amazon.ion.IonType; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -25,6 +27,7 @@ import static com.amazon.ion.impl.IonCursorTestUtilities.startContainer; import static com.amazon.ion.impl.IonCursorTestUtilities.fillStringValue; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; public class IonReaderContinuableCoreBinaryTest { @@ -290,4 +293,102 @@ public void fillContainerThenSkip(boolean constructFromBytes) { endStream() ); } + + @Test + public void expectMalformedDecimalScaleToFailCleanly() { + IonReaderContinuableCoreBinary reader = initializeReader( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0x51, // Decimal length 1 + 0x01 // Non-terminal VarInt byte + ); + reader.nextValue(); + assertThrows(IonException.class, reader::decimalValue); + assertThrows(IonException.class, reader::bigDecimalValue); + reader.close(); + } + + @Test + public void expectMissingDecimalScaleToFailCleanly() { + IonReaderContinuableCoreBinary reader = initializeReader( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0x51 // Decimal length 1, missing scale + ); + reader.nextValue(); + assertThrows(IonException.class, reader::decimalValue); + assertThrows(IonException.class, reader::bigDecimalValue); + reader.close(); + } + + @Test + public void expectMalformedDecimalHeaderToFailCleanly() { + IonReaderContinuableCoreBinary reader = initializeReader( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0x53, // Decimal length 3, only 2 bytes follow + 0xC1, // VarInt -1 + 0x01 // Non-terminal VarInt byte + ); + reader.nextValue(); + assertThrows(IonException.class, reader::decimalValue); + assertThrows(IonException.class, reader::bigDecimalValue); + reader.close(); + } + + @Test + public void expectMalformedIntHeaderToFailCleanly() { + IonReaderContinuableCoreBinary reader = initializeReader( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0x21 // Int length 1 + ); + reader.nextValue(); + assertThrows(IonException.class, reader::intValue); + assertThrows(IonException.class, reader::bigIntegerValue); + assertThrows(IonException.class, reader::getIntegerSize); + reader.close(); + } + + @Test + public void expectMalformedTimestampOffsetToFailCleanly() { + IonReaderContinuableCoreBinary reader = initializeReader( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0x63, // Timestamp length 3 + 0x01, // Non-terminal VarInt byte + 0x01, // Non-terminal VarInt byte + 0x01 // Non-terminal VarInt byte + ); + reader.nextValue(); + assertThrows(IonException.class, reader::timestampValue); + reader.close(); + } + + @Test + public void expectMalformedTimestampYearToFailCleanly() { + IonReaderContinuableCoreBinary reader = initializeReader( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0x63, // Timestamp length 3 + 0xC0, // VarInt -0 (unknown offset) + 0x01, // Non-terminal VarUInt byte + 0x01 // Non-terminal VarUInt byte + ); + reader.nextValue(); + assertThrows(IonException.class, reader::timestampValue); + reader.close(); + } + + @Test + public void expectMissingTimestampBodyToFailCleanly() { + IonReaderContinuableCoreBinary reader = initializeReader( + true, + 0xE0, 0x01, 0x00, 0xEA, + 0x63 // Timestamp length 3, missing body + ); + reader.nextValue(); + assertThrows(IonException.class, reader::timestampValue); + reader.close(); + } } diff --git a/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java b/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java index 01167e8adb..b7d379922e 100644 --- a/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java +++ b/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java @@ -25,6 +25,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import com.amazon.ion.BitUtils; import com.amazon.ion.IonBufferConfiguration; import com.amazon.ion.IonCatalog; import com.amazon.ion.IonException; @@ -42,6 +43,8 @@ import com.amazon.ion.impl._Private_IonConstants; import org.junit.Rule; import org.junit.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.junit.rules.ExpectedException; /** @@ -149,8 +152,10 @@ public void testEnableIncrementalReading() throws IOException // Expected; this is a non-incremental reader, but a complete value was not available. } builder.withIncrementalReadingEnabled(true); + // Even though incremental reading is enabled, when a byte array is provided an error should be raised on the + // incomplete input because additional data can never be provided. IonReader reader2 = builder.build(data.toByteArray()); - assertNull(reader2.next()); + assertThrows(IonException.class, reader2::next); IonReader reader3 = builder.build(new ByteArrayInputStream(data.toByteArray())); assertNull(reader3.next()); } @@ -221,4 +226,12 @@ public void concatenatedAfterGZIPHeader() throws Exception { assertNull(reader.next()); } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void incompleteIvmFailsCleanly(boolean isIncremental) throws Exception { + IonReader reader = IonReaderBuilder.standard().withIncrementalReadingEnabled(isIncremental).build(BitUtils.bytes(0xE0, 0x01, 0x00)); + assertThrows(IonException.class, reader::next); + reader.close(); + } + }