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

IonReader classes contain assert statement which could throw unexpected AssertionError #417

Closed
arthurscchan opened this issue Dec 5, 2023 · 1 comment · Fixed by #418
Labels
2.17 fuzz Issue found by OssFuzz ion
Milestone

Comments

@arthurscchan
Copy link
Contributor

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 cowtowncoder added ion fuzz Issue found by OssFuzz 2.17 labels Dec 6, 2023
@cowtowncoder cowtowncoder changed the title IonReader classes contain assert statement which could throw unexpected AssertionError IonReader classes contain assert statement which could throw unexpected AssertionError Dec 6, 2023
cowtowncoder added a commit that referenced this issue Dec 6, 2023
@cowtowncoder
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 fuzz Issue found by OssFuzz ion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants