Skip to content

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