Fix default_object_handlers pointing to invalid memory with file_cache

[iluuu1994]

See https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=28625&view=ms.vss-test-web.build-test-results-tab.

@dstogov Let me know if you see a cleaner solution.

[dstogov]

I wonder how a class linked with internal class may be stored in file cache at all.
opcache_compile_file() sets ZEND_COMPILE_IGNORE_INTERNAL_CLASSES in CG(compiler_options) that should prevent the compile-time linking with internal classes.

[iluuu1994]

Oh. I guess it would be enough to set default_object_handlers to std_object_handlers/zend_enum_object_handlers then?

[dstogov]

Oh shit! I missed, when these ce->default_object_handlers were added :(
That separation of handlers from class entries was done by design to allow creation of proxy objects.
I don't see a big reason of adding that ce->default_object_handlers in 9e6eab3
I suspect, this is done for extending observer capabilities or to allow some run-time tricks by third-party extension.
@bwoebi ?

I'm not sure about all consequences of this change.
Personally, I would revert the original patch.

[iluuu1994]

@dstogov I think the rationale was that since different object handlers for instances of the same class are rarely used (or rather never, there wasn't a known case) storing it on each object doesn't really make sense. Removing handlers would reduce the size of zend_object from 56 to 48 bytes. On the other hand, it could increase cache misses when invoking handlers due to looking up handlers in the class (but probably unlikely, given that the class is used most of the time).

[dstogov]

Removing handlers from zend_object will break ability of proxy objects and some extensions (e.g. ext/ffi).

Also as you said, calling handlers through class_entry would require additional memory load that may cause cache miss.

[bwoebi]

I suspect, this is done for extending observer capabilities or to allow some run-time tricks by third-party extension.

Yes. Both, actually. Currently, replacing of handlers requires you to get hold of some object instance including the handlers. It's sort of possible to achieve it, by instantiating an object in MINIT stage, then freeing it. But that's a terrible hack and not always without side-effects.

[dstogov]

Replacing object handers is a hack of course, and this may work improperly.

Did you try to set object->handlers through overridden zend_class_entry.create_object?
This should work without BC breaks, and prevent some unsafe optimizations and JIT-ing.

[bwoebi]

@dstogov This gets complicated very quickly, if the object handlers turn out to be dynamic (like FFI CData) (or another extension doing similar tricks and co-existing with ours). To ensure proper handling then, I'd essentially have to store the old object handlers in run-time allocated structures, stored in a hashmap addressed via the object pointer, which then needs to be freed in a free_obj handler.
This adds quite the overhead.

[dstogov]

I didn't get how this is related. Nowadays zend_class_entries are usually stored in opcache SHM. They are immutable. So changing zend_class_entry.create_object or introducing zend_class_entry->default_handlers doesn't make a lot of difference. Both may be changed/set only at MINIT. Both cases won't affect run-time assignments of object->handlers in FFI::CData.

[bwoebi]

The problem is calling the existing object handlers. Replacing is not an issue. But dynamically calling the correct existing object handler (like, a CData object should still work, just be observed) is. I.e. if I replace clone_obj, do I have to call zend_ffi_free_clone_obj or zend_ffi_cdata_clone_obj to invoke the original? It depends on what object handlers were set before...

[iluuu1994]

@dstogov Are you ok with merging this as is until we make a final decision? I'm working on fixing all the nightly issues.

[TysonAndre]

So the impact until this is fixed is that in php 8.3-dev+, any userland class or enum declarations would crash when they're loaded from file cache into memory (if opcache is enabled (e.g. opcache.enable_cli=1, opcache.enable=1) and opcache.file_cache is used)

For example, make test in PECLs would be affected in 8.3-dev+, because the run-tests.php script has class definitions that would be loaded from the file cache

I'd personally be in favor of merging this (and reverting both if the final decision is to undo the object handlers change).

---

Do we have any examples of PECLs that override object handlers outside of create_object in practice other than ffi (which seems like it can check interned data flags inside of a shared handler and conditionally execute the right handler)? Or of proxying functionality?

Even for extensions that touch the internals of php, that isn't done

• https://github.com/krakjoe/componere doesn't build in php 8.1+. For the php versions it does support, it looks like it sets obj->handlers in create_object normally
• uopz overrides opcode handlers, not object handlers
• runkit7 doesn't override object handlers at all, only method/function definitions and functions on class declarations (doesn't change handlers, which are constant memory, and doesn't change the pointer obj->handler either)

I looked at some of the commonly used pecls I downloaded years ago and didn't see anything.

I've only done that for https://pecl.php.net/package/weakreference_bc (and seen it done in the weakref PECL it was forked from, which modified the previously non-constant handlers struct instead of changing pointers) - and that's a polyfill for the standard https://www.php.net/WeakReference and https://www.php.net/WeakMap (to add another alternative to ease migration for anything previously using unmaintained third party code with dynamic properties), so it only supports php 7.4 and older by design. - I hadn't seen obj->handlers overridden elsewhere before.

---

I'd wonder if there'd be some new optimizations available to opcache or the JIT from knowing exactly what the object handlers could be for a class and that they wouldn't be changed - E.g. if opcache knew that something was an instance of a commonly used special cased final internal class, then for some classes opcache could emit assembly to read the count($obj) from the memory offset of the interned object data containing the count, instead of calling ZEND_COUNT (same for array access shorthand syntax, property access, comparisons, casts, operators, etc)

• Right now, opcache probably can't infer the handlers, since the create_object implementation is opaque

[iluuu1994]

I merged this for now so we finally have a green build. If default handlers were to be reverted this fix should be as well (it wouldn't compile with it, so it can't be forgotten either).