better defensive behavior when libxml2 or libxslt will make unsafe modifications to a document #2829
Conversation
This file contains 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
flavorjones
added
the
topic/memory
|
Memcheck caught that I need to free the doc returned from |
when there are blank text node objects. previously we raised an exception.
flavorjones
force-pushed
the
flavorjones-2800-xslt-modifying-doc
branch
from
53120ec
to
7e6045d
Compare
which it does be default when xsl:strip-space is used this approach makes a defensive copy of the doc if there's a chance the original may be modified in an unsafe way: - if any spaces will be stripped - and there are blank node objects that might be removed Fixes #2800
flavorjones
force-pushed
the
flavorjones-2800-xslt-modifying-doc
branch
from
7e6045d
to
cfbb42f
Compare
|
Another segfault, again with Darwin and Ruby 3.2: https://github.com/sparklemotion/nokogiri/actions/runs/4381034712/jobs/7668806803#step:5:911 |
|
Whoops, that was due to user error. Still trying to reproduce a crash on darwin. |
|
The segfaults seem to be caused by a bug that was already fixed by https://bugs.ruby-lang.org/issues/19469 (present in Ruby 3.2.0 and 3.2.1). |
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.
What problem is this PR intended to solve?
Closes #2800
#2800 reported behavior in libxslt (upstream https://gitlab.gnome.org/GNOME/libxslt/-/issues/14) wherein using
xsl:strip-spacesmay remove blank text nodes from the original document. If a Ruby object is wrapped around this node, then that's a problem that may lead to a segfault.Fortunately, we can tell if
strip-spacesis going to be applied. We can also (thanks in part to prior work in #2001 to fix #1985) detect when the document has Ruby objects for blank text nodes. If both of these things are true, we make a "defensive" copy of the original document, and pass that copy intoxsltApplyStylesheet. The original then remains unmodified.This might be surprising behavior, and in fact in #2001 @tenderlove opted for an explicit exception to be raised in these conditions. Here I'm opting to just "do what the user means", and if someone is relying on the original being modified and reports a bug we can talk about it then.
Note that I've also backported this approach to the fix from #2001, so in that circumstance we choose to make a defensive copy of the original and pass it into
xmlSchemaNewDocParserCtxtso that the original remains unmodified.Have you included adequate test coverage?
Yes!
Does this change affect the behavior of either the C or the Java implementations?
This kind of edge-case handling is unnecessary in JRuby because this isn't a problem with the Java libraries.