Skip to content

Fix bugs GH-16150 and GH-16152: intern document mismanagement - #16178

Closed
ndossche wants to merge 1 commit into
php:PHP-8.3from
ndossche:fix-dom-doc-refs
Closed

ndossche wants to merge 1 commit into
php:PHP-8.3from
ndossche:fix-dom-doc-refs

Conversation

@ndossche

@ndossche ndossche commented Oct 2, 2024

Copy link
Copy Markdown
Member

The reference counts of the internal document pointer are mismanaged. In the case of fragments the refcount may be increased too much, while for other cases the document reference may not be applied to all children.

This bug existed for a long time and this doesn't reproduce (easily) on 8.2 due to other bugs. Furthermore 8.2 will enter security mode soon, and this change may be too risky.

The reference counts of the internal document pointer are mismanaged.
In the case of fragments the refcount may be increased too much, while
for other cases the document reference may not be applied to all
children.

This bug existed for a long time and this doesn't reproduce (easily)
on 8.2 due to other bugs. Furthermore 8.2 will enter security mode soon,
and this change may be too risky.

@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 feel like I get the spirit of the change, but not exactly how it achieves this.

But then I've never dug into the internals of libxml

@ndossche

ndossche commented Oct 3, 2024

Copy link
Copy Markdown
Member Author

It moves all document setting operations to one place where each child gets changed at most once, fixing both the issues. Previously fragments could cause a child to be updated twice, and it would also only change direct descendants and not indirect descendants.

@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.

Okay this makes sense :) Could you add this in the commit message?

@ndossche

ndossche commented Oct 3, 2024

Copy link
Copy Markdown
Member Author

Could you add this in the commit message?

Sure

@ndossche ndossche closed this in d4a4d2e Oct 3, 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.

Memory leak in DOMProcessingInstruction/DOMDocument Use after free in php_dom.c

2 participants