From 57c6787d0da442984d441b32343f6ecb7e2325c6 Mon Sep 17 00:00:00 2001 From: Tyler Gregg Date: Thu, 21 Dec 2023 20:22:13 -0800 Subject: [PATCH] Handles the case where the reader unexpectedly encounters an oversized value, throwing IonException with a helpful message. --- .../com/amazon/ion/BufferConfiguration.java | 7 +- .../amazon/ion/IonBufferConfiguration.java | 30 +++- .../com/amazon/ion/impl/IonCursorBinary.java | 36 +++-- ...IonReaderContinuableApplicationBinary.java | 1 + .../amazon/ion/system/IonReaderBuilder.java | 5 +- ...onReaderContinuableTopLevelBinaryTest.java | 142 ++++++++++++++++++ .../ion/system/IonReaderBuilderTest.java | 13 +- 7 files changed, 213 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/amazon/ion/BufferConfiguration.java b/src/main/java/com/amazon/ion/BufferConfiguration.java index d86da2c7a5..e31c657d12 100644 --- a/src/main/java/com/amazon/ion/BufferConfiguration.java +++ b/src/main/java/com/amazon/ion/BufferConfiguration.java @@ -161,6 +161,11 @@ public int getMaximumBufferSize() { */ public abstract OversizedValueHandler getNoOpOversizedValueHandler(); + /** + * @return an {@link OversizedValueHandler} that always throws a runtime exception. + */ + public abstract OversizedValueHandler getThrowingOversizedValueHandler(); + /** * @return the no-op {@link DataHandler} for the type of BufferConfiguration that this Builder builds. */ @@ -210,7 +215,7 @@ protected BufferConfiguration(Builder builder) { } if (builder.getOversizedValueHandler() == null) { requireUnlimitedBufferSize(); - oversizedValueHandler = builder.getNoOpOversizedValueHandler(); + oversizedValueHandler = builder.getThrowingOversizedValueHandler(); } else { oversizedValueHandler = builder.getOversizedValueHandler(); } diff --git a/src/main/java/com/amazon/ion/IonBufferConfiguration.java b/src/main/java/com/amazon/ion/IonBufferConfiguration.java index c89c3f27b0..48190854b1 100644 --- a/src/main/java/com/amazon/ion/IonBufferConfiguration.java +++ b/src/main/java/com/amazon/ion/IonBufferConfiguration.java @@ -8,6 +8,12 @@ */ public final class IonBufferConfiguration extends BufferConfiguration { + /** + * The standard configuration. This will be used unless the user chooses custom settings. + */ + public static final IonBufferConfiguration DEFAULT = + IonBufferConfiguration.Builder.standard().build(); + /** * Functional interface for handling oversized symbol tables. */ @@ -31,6 +37,16 @@ public static final class Builder extends BufferConfiguration.Builder 0) { fixedBufferSize += alreadyReadLen; } - if (STANDARD_BUFFER_CONFIGURATION.getInitialBufferSize() > fixedBufferSize) { + if (IonBufferConfiguration.DEFAULT.getInitialBufferSize() > fixedBufferSize) { return FIXED_SIZE_CONFIGURATIONS[logBase2(fixedBufferSize)]; } - return STANDARD_BUFFER_CONFIGURATION; + return IonBufferConfiguration.DEFAULT; } /** @@ -416,19 +423,18 @@ private static IonBufferConfiguration getFixedSizeConfigurationFor( int alreadyReadOff, int alreadyReadLen ) { - this.dataHandler = (configuration == null) ? null : configuration.getDataHandler(); - if (configuration == null) { + if (configuration == IonBufferConfiguration.DEFAULT) { + dataHandler = null; if (inputStream instanceof ByteArrayInputStream) { // ByteArrayInputStreams are fixed-size streams. Clamp the reader's internal buffer size at the size of // the stream to avoid wastefully allocating extra space that will never be needed. It is still // preferable for the user to manually specify the buffer size if it's less than the default, as doing // so allows this branch to be skipped. configuration = getFixedSizeConfigurationFor((ByteArrayInputStream) inputStream, alreadyReadLen); - } else { - configuration = STANDARD_BUFFER_CONFIGURATION; } } else { validate(configuration); + dataHandler = getDataHandler(configuration); } peekIndex = 0; checkpoint = 0; diff --git a/src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java b/src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java index 9fe4e334b8..3e5a57fd1d 100644 --- a/src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java +++ b/src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java @@ -3,6 +3,7 @@ package com.amazon.ion.impl; +import com.amazon.ion.IonBufferConfiguration; import com.amazon.ion.IonCatalog; import com.amazon.ion.IonException; import com.amazon.ion.IonReader; diff --git a/src/main/java/com/amazon/ion/system/IonReaderBuilder.java b/src/main/java/com/amazon/ion/system/IonReaderBuilder.java index 17c2fa9142..0c0771749b 100644 --- a/src/main/java/com/amazon/ion/system/IonReaderBuilder.java +++ b/src/main/java/com/amazon/ion/system/IonReaderBuilder.java @@ -44,7 +44,7 @@ public abstract class IonReaderBuilder private IonCatalog catalog = null; private boolean isIncrementalReadingEnabled = false; - private IonBufferConfiguration bufferConfiguration = null; + private IonBufferConfiguration bufferConfiguration = IonBufferConfiguration.DEFAULT; protected IonReaderBuilder() { @@ -249,6 +249,9 @@ public IonReaderBuilder withBufferConfiguration(IonBufferConfiguration configura */ public void setBufferConfiguration(IonBufferConfiguration configuration) { mutationCheck(); + if (configuration == null) { + throw new IllegalArgumentException("Configuration must not be null. To use the default configuration, provide IonBufferConfiguration.DEFAULT."); + } bufferConfiguration = configuration; } diff --git a/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java b/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java index 3d6140bc94..3abc60748a 100644 --- a/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java +++ b/src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java @@ -38,6 +38,7 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.EOFException; +import java.io.FilterInputStream; import java.io.IOException; import java.io.InputStream; import java.math.BigDecimal; @@ -51,6 +52,7 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.Supplier; import java.util.zip.GZIPInputStream; import static com.amazon.ion.BitUtils.bytes; @@ -3604,4 +3606,144 @@ public void earlyStepOutNonIncremental(boolean constructFromBytes) throws Except // However, the reader *must* fail if the user requests the next value, because the stream is incomplete. assertThrows(IonException.class, () -> reader.next()); // Unexpected EOF } + + /** + * Verifies that the reader throws IonException when the provided code is executed over the given input. + * @param constructFromBytes whether to provide bytes (true) or an InputStream (false) to the reader. + * @param codeExpectedToThrow the code expected to throw IonException. + * @param input the input data. + */ + private void expectIonException( + boolean constructFromBytes, + Consumer codeExpectedToThrow, + int... input + ) throws Exception { + readerBuilder = readerBuilder.withIncrementalReadingEnabled(false).withBufferConfiguration(IonBufferConfiguration.DEFAULT); + reader = readerFor(constructFromBytes, input); + assertThrows(IonException.class, () -> codeExpectedToThrow.accept(reader)); + reader.close(); + } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void scalarValueLargerThan2GB(boolean constructFromBytes) throws Exception { + expectIonException( + constructFromBytes, + reader -> { + reader.next(); + reader.longValue(); + }, + 0x2E, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFF // Int with length larger than 2GB + ); + } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void scalarValueLargerThan2GBWithinSymbolTable(boolean constructFromBytes) throws Exception { + expectIonException( + constructFromBytes, + IonReader::next, + 0xEB, 0x81, 0x83, // Annotation wrapper length 11 with one annotation: 3 ($ion_symbol_table) + 0xD8, // Struct length 8 + 0x87, 0x2E, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFF // Field name 7 (symbols), int with length larger than 2GB + ); + } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void possibleSymbolTableStructDeclaresLengthLargerThan2GB(boolean constructFromBytes) throws Exception { + expectIonException( + constructFromBytes, + IonReader::next, + 0xEB, 0x81, 0x83, // Annotation wrapper length 11 with one annotation: 3 ($ion_symbol_table) + 0xDE, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFF, // Struct with length larger than 2GB + 0x87, 0x20 // Field name 7 (symbols), int 0 + ); + } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void possibleSymbolTableAnnotationDeclaresLengthLargerThan2GB(boolean constructFromBytes) throws Exception { + expectIonException( + constructFromBytes, + IonReader::next, + 0xEE, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0xFF, // Annotation wrapper with length larger than 2GB + 0x81, 0x83, // One annotation: 3 ($ion_symbol_table) + 0xDE, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7C, // Struct with length larger than 2GB + 0x87, 0x20 // Field name 7 (symbols), int 0 + ); + } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void annotationSequenceLengthThatOverflowsBufferThrowsIonException(boolean constructFromBytes) throws Exception { + // This emulates a bad byte (0x52) replacing the annotation sequence length VarUInt in a symbol table annotation + // wrapper. This results in a declared annotation sequence length much larger than the total length of the + // stream. This should be detected and raised as an IonException. + expectIonException( + constructFromBytes, + IonReader::next, + 0xEE, 0x9A, 0x52, 0x83, 0xDE, 0x96, 0x87 + ); + } + + /** + * Verifies that corrupting each byte in the input data results in IonException, or nothing. + * @param constructFromBytes whether to provide bytes (true) or an InputStream (false) to the reader. + * @param incrementalReadingEnabled whether to enable incremental reading. + * @param emptyBufferConfiguration whether to set the buffer configuration to null (true), or use the standard test configuration (false). + */ + private void corruptEachByteThrowsIonException( + boolean constructFromBytes, + boolean incrementalReadingEnabled, + boolean emptyBufferConfiguration + ) { + IonReaderBuilder builder = readerBuilder.copy(); + builder = builder.withIncrementalReadingEnabled(incrementalReadingEnabled); + if (emptyBufferConfiguration) { + builder = builder.withBufferConfiguration(IonBufferConfiguration.DEFAULT); + } + // The following data begins with a symbol table, contains nested containers with length subfields, and + // contains a string, an integer, and blobs. + byte[] original = toBinary( + "{\n" + + " C:\"CRDR\",\n" + + " V:3,\n" + + " DR:{\n" + + " P:\"dummyPartitionKey\",\n" + + " D:{{ dGVzdERhdGE= }},\n" + + " S:{{ AeJA }},\n" + + " DC:{{ brzdAsJNBCzI3S63zno7Uw== }},\n" + + " HC:{{ f5C7STBM1f2S4SFkJWgbIizTYBE= }}\n" + + " }\n" + + "}" + ); + byte[] corrupted = new byte[original.length]; + for (int i = 0; i < corrupted.length; i++) { + // Fill with the original data + System.arraycopy(original, 0, corrupted, 0, original.length); + // Corrupt the byte at index i + corrupted[i] = 0x52; + // Ensure the corruption results in IonException or nothing. + reader = readerFor(builder, constructFromBytes, corrupted); + try { + TestUtils.deepRead(reader); + // Successful parsing is possible because the input contains blobs whose content is not inspected for corruption. + reader.close(); + } catch (IonException e) { + // This is expected to be thrown when corruption is detected. + } catch (Exception f) { + fail("Expected IonException to be thrown, but was: ", f); + } + } + } + + @ParameterizedTest(name = "constructFromBytes={0}") + @ValueSource(booleans = {true, false}) + public void corruptEachByteThrowsIonException(boolean constructFromBytes) { + corruptEachByteThrowsIonException(constructFromBytes, true, true); + corruptEachByteThrowsIonException(constructFromBytes, true, false); + corruptEachByteThrowsIonException(constructFromBytes, false, true); + corruptEachByteThrowsIonException(constructFromBytes, false, false); + } } diff --git a/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java b/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java index 36454e3346..01167e8adb 100644 --- a/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java +++ b/src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -160,13 +161,19 @@ public void testBufferConfiguration() IonBufferConfiguration configuration1 = IonBufferConfiguration.Builder.standard().build(); IonBufferConfiguration configuration2 = IonBufferConfiguration.Builder.standard().build(); IonReaderBuilder builder = IonReaderBuilder.standard(); - assertNull(builder.getBufferConfiguration()); + assertSame(IonBufferConfiguration.DEFAULT, builder.getBufferConfiguration()); builder.withBufferConfiguration(configuration1); assertSame(configuration1, builder.getBufferConfiguration()); builder.setBufferConfiguration(configuration2); assertSame(configuration2, builder.getBufferConfiguration()); - builder.withBufferConfiguration(null); - assertNull(builder.getBufferConfiguration()); + builder.withBufferConfiguration(IonBufferConfiguration.DEFAULT); + assertSame(IonBufferConfiguration.DEFAULT, builder.getBufferConfiguration()); + } + + @Test + public void testNullBufferConfigurationThrows() { + IonReaderBuilder builder = IonReaderBuilder.standard(); + assertThrows(IllegalArgumentException.class, () -> builder.withBufferConfiguration(null)); } @Test