Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Oct 2, 2024

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.
Copy link
Member

@Girgias Girgias left a comment

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

@nielsdos
Copy link
Member Author

nielsdos commented Oct 3, 2024

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.

Copy link
Member

@Girgias Girgias left a comment

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?

@nielsdos
Copy link
Member Author

nielsdos commented Oct 3, 2024

Could you add this in the commit message?

Sure

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