Skip to content

Fix GH-17847: xinclude destroys live node - #17878

Closed
ndossche wants to merge 3 commits into
php:PHP-8.3from
ndossche:fix-17847
Closed

ndossche wants to merge 3 commits into
php:PHP-8.3from
ndossche:fix-17847

Conversation

@ndossche

Copy link
Copy Markdown
Member

dom_xinclude_strip_fallback_references() now also takes into account xi:include nodes children. This now subsumes all work done normally by the old start/end node removal, so we can remove that code and start using XML_PARSE_NOXINCNODE.

@ndossche ndossche linked an issue Feb 21, 2025 that may be closed by this pull request
dom_xinclude_strip_fallback_references() now also takes into account
xi:include nodes children. This now subsumes all work done normally by
the old start/end node removal, so we can remove that code and start
using XML_PARSE_NOXINCNODE.
Comment thread ext/dom/document.c Outdated
* and remove the original nodes. If the originals are removed while there are still userland references
* this will cause memory corruption. */
static void dom_xinclude_strip_fallback_references(const xmlNode *basep)
static void dom_xinclude_strip_fallback_references(xmlDocPtr docp, const xmlNode *basep)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the docPtr necessary here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Leftover from when I was playing around with this. Dropped.

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can't see anything wrong now, so if CI is green feel free to merge.

@ndossche

Copy link
Copy Markdown
Member Author

Also for future reference: the existing code to remove the xinclude start/end nodes wasn't right anyway as it only looked at the top level AFAICT

@ndossche

Copy link
Copy Markdown
Member Author

Ah great, CI is red on some instances because libxml error messages are not consistent between versions :(
I slapped an @ on it

@Girgias

Girgias commented Feb 21, 2025

Copy link
Copy Markdown
Member

Win CI is unrelated.

@ndossche ndossche closed this in 9beccce Feb 21, 2025
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.

xinclude destroys live node

2 participants