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` classes contain assert statement which could throw unexpected `AssertionError` #417

Closed arthurscchan closed 9 months ago

arthurscchan commented 9 months ago

In the IonParser::getText() method, there is a call to the IonReader::stringValue() which is served by an Amazon implementation of IonReaderTextSystemX. The method does throw UnknownSymbolException if the symbol id cannot be resolved. But it also contains some assert statements which throw AssertionError when the resolved symbol id is 0 or negative. The AssertionError`` is not handled and is thrown to the users unexpectedly.

    @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;
    }

The simplest fix is to also catch the AssertionError, the same as the UnkonwnSymbolException. In general, AssertionError should be internal use only and should be wrapped and avoided by throwing directly to the users. Not sure if it is meant to not handle it in this situation.

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

cowtowncoder commented 9 months ago

Thank you @arthurscchan ! I merged the PR, made some minor changes (not to logic but just testing, message) and it should go in 2.17. And Fuzzer can hopefully clear up one of crash cases.