PHP crashes in _zend_observe_fcall_begin() on internal method inherited by user child when run with opcache

[dstogov]

Description

The following code crashes when run with observer and opcache enabled (php -d opcache.enable=1 -d opcache.jit=0 -d zend_test.observer.enabled=1 -d zend_test.observer.observe_all=1 -d zend_test.observer.show_return_value=1 bug.php)

<?php
class MyReflectionProperty extends ReflectionProperty {
}

class A {
    protected $protected = 'a';
}

$property = new MyReflectionProperty('A', 'protected');
$property->setAccessible(true);
?>

Resulted in this output:

SIGSEGV
    #0 0x2502451 in _zend_observe_fcall_begin php8.2/Zend/zend_observer.c:227
    #1 0x2502858 in zend_observer_fcall_begin php8.2/Zend/zend_observer.c:257
    #2 0x2140129 in ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER php8.2/Zend/zend_vm_execute.h:2058
    #3 0x23c4531 in execute_ex php8.2/Zend/zend_vm_execute.h:56056
    #4 0x23dfb1a in zend_execute php8.2/Zend/zend_vm_execute.h:60380

But I expected this output instead:

<!-- init '%sbug.php' -->
<file '%sbug.php'>
  <!-- init ReflectionProperty::__construct() -->
  <ReflectionProperty::__construct>
  </ReflectionProperty::__construct:NULL>
  <!-- init ReflectionProperty::setAccessible() -->
  <ReflectionProperty::setAccessible>
  </ReflectionProperty::setAccessible:NULL>
</file '%sbug.php'>

The crash occurs because the recent engine changes related to observation of internal functions don't respect opcache.
Internal method inherited from internal class is stored in opcache SHM and corresponding map_ptr slot for run_time_cache is allocated. However, run_time_cache itself is not allocated. It should be allocated on each request.

PHP Version

PHP 8.2.0-dev

Operating System

[dstogov]

@bwoebi please review the possible fix. At first, it fixes run_tine_cache initialization for internal methods in user children classes. At second, it fixes zend_ineritance.c to call zend_observer_class_linked_notify after the linking. I'm not sure if this fix fits in observer API design.

I hate this over-designed subsystem and especially the related core engine changes that you committed.

diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c
index 3eaaca3409..2d99709757 100644
--- a/Zend/zend_inheritance.c
+++ b/Zend/zend_inheritance.c
@@ -3135,13 +3135,11 @@ static zend_always_inline bool register_early_bound_ce(zval *delayed_early_bindi
 		if (EXPECTED(!(ce->ce_flags & ZEND_ACC_PRELOADED))) {
 			if (zend_hash_set_bucket_key(EG(class_table), (Bucket *)delayed_early_binding, lcname) != NULL) {
 				Z_CE_P(delayed_early_binding) = ce;
-				zend_observer_class_linked_notify(ce, lcname);
 				return true;
 			}
 		} else {
 			/* If preloading is used, don't replace the existing bucket, add a new one. */
 			if (zend_hash_add_ptr(EG(class_table), lcname, ce) != NULL) {
-				zend_observer_class_linked_notify(ce, lcname);
 				return true;
 			}
 		}
@@ -3149,7 +3147,6 @@ static zend_always_inline bool register_early_bound_ce(zval *delayed_early_bindi
 		return false;
 	}
 	if (zend_hash_add_ptr(CG(class_table), lcname, ce) != NULL) {
-		zend_observer_class_linked_notify(ce, lcname);
 		return true;
 	}
 	return false;
@@ -3170,6 +3167,7 @@ ZEND_API zend_class_entry *zend_try_early_bind(zend_class_entry *ce, zend_class_
 				if (UNEXPECTED(!register_early_bound_ce(delayed_early_binding, lcname, ret))) {
 					return NULL;
 				}
+				zend_observer_class_linked_notify(ret, lcname);
 				return ret;
 			}
 		} else {
@@ -3244,6 +3242,7 @@ ZEND_API zend_class_entry *zend_try_early_bind(zend_class_entry *ce, zend_class_
 		if (ZSTR_HAS_CE_CACHE(ce->name)) {
 			ZSTR_SET_CE_CACHE(ce->name, ce);
 		}
+		zend_observer_class_linked_notify(ce, lcname);
 
 		return ce;
 	}
diff --git a/Zend/zend_observer.c b/Zend/zend_observer.c
index f699ea714d..7de871c97d 100644
--- a/Zend/zend_observer.c
+++ b/Zend/zend_observer.c
@@ -319,12 +319,38 @@ ZEND_API void zend_observer_class_linked_register(zend_observer_class_linked_cb
 	zend_llist_add_element(&zend_observer_class_linked_callbacks, &cb);
 }
 
+static bool may_have_internal_methods(const zend_class_entry *ce)
+{
+	zend_class_entry *parent = ce->parent;
+
+	while (parent) {
+		if (parent->type == ZEND_INTERNAL_CLASS) {
+			return zend_hash_num_elements(&parent->function_table) > 0;
+		}
+		parent = parent->parent;
+	}
+	return false;
+}
+
 ZEND_API void ZEND_FASTCALL _zend_observer_class_linked_notify(zend_class_entry *ce, zend_string *name)
 {
 	if (CG(compiler_options) & ZEND_COMPILE_IGNORE_OBSERVER) {
 		return;
 	}
 
+	if (ce->type == ZEND_USER_CLASS
+	 && (ce->ce_flags & ZEND_ACC_CACHED)
+	 && may_have_internal_methods(ce)) {
+		zend_function *fn;
+
+		ZEND_HASH_MAP_FOREACH_PTR(&ce->function_table, fn) {
+			if (fn->type == ZEND_INTERNAL_FUNCTION) {
+				char *ptr = zend_arena_calloc(&CG(arena), zend_op_array_extension_handles, sizeof(void*));
+				ZEND_MAP_PTR_SET(fn->common.run_time_cache, (void *)ptr);
+			}
+		} ZEND_HASH_FOREACH_END();
+	}
+
 	for (zend_llist_element *element = zend_observer_class_linked_callbacks.head; element; element = element->next) {
 		zend_observer_class_linked_cb callback = *(zend_observer_class_linked_cb *) (element->data);
 		callback(ce, name);

[bwoebi]

I like the move of zend_observer_class_linked_notify() to the end of zend_try_early_bind(); the class is actually only fully linked by that point. Thanks for spotting.

I'm afraid though, with the current design you cannot do that after the ZEND_COMPILE_IGNORE_OBSERVER check, as it is not only responsible for observers, but any extension should be able to rely on an initialized run_time_cache. Can only be skipped if zend_op_array_extension_handles == 0.

[dstogov]

@bwoebi please adopt and commit the patch yourself. I had to take a look into this bug, because we needed a quick fix.

[bwoebi]

Will do now.

[dstogov]

Another possible fix for the same crash

diff --git a/Zend/zend_observer.c b/Zend/zend_observer.c
index f699ea714d..523258ac8a 100644
--- a/Zend/zend_observer.c
+++ b/Zend/zend_observer.c
@@ -224,6 +224,12 @@ static void ZEND_FASTCALL _zend_observe_fcall_begin(zend_execute_data *execute_d
 	}
 
 	zend_observer_fcall_begin_handler *handler = (zend_observer_fcall_begin_handler *)&ZEND_OBSERVER_DATA(function);
+	if (UNEXPECTED(!handler)) {
+		ZEND_ASSERT(function->type == ZEND_INTERNAL_FUNCTION);
+		char *ptr = zend_arena_calloc(&CG(arena), zend_op_array_extension_handles, sizeof(void*));
+		ZEND_MAP_PTR_SET(function->common.run_time_cache, (void *)ptr);
+		handler = (zend_observer_fcall_begin_handler *)&ZEND_OBSERVER_DATA(function);
+	}
 	if (!*handler) {
 		zend_observer_fcall_install(execute_data);
 	}

[bwoebi]

I've had a deeper look at the root cause - this is a regression caused by 5e9654b. Internal function copies are supposed to share their run_time_cache, but dynamically created internal functions in user classes like enum functions are not.
The proper fix is to not re-assign the run_time_cache map_ptr for copied functions.
That way it will use the run_time_cache already allocated within zend_init_internal_run_time_cache, and by extension mirror the behaviour with userland functions as well as with and without opcache: there is only one run_time_cache for a specific original function.

I'll push a fix shortly.