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

Fix GH-14702: DOMDocument::xinclude() crash #14704

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

The xinclude code from libxml removes the fallback node, but the fallback node is still reference via $fallback. The solution is to detach the nodes that are going to be removed in advance.

The xinclude code from libxml removes the fallback node,
but the fallback node is still reference via $fallback.
The solution is to detach the nodes that are going to be removed in
advance.
@nielsdos nielsdos linked an issue Jun 28, 2024 that may be closed by this pull request
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess no choice, hopefully no real impact perf wise

@nielsdos
Copy link
Member Author

I guess no choice, hopefully no real impact perf wise

Yeah I don't see a better way, I tried putting all the cheap checks first.

We're actually already doing a tree traversal to get rid of the xinclude fake start and end nodes.
I suppose the main bottleneck here is data cache utilisation...

In an ideal world we should combine the php_dom_remove_xinclude_nodes into the code I added such that the performance impact is minimal, but that requires some refactoring as php_dom_remove_xinclude_nodes is recursive instead of iterative, and should be left for master.

@nielsdos
Copy link
Member Author

In an ideal world we should combine the php_dom_remove_xinclude_nodes into the code I added such that the performance impact is minimal, but that requires some refactoring as php_dom_remove_xinclude_nodes is recursive instead of iterative, and should be left for master.

Err scratch that, the reference stripping must happen before xinclude, but the pre-existing removal of the fake nodes must happen after the xinclude... Annoying....

@nielsdos nielsdos closed this in 42908f9 Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOMDocument::xinclude() crash
2 participants