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

[nielsdos]

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 error 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.

Important I have an alternative patch for this here: https://gist.github.com/nielsdos/f70055db1876c95aa1bd6ade3c0dd285
I chose the approach in this PR however because it seems like the safer option. E.g. depending on how a SAPI restarts, and the fact that dlclose doesn't necessarily unload a DSO, the zend_module_entry may survive, so the handle == NULL check may not be safe (i.e. may not be true even though the module is not in the registry). I'm mainly thinking about embed in this case.

[dstogov]

Instead of reverting modifications, I would prefer to check if the module was already loaded before doing any modification.

The proposed approach should also work.

[iluuu1994]

I would agree with Dmitry, checking whether a module is already loaded sounds like a more robust solution.

[nielsdos]

I would agree with Dmitry, checking whether a module is already loaded sounds like a more robust solution.

@iluuu1994

I have an alternative solution (linked at the bottom of the PR description) that checks if the handle is null and then warns that the module is already loaded and bails out.
I believe that approach is unsafe, or at least less safe, than this one. Consider for example an embed SAPI. It may start and stop modules many times, but only the first time will a dynamic linked library be loaded. Subsequent module loads will return the same module entry as before, and therefore the handle pointer will be non-NULL causing the module to fail loading while it should actually succeed. An option could be to set the handle to NULL after a module it shut down, but I'm not completely sure what consequences that may be.
Maybe it's possible to check if the handle is still the same, then we know it's a double load, but I'm not so sure if dlopen always guarantees that the handle stays the same.

Another approach is doing the same module name lookup in the registry that zend_register_module_ex does. However, it seems silly to allocate a name to do that lookup, the approach in this PR seems cheaper. And we cannot move the check to the caller as its part of the public API.

WDYT?

[dstogov]

The alternative solution may be really unsafe. I'm not sure about multiple dlload()/dlunload(). It's possible to reset module_entry->handle everywhere before DL_UNLOAD().

Lookup in the module registry and string lowercasing should be a performance problem. dlopen() should take significantly more time anyway. But I also wouldn't like additional complex checks.

Anyway, modification of not owned data with the following roll-back looks wrong.

I think, a better solution might be modifying prototype/behavior of zend_register_module_ex() to make module_entry modification only after registration. (This probably won't work for old branches, and we still have to use your solution). Can you please think in this direction a bit?