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

Fix ArrayIndexOutofBoundsException found by LGTM.com #510

Closed
wants to merge 1 commit into from
Closed

Fix ArrayIndexOutofBoundsException found by LGTM.com #510

wants to merge 1 commit into from

Conversation

alexet
Copy link

@alexet alexet commented Jan 16, 2019

Seen on LGTM.com here

As codes.length == maxCode so if i == maxCode an ArrayIndexOutOfBoundsException is thrown. This happens when ALLOW_UNQUOTED_FIELD_NAMES is enabled and character 256 is found as part of a field name after needing to consume more data from the reader.

A gist containing code to trigger this path can be found here. I could find any tests for this class but if there is a place to add tests I can add the example as a test.

(Full disclosure: I'm part of the company behind LGTM.com)

@cowtowncoder
Copy link
Member

First of all, thank you for reporting this.

But would it be possible to write a test that shows how this actually works? It would be great to have a regression test, to guard against this happening in future.

@alexet
Copy link
Author

alexet commented Jan 16, 2019

The gist I posted was an example that I can make into a test but like I said, I wasn't sure where the test should go. I can't seem to find any general parsing tests.

@cowtowncoder
Copy link
Member

@aeyerstaylor ah sorry, I skimmed over issue text too fast. That should be enough, I can work with it.
There are a few tests similar to this. I'll also need to patch this in 2.9 branch (then merge forward) so I might as well make these, cover both byte- and char-backed parsers.

cowtowncoder added a commit that referenced this pull request Jan 17, 2019
@cowtowncoder
Copy link
Member

@aeyerstaylor I ended up merging this manually -- thank you very much for submitting the report, fix! And if you find other likely bugs via LGTM (I noticed there were couple of other warnings), looking forward to more reports!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants