Skip to content

Fix GH-12215: Module entry being overwritten causes type errors in ext/dom #12246

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.

Sign up for GitHub

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
Closed

Fix GH-12215: Module entry being overwritten causes type errors in ext/dom #12246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions UPGRADING.INTERNALS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ PHP 8.4 INTERNALS UPGRADE NOTES
1. Internal API changes
========================

* zend_register_module_ex() now takes an additional int module_type argument.
This function will also assign the module number and type, there is no need
to do this at the call site anymore. Writing the handle should happen after
successful registration.

========================
2. Build system changes
========================
Expand Down
15 changes: 9 additions & 6 deletions Zend/zend_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2446,7 +2446,7 @@ ZEND_API void zend_destroy_modules(void) /* {{{ */
}
/* }}} */

ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module) /* {{{ */
ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module, int module_type) /* {{{ */
{
size_t name_len;
zend_string *lcname;
Expand Down Expand Up @@ -2483,9 +2483,11 @@ ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module) /
}

name_len = strlen(module->name);
lcname = zend_string_alloc(name_len, module->type == MODULE_PERSISTENT);
lcname = zend_string_alloc(name_len, module_type == MODULE_PERSISTENT);
zend_str_tolower_copy(ZSTR_VAL(lcname), module->name, name_len);

int module_number = zend_next_free_module();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you call zend_next_free_module(); at this point?
May be it's better to delay it until the module->module_number asisgnment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zend_next_free_module does zend_hash_num_elements(&module_registry) + 1;. So if I move it after adding, the module numbers start at 2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I don't see any problems. I think the old PR may be merged into old branches and this one into master.


lcname = zend_new_interned_string(lcname);
if ((module_ptr = zend_hash_add_ptr(&module_registry, lcname, module)) == NULL) {
zend_error(E_CORE_WARNING, "Module \"%s\" is already loaded", module->name);
Expand All @@ -2495,7 +2497,10 @@ ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module) /
module = module_ptr;
EG(current_module) = module;

if (module->functions && zend_register_functions(NULL, module->functions, NULL, module->type)==FAILURE) {
module->module_number = module_number;
module->type = module_type;

if (module->functions && zend_register_functions(NULL, module->functions, NULL, module_type)==FAILURE) {
zend_hash_del(&module_registry, lcname);
zend_string_release(lcname);
EG(current_module) = NULL;
Expand All @@ -2511,9 +2516,7 @@ ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module) /

ZEND_API zend_module_entry* zend_register_internal_module(zend_module_entry *module) /* {{{ */
{
module->module_number = zend_next_free_module();
module->type = MODULE_PERSISTENT;
return zend_register_module_ex(module);
return zend_register_module_ex(module, MODULE_PERSISTENT);
}
/* }}} */

Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_API.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ ZEND_API zend_result zend_register_functions(zend_class_entry *scope, const zend
ZEND_API void zend_unregister_functions(const zend_function_entry *functions, int count, HashTable *function_table);
ZEND_API zend_result zend_startup_module(zend_module_entry *module_entry);
ZEND_API zend_module_entry* zend_register_internal_module(zend_module_entry *module_entry);
ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module);
ZEND_API zend_module_entry* zend_register_module_ex(zend_module_entry *module, int module_type);
ZEND_API zend_result zend_startup_module_ex(zend_module_entry *module);
ZEND_API void zend_startup_modules(void);
ZEND_API void zend_collect_module_handlers(void);
Expand Down
4 changes: 1 addition & 3 deletions Zend/zend_builtin_functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,7 @@ zend_module_entry zend_builtin_module = { /* {{{ */

zend_result zend_startup_builtin_functions(void) /* {{{ */
{
zend_builtin_module.module_number = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This 0 may be lost. It wasn't used. Right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unless I change the implementation of zend_next_free_module to not add 1.

zend_builtin_module.type = MODULE_PERSISTENT;
return (EG(current_module) = zend_register_module_ex(&zend_builtin_module)) == NULL ? FAILURE : SUCCESS;
return (EG(current_module) = zend_register_module_ex(&zend_builtin_module, MODULE_PERSISTENT)) == NULL ? FAILURE : SUCCESS;
}
/* }}} */

Expand Down
7 changes: 3 additions & 4 deletions ext/standard/dl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,15 +230,14 @@ PHPAPI int php_load_extension(const char *filename, int type, int start_now)
DL_UNLOAD(handle);
return FAILURE;
}
module_entry->type = type;
module_entry->module_number = zend_next_free_module();
module_entry->handle = handle;

if ((module_entry = zend_register_module_ex(module_entry)) == NULL) {
if ((module_entry = zend_register_module_ex(module_entry, type)) == NULL) {
DL_UNLOAD(handle);
return FAILURE;
}

module_entry->handle = handle;

if ((type == MODULE_TEMPORARY || start_now) && zend_startup_module_ex(module_entry) == FAILURE) {
DL_UNLOAD(handle);
return FAILURE;
Expand Down
8 changes: 3 additions & 5 deletions sapi/phpdbg/phpdbg_prompt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,16 +1314,14 @@ PHPDBG_API const char *phpdbg_load_module_or_extension(char **path, const char *
goto quit;
}

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

if ((module_entry = zend_register_module_ex(module_entry)) == NULL) {
if ((module_entry = zend_register_module_ex(module_entry, MODULE_PERSISTENT)) == NULL) {
phpdbg_error("Unable to register module %s", *name);

goto quit;
}

module_entry->handle = handle;

if (zend_startup_module_ex(module_entry) == FAILURE) {
phpdbg_error("Unable to startup module %s", module_entry->name);

Expand Down