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

add recursion limit of 500 for DTD parsing #159

Merged
merged 4 commits into from Oct 24, 2022

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Oct 21, 2022

Relates to #157

It's DTD parsing where the recursion happens and who really wants to parse DTDs any more, let alone deeply nested ones? So it seems adequate to just set a recursion limit instead of trying to rewrite the code not to recurse (and the extensive rewrite that would need).

With testing, I've tried a few of the files that supposedly cause the recursion issue but I all I get is UTF-8 parsing issues - so the code never even really gets to the recursion.

I modified the first XML file in ...5219006592450560.zip and removed the UTF-8 issues - this has given me a test case that does fail with the recursion limit kicking in.

@pjfanning pjfanning marked this pull request as draft October 21, 2022 22:59
@pjfanning pjfanning marked this pull request as ready for review October 21, 2022 23:20
@cowtowncoder
Copy link
Member

@pjfanning Excellent, thank you for providing this!

The only thing I was wondering is whether this could be made configurable? I know 500 should typically be enough for real usage so maybe it's not a big deal -- and/or could add configurability if someone actually hits the limit -- but then again other processing limits are settable so there should be an existing pattern to follow. Although it may be (I didn't check the code) that perhaps config object is not being passed to validator.

Another question: if it's easy enough to rebase, could you rebase this against 5.3 branch? If not, not a big deal, but if trivially simple I might want to release 5.4.0 for any users that might still use that.

@pjfanning pjfanning changed the base branch from master to 5.3 October 23, 2022 18:49
@pjfanning
Copy link
Member Author

@cowtowncoder I changed the target branch to 5.3.

I added a static FullDTDReader.setDtdRecursionDepthLimit(final int limit)

@cowtowncoder
Copy link
Member

Ok looks good: I will change configuration to use ReaderConfig class but implementation should be fine.

@cowtowncoder
Copy link
Member

@pjfanning Ok, created #160 as the issue for PR, and changed configuration to be applied similar to all other Woodstox-specific properties (via XMLInputFactory.setProperty()).

Planning to release 5.4.0 and 6.4.0 (minor version upgrade since it's an API change even if minor (I guess one could argue that adding a property could be done in patch but whatever)).

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.

None yet

2 participants