Repository navigation
Reject symbol IDs and VarInts that overflow int in the binary reader - #1173
Open
radhika1314 wants to merge 1 commit into
Open
radhika1314 wants to merge 1 commit into
radhika1314 wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: n/a
Description of changes:
The Ion 1.0 binary reader decodes VarUInt and VarInt sub-fields into a Java
intwithout checking that they fit, and narrows field name and symbol value IDs fromlongwith a plain cast. A 5-byte VarUInt of 2^32 + 4 wraps to 4, so an out-of-range symbol ID used as a field name, annotation or symbol value resolves to$4(name) instead of failing the wayfieldNameSymbolIDUnmapped.10nandannotationSymbolIDUnmapped.10nin ion-tests require. The same wrap turns a timestamp year of 2^32 + 2000 into2000T, and a decimal exponent of 2^31 + 1 into -(2^31 - 1), so1d2147483649reads back as1d-2147483647. Two Ion implementations can end up disagreeing about the contents of the same document.Reproducer for the field name case (the others are covered by the new tests):
Before:
{name: 1}. After:IonException: Found a symbol ID that was too large to fit in an int.The checks live in the sub-field readers so that every caller (field names, annotations, symbol values, timestamp fields, decimal exponents) is covered in one place:
readVarUInt_1_0bounds its accumulator,readVarInt_1_0accumulates in alongwith a 2^31 magnitude bound and the decimal scale and timestamp offset are range checked before narrowing, andIonCursorBinary.symbolIdToIntrejects symbol IDs outside theintrange. The VarInt bound still admits an exponent of exactly 2^31, so aBigDecimalwith scaleInteger.MIN_VALUEremains readable.One side effect worth a look:
IonRawBinaryWriter.writeDecimalValuecomputes the exponent as-value.scale()inintarithmetic, so a scale ofInteger.MIN_VALUEis written as exponent -2^31 and only round tripped because the reader overflowed the same way. That encoding now fails to read back. I kept this change on the read side, but can fold the writer fix in here if you would rather have both together.Regression tests were added to
IonReaderContinuableCoreBinaryTestandIonReaderContinuableTopLevelBinaryTest;./gradlew buildandminifyTest21pass locally.