Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handles the case where the reader unexpectedly encounters an oversized value, throwing IonException with a helpful message. #680

Merged
merged 1 commit into from
Dec 26, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Handles the case where the reader unexpectedly encounters an oversize…
…d value, throwing IonException with a helpful message.
tgregg committed Dec 22, 2023
commit aa9a7940657dc6bee35fca49ac67d1678e7a0639
7 changes: 6 additions & 1 deletion src/main/java/com/amazon/ion/BufferConfiguration.java
Original file line number Diff line number Diff line change
@@ -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<Configuration, ?> builder) {
}
if (builder.getOversizedValueHandler() == null) {
requireUnlimitedBufferSize();
oversizedValueHandler = builder.getNoOpOversizedValueHandler();
oversizedValueHandler = builder.getThrowingOversizedValueHandler();
} else {
oversizedValueHandler = builder.getOversizedValueHandler();
}
30 changes: 29 additions & 1 deletion src/main/java/com/amazon/ion/IonBufferConfiguration.java
Original file line number Diff line number Diff line change
@@ -8,6 +8,12 @@
*/
public final class IonBufferConfiguration extends BufferConfiguration<IonBufferConfiguration> {

/**
* 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<IonBufferC
*/
private static final int MINIMUM_MAX_VALUE_SIZE = 5;

private static void throwDueToUnexpectedOversizedValue() {
throw new IonException("An oversized value was found even though no maximum size was configured. " +
"This is either due to data corruption or encountering a scalar larger than 2 GB. In the latter case, " +
"an IonBufferConfiguration can be configured to allow the reader to skip the oversized scalar."
);
}

private static final OversizedValueHandler THROWING_OVERSIZED_VALUE_HANDLER = Builder::throwDueToUnexpectedOversizedValue;
private static final OversizedSymbolTableHandler THROWING_OVERSIZED_SYMBOL_TABLE_HANDLER = Builder::throwDueToUnexpectedOversizedValue;

/**
* An OversizedValueHandler that does nothing.
*/
@@ -120,6 +136,11 @@ public OversizedValueHandler getNoOpOversizedValueHandler() {
return NO_OP_OVERSIZED_VALUE_HANDLER;
}

@Override
public OversizedValueHandler getThrowingOversizedValueHandler() {
return THROWING_OVERSIZED_VALUE_HANDLER;
}

@Override
public DataHandler getNoOpDataHandler() {
return NO_OP_DATA_HANDLER;
@@ -132,6 +153,13 @@ public OversizedSymbolTableHandler getNoOpOversizedSymbolTableHandler() {
return NO_OP_OVERSIZED_SYMBOL_TABLE_HANDLER;
}

/**
* @return an OversizedSymbolTableHandler that always throws {@link IonException}.
*/
public OversizedSymbolTableHandler getThrowingOversizedSymbolTableHandler() {
return THROWING_OVERSIZED_SYMBOL_TABLE_HANDLER;
}

@Override
public IonBufferConfiguration build() {
return new IonBufferConfiguration(this);
@@ -151,7 +179,7 @@ private IonBufferConfiguration(Builder builder) {
super(builder);
if (builder.getOversizedSymbolTableHandler() == null) {
requireUnlimitedBufferSize();
oversizedSymbolTableHandler = builder.getNoOpOversizedSymbolTableHandler();
oversizedSymbolTableHandler = builder.getThrowingOversizedSymbolTableHandler();
} else {
oversizedSymbolTableHandler = builder.getOversizedSymbolTableHandler();
}
36 changes: 21 additions & 15 deletions src/main/java/com/amazon/ion/impl/IonCursorBinary.java
Original file line number Diff line number Diff line change
@@ -278,6 +278,16 @@
private long lastReportedByteTotal = 0;


/**
* @return the given configuration's DataHandler, or null if that DataHandler is a no-op.
*/
private static BufferConfiguration.DataHandler getDataHandler(IonBufferConfiguration configuration) {
// Using null instead of a no-op handler enables a quick null check to skip calculating the amount of data
// processed, improving performance.
BufferConfiguration.DataHandler dataHandler = configuration.getDataHandler();
return dataHandler == IonBufferConfiguration.DEFAULT.getDataHandler() ? null : dataHandler;
Copy link
Contributor

@popematt popematt Dec 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could do something like this. It makes the intention a little more clear. However, if it would be one of those surprisingly invasive changes, then don't bother with it.

Suggested change
return dataHandler == IonBufferConfiguration.DEFAULT.getDataHandler() ? null : dataHandler;
return dataHandler == DataHandler.NO_OP ? null : dataHandler;

}

/**
* Constructs a new fixed (non-refillable) cursor from the given byte array.
* @param configuration the configuration to use. The buffer size and oversized value configuration are unused, as
@@ -292,7 +302,7 @@
int offset,
int length
) {
this.dataHandler = (configuration == null) ? null : configuration.getDataHandler();
this.dataHandler = getDataHandler(configuration);
peekIndex = offset;
valuePreHeaderIndex = offset;
checkpoint = peekIndex;
@@ -310,12 +320,6 @@
refillableState = null;
}

/**
* The standard {@link IonBufferConfiguration}. This will be used unless the user chooses custom settings.
*/
private static final IonBufferConfiguration STANDARD_BUFFER_CONFIGURATION =
IonBufferConfiguration.Builder.standard().build();

/**
* @param value a non-negative number.
* @return the exponent of the next power of two greater than or equal to the given number.
@@ -352,13 +356,13 @@
private static final IonBufferConfiguration[] FIXED_SIZE_CONFIGURATIONS;

static {
int maxBufferSizeExponent = logBase2(STANDARD_BUFFER_CONFIGURATION.getInitialBufferSize());
int maxBufferSizeExponent = logBase2(IonBufferConfiguration.DEFAULT.getInitialBufferSize());
FIXED_SIZE_CONFIGURATIONS = new IonBufferConfiguration[maxBufferSizeExponent + 1];
for (int i = 0; i <= maxBufferSizeExponent; i++) {
// Create a buffer configuration for buffers of size 2^i. The minimum size is 8: the smallest power of two
// larger than the minimum buffer size allowed.
int size = Math.max(8, (int) Math.pow(2, i));
FIXED_SIZE_CONFIGURATIONS[i] = IonBufferConfiguration.Builder.from(STANDARD_BUFFER_CONFIGURATION)
FIXED_SIZE_CONFIGURATIONS[i] = IonBufferConfiguration.Builder.from(IonBufferConfiguration.DEFAULT)
.withInitialBufferSize(size)
.withMaximumBufferSize(size)
.build();
@@ -370,6 +374,9 @@
* @param configuration the configuration to validate.
*/
private static void validate(IonBufferConfiguration configuration) {
if (configuration == null) {
throw new IllegalArgumentException("Buffer configuration must not be null.");

Check warning on line 378 in src/main/java/com/amazon/ion/impl/IonCursorBinary.java

Codecov / codecov/patch

src/main/java/com/amazon/ion/impl/IonCursorBinary.java#L378

Added line #L378 was not covered by tests
}
if (configuration.getInitialBufferSize() < 1) {
throw new IllegalArgumentException("Initial buffer size must be at least 1.");
}
@@ -396,10 +403,10 @@
if (alreadyReadLen > 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 @@
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;
Original file line number Diff line number Diff line change
@@ -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;
5 changes: 4 additions & 1 deletion src/main/java/com/amazon/ion/system/IonReaderBuilder.java
Original file line number Diff line number Diff line change
@@ -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;
}

Original file line number Diff line number Diff line change
@@ -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<IonReader> 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);
}
}
13 changes: 10 additions & 3 deletions src/test/java/com/amazon/ion/system/IonReaderBuilderTest.java
Original file line number Diff line number Diff line change
@@ -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