Fix unregistering ini entries of dynamically loaded extension

[arnaud-lb]

This fixes a crash involving ZTS and dl().

In ZTS, the global registered_zend_ini_directives is copied to a thread-specific EG(ini_entries). Extensions loaded with dl() register their ini entries in EG(ini_entries), however they unregister them from registered_zend_ini_directives, which has no effect and leaves the entries in EG(ini_entries). This leads to problems when destroying EG(ini_entries) later, because it references resources that are already freed.

This change ensures that MODULE_TEMPORARY modules will unregister their entries from EG(ini_entries).

I added a test extension specifically for the purpose of testing dl(). I could have used the zend_test for that, but I felt that with a dedicated extension it would be harder to accidentally nerf/no-op the tests.

[cmb69]

This is supposed to fix #8185, I think.

[cmb69]

Thank you for the PR! Before more detailed review, we need to clarify whether this constitutes an ABI break (if so, we cannot target any of the stable PHP versions).

[cmb69]

Overall, this looks good to me, but I left some style nits.

[cmb69]

Do we really need this file? Isn't that already handled by .gitignore in the root?

[cmb69]

I think you should add yourself as author.

[cmb69]

I don't see the point in having an own version for a bundled extension; consider to use PHP_VERSION instead of some hard-coded value.

[cmb69]

I prefer

Suggested change

	if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
	if (PHP_OS_FAMILY === 'Windows') {

[cmb69]

Do we have to cater to macOS which uses .dylib as far as I know?

[arnaud-lb]

I checked because I was not sure. The PHP build appears to generate .so files. The CI looks ok too (no mention of skipping or failing these tests).

[cmb69]

I very much prefer to explicitly have the closing PHP tags in .phpt files, so that syntax highlighting works.

[Girgias]

Aren't they also required for another reason? But yes I agree adding the closing tags is better

[cmb69]

No. The sections containing PHP code are extracted to temporary files, so closing tags are not strictly needed.

[bwoebi]

For me personally it's because I like to be able to run tests quickly without invoking run-tests, by just doing php thetest.phpt

[Girgias]

Other than the PHPT test changes LGTM

[Girgias]

Suggested change

	<?php include __DIR__ . "/../../../dl_test/tests/skip.inc";
	<?php include dirname(__DIR__, 3) . "/dl_test/tests/skip.inc";

Might be 2, not sure of the top of my head.

[Girgias]

Aren't they also required for another reason? But yes I agree adding the closing tags is better

[bukka]

@arnaud-lb just stumbled on this when fixing FPM extension loading using php_admin_value[extension] . It actaully uses php_dl and initializes permanent module so this is actually not going to work so I had to remove this assertion in #12277 . FPM is single threaded so it should not impact this issue - it just really needs to remove the assertion.

There might be potentially other issues with using dl for permanent modules and it has got some limitation but think the whole concept of using php_dl in FPM is really problematic which will require some bigger changes in FPM to get it rif of it (introduction of pool manager).

Anyway if you see some bigger problem with my change, please comment on that PR.