Skip to content

Revert "Stop copying zend_module_entry (#8551)" #9648

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

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Sep 30, 2022

This reverts commit b63df3c.

Fixes #9589

After all, copying the module entry was useful. This at least prevents overriding the module entry in case the module is loaded again in

php-src/ext/standard/dl.c

Lines 228 to 230 in ed0f1f0

module_entry->type = type;
module_entry->module_number = zend_next_free_module();
module_entry->handle = handle;

@arnaud-lb arnaud-lb added this to the PHP 8.2 milestone Sep 30, 2022
@cmb69
Copy link
Member

cmb69 commented Sep 30, 2022

An alternative might be to catch double-loading of modules:

 ext/standard/dl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ext/standard/dl.c b/ext/standard/dl.c
index aae0996384..b8d1cbc78c 100644
--- a/ext/standard/dl.c
+++ b/ext/standard/dl.c
@@ -205,6 +205,10 @@ PHPAPI int php_load_extension(const char *filename, int type, int start_now)
 		return FAILURE;
 	}
 	module_entry = get_module();
+	if (zend_hash_str_exists(&module_registry, module_entry->name, strlen(module_entry->name))) {
+		zend_error(E_CORE_WARNING, "Module \"%s\" is already loaded", module_entry->name);
+		return FAILURE;
+	}
 	if (module_entry->zend_api != ZEND_MODULE_API_NO) {
 			php_error_docref(NULL, error_type,
 					"%s: Unable to initialize module\n"

@morrisonlevi
Copy link
Contributor

I think detecting the double-load makes sense, even if we choose to revert this change for PHP 8.

@arnaud-lb
Copy link
Member Author

@cmb69 do you want to pursue the double-load catching before RC4 ?

@cmb69
Copy link
Member

cmb69 commented Oct 7, 2022

@arnaud-lb, sure, I can do that. Will provide a PR shortly.

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.

3 participants