-
Notifications
You must be signed in to change notification settings - Fork 42
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
Denial of service when parsing a JSON object with an unexpected field that has a big number #118
Comments
A similar issue can be reproduced using jackson-module-scala: FasterXML/jackson-module-scala#609 |
Thanks for the detailed report. I see that this branch is using jackson-core 2.14-rc2. Do you get the same results with the jackson-core 2.13.x version used by weepickle? |
From a quick glance at the source code, it seems like jackson-core 2.13 should be affected as well.
weepickle is push-based, so we can't easily do that. The closest option I can think of without breaking binary compatibility would be to force all numbers through newly allocated strings, then handle number parsing on the visitor side. However, I expect that would significantly hurt common-case performance. Ideally, I'd prefer to see a fix in jackson-core. |
Another non-ideal but slightly better workaround might be to call |
I put small improvement into jackson-core for the v2.14.0 release so that parsing very large bigints should be a little bit faster - but still has sub-quadratic performance. I'm not sure if jackson code benefits from rewriting ParserBase.getNumberType to make the number parsing lazy. I would suggest that you try your idea of calling getLongValue instead of getNumberType. For me, the next priority in jackson-core is to have a limit on the number of chars in a number - FasterXML/jackson-core#815 - @cowtowncoder might have other ideas on this topic Edit: I have added FasterXML/jackson-core#828 which might be enough to solve this - but it might be a bit late for Jackson v2.14 |
Quick note: I did merge @pjfanning's nice patch mentioned above, and it will be included in 2.14.0. Limits on token lengths and various limits will have to wait until 2.15, fwtw. |
@htmldoug the PR I mentioned yesterday is merged and is built into latest jackson-core 2.14.0-SNAPSHOT jar |
@htmldoug would it be possible to upgrade to jackson v2.14.1? |
The jackson v2.14.1 changes seem to help make the performance degradation less dramatic. On my laptop, I got these numbers.
|
@pjfanning thanks for your improvements and verifying the changes.
Yes, this needs to happen. I'm looking forward to it for #117 as well. I need to redo the maven central publishing which is mostly why that hasn't happened yet. In the mean time, consumers who want the latest version can pull in the latest jackson-core as jsoniter-scala has done. |
Sub-quadratic decreasing of throughput when length of the JSON object is increasing
On contemporary CPUs parsing of such JSON object with an additional field that has of 1000000 decimal digits (~1Mb) can took ~13 seconds:
Probably the root cause is in the jackson core library, but reporting hear hoping that a hot fix to just skip unwanted values can be applied on the weePickle level.
Flame graph for CPU cycles at size=1000000
weePickle version
1.8.0
Scala version
2.13.10
JDK version
17
Steps to reproduce
To run that benchmarks on your JDK:
sbt
and/or ensure that it already installed properly:jsoniter-scala
repo:The text was updated successfully, but these errors were encountered: