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

Allow overriding of file size limit for YAMLParser by exposing SnakeYAML LoaderOptions #337

Closed
pjfanning opened this issue Sep 19, 2022 · 8 comments
Labels
yaml Issue related to YAML format backend
Milestone

Comments

@pjfanning
Copy link
Member

Relates to #335 (comment)

snakeyaml 1.32 brings in a default limit of 3Mb when parsing yaml files.

Need to allow users to specify another value if they need to.

https://bitbucket.org/snakeyaml/snakeyaml/src/72dfa9f1074abe2b8a6c8776bee4476b0aed02e3/src/main/java/org/yaml/snakeyaml/LoaderOptions.java

@cowtowncoder
Copy link
Member

See my notes on #335: there will be need, likely, to carefully apply any new configurations so as to avoid having a hard limit on very recent SnakeYAML versions, assuming configurability will be added post-1.32 (which seems likely).

@pjfanning
Copy link
Member Author

@cowtowncoder probably best to revert the v2.13 branch to stick with snakeyaml 1.31 - with v2.14, I see YAMLParser has feature support but this is boolean - is there a preferred to set int properties?

@cowtowncoder
Copy link
Member

@pjfanning That might make sense (2.13).

As to int value configuration, right, I think it has to go via YAMLFactory.Builder and no there isn't anything out of the box. I guess there's no point in doing on/off setting here, it being a security feature and realistically I think it makes sense to always have some upper limit, even it was Integer.MAX_VALUE.

On Builder, further, it may get bit trickier with 2.x vs 3.0 since 2.x has no immutability requirement but 3.0 has.

@cowtowncoder
Copy link
Member

Added notes to the PR: my position now is that I kind of like exposing LoaderOptions, to make users configure aspects they want to. This reduces some of compatibility problems, and increases configurability significantly.

It will also be a problem for 2.x/3.0 migration, likely; may need to see how snakeyaml-engine deals with this. Seems likely there is a counterpart to this configuration object, and if so, it may be relatively easy to offer similar extension point.

Put another way, I am ok with tighter bundling of SnakeYAML at least for Jackson 2.x.
There are no alternatives that I know of, and really my only wish would be performance improvements. But functionality is solid and author has been very quick with fixes, releases.

@pjfanning
Copy link
Member Author

pjfanning commented Sep 21, 2022

@cowtowncoder LoaderOptions seems to have been added to snakeyaml in v1.18 - methods have been added over time

@cowtowncoder
Copy link
Member

Ok, 1.18 sounds fine: jackson-dataformat-yaml 2.10.0 required version 1.24 already.
So that's good, no need to (IMO) add dynamic loading there.

Addition of methods sort of works well for us, as long as caller passes instance(s).

@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Sep 24, 2022
@cowtowncoder cowtowncoder added yaml Issue related to YAML format backend 2.14 labels Sep 24, 2022
@cowtowncoder cowtowncoder changed the title Introduce way to override file size limit for YAMLParser Allow overriding of file size limit for YAMLParser by exposing SnakeYAML LoaderOptions Sep 24, 2022
cowtowncoder added a commit that referenced this issue Sep 24, 2022
@pjfanning
Copy link
Member Author

@cowtowncoder I'm closing this because the changes were released with jackson v2.14.0. Feel free to reopen.

@cowtowncoder
Copy link
Member

Thank you @pjfanning -- yes, I had forgotten to close it.

mnonnenmacher added a commit to oss-review-toolkit/ort that referenced this issue Nov 28, 2022
Since version 1.32 [1] SnakeYAML sets the limit for incoming data to
3MB. Since Jackson 2.14 [2] it is possible to override this default, so
hardcode the limit to 1GB to allow parsing large ORT result files.

[1]: https://bitbucket.org/snakeyaml/snakeyaml/wiki/Changes
[2]: FasterXML/jackson-dataformats-text#337

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
sschuberth pushed a commit to oss-review-toolkit/ort that referenced this issue Nov 28, 2022
Since version 1.32 [1] SnakeYAML sets the limit for incoming data to
3MB. Since Jackson 2.14 [2] it is possible to override this default, so
hardcode the limit to 1GB to allow parsing large ORT result files.

[1]: https://bitbucket.org/snakeyaml/snakeyaml/wiki/Changes
[2]: FasterXML/jackson-dataformats-text#337

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yaml Issue related to YAML format backend
Projects
None yet
Development

No branches or pull requests

2 participants