Skip to content
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

Ignore libxml leaks #16521

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Ignore libxml leaks #16521

wants to merge 3 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 20, 2024

At least on Windows with libxml2 2.11.9, calling the default entity loader leaks a couple of bytes. Since we cannot do anything about that, we ignore these leaks.


These leaks can be detected when running ext/zend_test/tests/observer_error_04.phpt with Windows debug heap enabled (configure --enable-debug; set PHP_WIN32_DEBUG_HEAP=1; and the usual environment variables). The leaks also can be seen ten soap tests.

With this patch, no further leaks are detected in ext/soap/tests, but I found some in ext/dom/tests; need to check this.

At least on Windows with libxml2 2.11.9, calling the default entity
loader leaks a couple of bytes.  Since we cannot do anything about
that, we ignore these leaks.
@cmb69
Copy link
Member Author

cmb69 commented Oct 20, 2024

More to come soon, including a fix for ZEND_IGNORE_LEAKS_BEGIN|END due to unforeseen recursion.

`xmlRelaxNGParse()` calls `xmlRelaxNGInitTypes()` and that allocates
memory. libxml2 recommends to call `xmlCleanupParser()` to free this
memory, but we don't call this function for reasons.
We cater to two more potentially leaking libxml functions.

We also need to update the `ZEND_IGNORE_LEAKS_BEGIN|END()` macros,
since we now have to cater to recursion, so we need to store the
original value of `_crtDbgFlag` and reset it afterwards.
@cmb69
Copy link
Member Author

cmb69 commented Oct 20, 2024

As it is now, ZEND_IGNORE_LEAKS_BEGIN() is a leaky abstraction, since it introduces a variable declaration in the surrounding scope, but contrary to PHP_LIBXML_SANITIZE_GLOBALS() doesn't give an opportunity to make the variable name unique. Still, it might be good enough to not break BC for potential clients outside of php-src.

Alternatively, we could break BC (and introduce a new block scope), or introduce new macros.


Anyhow, given that our MSan builds apparently don't include any external libraries (because these are not instrumented by default), it might be a good idea to have nightly Windows debug builds which could check for memory leaks. The debug heap is, however, currently only supported with cli (neither cgi nor phpdbg).

@cmb69 cmb69 changed the title Ignore xmlDefaultExternalEntityLoader leaks Ignore libxml leaks Oct 20, 2024
@cmb69 cmb69 marked this pull request as ready for review October 20, 2024 22:13
@cmb69 cmb69 requested a review from nielsdos as a code owner October 20, 2024 22:13
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.

1 participant