IndexOutOfBoundsException thrown by IonReader implementations are not handled

[arthurscchan]

The IonParser::nextToken() method relies on the IonReader implementations to retrieve the next token. Those IonReader implementations are provided by the upstream Amazon Java-Ion package and some of the code in those IonReader implementations does mention that if the provided data is malformed, it could throw IndexOutOfBoundsException and that is not handled because it would sacrifice performance. And IonParser::nextToken() fails to handle them nor check if the input is malformed. This results in an unexpected IndexOutOfBoundsException being thrown to the user.

Below are two sample code that mentioned the possible throwing of IndexOutOfBoundsException from the upstream IonCursorBinary::uncheckedReadVarUInt_1_0(byte) method and IonReaderContinuableCoreBinary::readVarInt_1_0 method.

    private int readVarInt_1_0(int firstByte) {
        int currentByte = 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.
            currentByte = buffer[(int)(peekIndex++)];
            result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
        }
        return result * sign;
    }

    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.
            currentByte = buffer[(int) (peekIndex++)];
            result = (result << VALUE_BITS_PER_VARUINT_BYTE) | (currentByte & LOWER_SEVEN_BITS_BITMASK);
        } while (currentByte >= 0);
        return result;
    }

The simplest fix is to catch the IndexOutOfBoundsException and wrap it with the JsonParseException. A better way may be adding some checking before the upstream call to ensure malformed data is detected and exit before calling those upstream methods.

We found this issue by OSS-Fuzz and it is reported in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65062 and https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65083.

[cowtowncoder]

Lol. "Doing the right thing would be bit slower than letting things fail in bad ways" is certainly one way to approach things! :-D :-D :-D

Ok, yeah, If so, need to work around such sub-standard implementation. Will look at PR next.