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

throw checked exception if growArray has a size that is too big #1075

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Aug 2, 2023

@cowtowncoder
Copy link
Member

Hmmmh. As per my comment, I don't like the idea of API exception from general-purpose helper.
I am bit meh on checking in general, actually, since it seems unlikely this would be hit. But I guess it's fine.
I'll merge & change exception type.

@cowtowncoder cowtowncoder merged commit c499978 into FasterXML:2.16 Aug 3, 2023
5 checks passed
@pjfanning
Copy link
Member Author

@cowtowncoder The point is to use a checked exception (ie not an exception that extends RuntimeException). Unfortunately, the people who are obsessed with getting CVEs assigned to everything are obsessed about unchecked exceptions. They go bananas when an exception or error occurs that is not part of the API signature. That spring-boot user was upset that jackson failed with java.lang.NegativeArraySizeException.

If StreamConstraintsException is a problem, could we use IOException?

@pjfanning pjfanning deleted the max-token-len branch August 3, 2023 00:33
@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 3, 2023

@pjfanning Let's add proper name size constraints checks and not rely on array expansion exhausting max array size. This is not the way to fix problems -- decoding will become problem way way before int32 exhausts.

EDIT: if we run out of time for release or such, we can change the exception to StreamReadException -- but only in that case. Otherwise let's try to get proper check if at all possible.

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