FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
310 stars 133 forks source link

`IonReader` throws `NullPointerException` for unchecked invalid data #424

Closed arthurscchan closed 9 months ago

arthurscchan commented 9 months ago

In the IonParser::getText() method, there is a call to the IonReader::stringValue(). Also, in IonParser::getXXXValue() for retrieving different number values from the IonReader calls to underlying IonReader for retrieving string or number value. According to the Javadoc of IonReader, each of the APIs requires a special IonType and IllegalStateException could be thrown if the wrong type is passed. But there is a special case when there is no more input, the IonType will be null and continuing calling those methods will result in NullPointerException.

    @Override
    public String getText() throws IOException
    {
         if (_currToken != null) { // null only before/after document
          ......
            case VALUE_STRING:
                try {
                    // stringValue() will throw an UnknownSymbolException if we're
                    // trying to get the text for a symbol ID that cannot be resolved.
                    return _reader.stringValue();
                } catch (UnknownSymbolException e) {
                    throw _constructError(e.getMessage(), e);
                }
            ......           
        return null;
    }
...
    @Override
    public BigInteger getBigIntegerValue() throws IOException {
        return _reader.bigIntegerValue();
    }

    @Override
    public BigDecimal getDecimalValue() throws IOException {
        return _reader.bigDecimalValue();
    }

    @Override
    public double getDoubleValue() throws IOException {
        return _reader.doubleValue();
    }

    @Override
    public float getFloatValue() throws IOException {
        return (float) _reader.doubleValue();
    }

    @Override
    public int getIntValue() throws IOException {
        return _reader.intValue();
    }

    @Override
    public long getLongValue() throws IOException {
        return _reader.longValue();
    }

It is found that in the IonParser::getNumberValue() method, there is a null check to ensure the IonType (and NumberType) of the current token is not null before calling the corresponding data retrieving method in the IonReader implementation. But these null checks are missing from the above method which could cause unexpected NullPointerException.

    @Override
    public Number getNumberValue() throws IOException {
        NumberType nt = getNumberType();
        if (nt != null) {
            switch (nt) {
            case INT:
                return _reader.intValue();
            case LONG:
                return _reader.longValue();
            case FLOAT:
                return (float) _reader.doubleValue();
            case DOUBLE:
                return _reader.doubleValue();
            case BIG_DECIMAL:
                return _reader.bigDecimalValue();
            case BIG_INTEGER:
                return getBigIntegerValue();
            }
        }
        return null;
    }

The simplest fix is to add a null check similar to the one done in the IonParser::getNumberValue() method.

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