Skip to content

Fix GH-17145: DOM memory leak - #17147

Closed
ndossche wants to merge 1 commit into
php:PHP-8.4from
ndossche:fix-17145
Closed

ndossche wants to merge 1 commit into
php:PHP-8.4from
ndossche:fix-17145

Conversation

@ndossche

Copy link
Copy Markdown
Member

Because the use of RETURN instead of RETVAL, the freeing code could not be executed. This only is triggerable if the content of the attribute is mixed text and entities, so it wasn't noticed earlier.

@ndossche ndossche linked an issue Dec 13, 2024 that may be closed by this pull request
Comment thread ext/dom/tests/gh17145.phpt Outdated

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.

Looking at CI, is this & difference coming from libxml2 versions ?

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.

Looks like it :( It seems that the CI jobs using a more recent version of libxml have the correct output while the older versions do not. I'll run a bisect on libxml to see what commit in libxml fixed this.

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.

This was fixed in GNOME/libxml2@b8597f4, so it's a pretty recent fix and since distros are quite behind on libxml updates, it'll take some time for them to get this fix.
It would be possible to patch this in PHP too, but I'm going for a SKIPIF right now as I don't want to add more workarounds than we already have to be honest...

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.

Not really happy about dealing with these kinds of stuff

Because the use of RETURN instead of RETVAL, the freeing code could not
be executed. This only is triggerable if the content of the attribute is
mixed text and entities, so it wasn't noticed earlier.
@ndossche ndossche closed this in 4656c22 Dec 14, 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.

DOM memory leak

3 participants