Repository navigation
Prevent calling clear() on shared EmptyNodeList object. - #2620
Merged
Merged
Conversation
The object EmptyNodeList is shared across instances of Element, as long as they don't have any children. The Element.empty() operation unconditionally calls clear() on that object, which modifies its internal state and becomes a source of ConcurrentModificationException. The included test serves as a reproducer of this concurrent modification problem; manipulation of a Stream of the EmptyNodeList object on new Element instances is affected by calls to Element.empty() on different objects in another thread.
Prevents CMEs on the shared empty list, and unnecessarry mods.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Owner
|
Thanks! Merged. I updated the fix a bit to check for any empty list not just the shared one (so we don't spuriously invalidate the list for element-allocated empty lists), and simplified the test coverage to more directly check the modcount. |
jhy
added
bug
Contributor
Author
|
Thanks for your review, I definitely learned something new! Best regards. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The object EmptyNodeList is shared across instances of Element, as long as they don't have any children. The Element.empty() operation unconditionally calls clear() on that object, which modifies its internal state and becomes a source of ConcurrentModificationException.
The included test serves as a reproducer of this concurrent modification problem; manipulation of a Stream of the EmptyNodeList object on new Element instances is affected by calls to Element.empty() on different objects in another thread.
We came across this problem when running tests of a piece of software, which has a pipeline that produces HTML documents. These tests use the pipeline to produce documents, then parse the result and check it against a pre-created model file. When running this on CI with multiple tests running in parallel, we got ConcurrentModificationExceptions even when these documents were independent from each other, pointing to some shared state across jsoup objects.
Thank you in advance for considering this patch for review!