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
Show file tree
Hide file tree
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
7 changes: 6 additions & 1 deletion src/main/java/com/amazon/ion/BufferConfiguration.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -210,7 +215,7 @@ protected BufferConfiguration(Builder<Configuration, ?> builder) {
}
if (builder.getOversizedValueHandler() == null) {
requireUnlimitedBufferSize();
oversizedValueHandler = builder.getNoOpOversizedValueHandler();
oversizedValueHandler = builder.getThrowingOversizedValueHandler();
} else {
oversizedValueHandler = builder.getOversizedValueHandler();
}
Expand Down
30 changes: 29 additions & 1 deletion src/main/java/com/amazon/ion/IonBufferConfiguration.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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();
}
Expand Down
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
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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();
Expand All @@ -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

View check run for this annotation

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.");
}
Expand All @@ -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;
}

/**
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
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
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}
Loading
Loading