Implement GH-9826: Make class_alias() work with internal classes

[nielsdos]

Credits to Nikita because he basically described how to implement this in the issue comment.

We can't increase the refcount of internal classes during request time.
To work around this problem we simply don't refcount aliases anymore and
add a check in the destruction to skip aliases entirely.
There were also some checks which checked for an alias implicitly by
comparing the refcount, these have been replaced by checking the type of
the zval instead.

[iluuu1994]

I haven't analyzed this in detail so my suggestion might not make sense.

Reading Nikitas comment it sounds like he suggested not refcounting aliases at all. Class aliases seem to be the only place where refcount is currently incremented for classes so I'm not sure if refcounting would even have a purpose anymore at that point.

init_executor sets EG(persistent_classes_count) and shutdown_executor will destroy all classes that were allocated after that point. Since we can't persistently alias a request-allocated class for obvious reasons it doesn't seem like refcounting is necessary. We'll either:

• persistently alias a persistent class
• temporarily (per request) alias a persistent class
• temporarily (per request) alias a temporary (per request) class

None of those can cause a use-after-free (if we skip IS_ALIAS_PTR during class destruction, that is). Thus, we might be able to avoid IS_INTERNAL_CLASS_REQUEST_ALIAS_PTR and just skip refcounting for all IS_ALIAS_PTR.

[iluuu1994]

I also noticed that there are a couple of if (ce->refcount > 1 && !zend_string_equals_ci(key, ce->name)) { checks, particularly opcache_get_status, zend_foreach_op_array and zend_optimize_script. If we skipped refcounting for all aliases we'd probably need to remove the ce->refcount > 1 check.

[nielsdos]

Right, your suggestion makes sense, quite a bit more sense than my current solution :p
Thanks for checking.
Regarding the if (ce->refcount > 1 && !zend_string_equals_ci(key, ce->name)) { check, I guess the ce->refcount > 1 is a performance optimisation to skip comparing the class names in case it cannot be aliased at all (due to its refcount). We can instead of doing if (ce->refcount > 1 && !zend_string_equals_ci(key, ce->name)) { just check the type to be a class alias I guess?

[iluuu1994]

We can instead of doing if (ce->refcount > 1 && !zend_string_equals_ci(key, ce->name)) { just check the type to be a class alias I guess?

Yeah, that makes way more sense than what I suggested 🙂

[nielsdos]

Pushed the changes with your suggestion applied.

[iluuu1994]

Thank you! Apart from this small optimization this looks good to me.

[iluuu1994]

@nielsdos I checked the failure locally, it looks like zend_class_add_ref needs adjustments (it's used as a copy constructor).

--- a/Zend/zend_opcode.c
+++ b/Zend/zend_opcode.c
@@ -524,7 +524,7 @@ void zend_class_add_ref(zval *zv)
 {
 	zend_class_entry *ce = Z_PTR_P(zv);
 
-	if (!(ce->ce_flags & ZEND_ACC_IMMUTABLE)) {
+	if (Z_TYPE_P(zv) != IS_ALIAS_PTR && !(ce->ce_flags & ZEND_ACC_IMMUTABLE)) {
 		ce->refcount++;
 	}
 }

[nielsdos]

Thank you! I fixed it. Feel free to add your Co-authored-by tag btw if you end up merging this, because you helped out quite alot with this PR :)

[nielsdos]

Something very weird went wrong with the CI... It claims there was a compile error on x32:

4850 | jit_globals_id = ts_allocate_id(&jit_globals_id, sizeof(zend_jit_globals), (ts_allocate_ctor) zend_jit_globals_ctor, zend_jit_globals_dtor);
| ^~~~~~~~~~~~~~~~~~~~~
| |
| void (*)(zend_jit_globals ) {aka void ()(struct _zend_jit_globals *)}

But that code doesn't exist in this branch?! Seems like caching gone wrong or something like that?
The x64 failure is due to apt failing to install something and the windows build randomly crashed (but that could be related to the unexisting code from x32).

[iluuu1994]

Hmm we merged ccache for CI recently. Maybe that's related. I'll check later.

[iluuu1994]

@nielsdos This was unrelated to ccache but fixed. Could you rebase this onto master when you have time? Thank you!

[nielsdos]

Alright, rebased. Let's see if the tests pass now :)

[iluuu1994]

@dstogov Does this look reasonable to you?

From what I can see, zend_class_entry.refcount was previously just used to skip class destruction for aliases, and as an optimization to skip the alias check (comparing class names) for classes that aren't aliased. Both of those go away with this PR. We could remove refcount altogether now, as it's never increased anymore. Does that sound ok to you?

[dstogov]

@dstogov Does this look reasonable to you?

This looks fine. I'm only not sure in opcache_get_status chage.

From what I can see, zend_class_entry.refcount was previously just used to skip class destruction for aliases

It's still used in zend_class_add_ref() at least in ZTS build.
You may try to eliminate it completely, but better do it in a separate PR on top of this.

[nielsdos]

Applied the requested changes, thanks.