Fix GH-12215: Module entry being overwritten causes type errors in ext/dom #12246
Conversation
… ext/dom When we try to load an extension multiple times, we still overwrite the type, module number, and handle. If the module number is used to indicate module boundaries (e.g. in reflection and in dom, see e.g. dom_objects_set_class_ex), then all sorts of errors can happen. In the case of ext/dom, OP's error happens because the following happens: - The property handler is set up incorrectly in dom_objects_set_class_ex() because the wrong module number is specified. The class highest in the hierarchy is DOMNode, so the property handler is incorrectly set to that of DOMNode instead of DOMDocument. - The documentElement property doesn't exist on DOMNode, it only exists on DOMDocument, so it tries to read using zend_std_read_property(). As there is no user property called documentElement, that read operation returns an undef value. However, the type is still checked, resulting in the strange exception. Solve this by changing the API such that the data is only overwritten if it's owned data.
| zend_str_tolower_copy(ZSTR_VAL(lcname), module->name, name_len); | ||
|
|
||
| int module_number = zend_next_free_module(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK. I don't see any problems. I think the old PR may be merged into old branches and this one into master.
| @@ -59,9 +59,7 @@ zend_module_entry zend_builtin_module = { /* {{{ */ | |||
|
|
|||
| zend_result zend_startup_builtin_functions(void) /* {{{ */ | |||
| { | |||
| zend_builtin_module.module_number = 0; | |||
There was a problem hiding this comment.
This 0 may be lost. It wasn't used. Right?
There was a problem hiding this comment.
Yes, unless I change the implementation of zend_next_free_module to not add 1.
|
@nielsdos |
Some places, possibly also outside PHP, assume the core extension has module number 0. After 8a812c3 this wasn't the case anymore as reported in [1]. Fix it by changing how the next module ID is computed. [1] php#12246 (comment)
|
@shivammathur That's an unintended regression, fix here: #12272 |
* Make sure core module has number 0 Some places, possibly also outside PHP, assume the core extension has module number 0. After 8a812c3 this wasn't the case anymore as reported in [1]. Fix it by changing how the next module ID is computed. [1] #12246 (comment) * Add assertion * Add test
Replaces #12219 for master
When we try to load an extension multiple times, we still overwrite the type, module number, and handle. If the module number is used to indicate module boundaries (e.g. in reflection and in dom, see e.g. dom_objects_set_class_ex), then all sorts of errors can happen.
In the case of ext/dom, OP's error happens because the following happens:
Solve this by changing the API such that the data is only overwritten if it's owned data.