Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assertion failure in Zend/zend_execute.c #15914

Open
YuanchengJiang opened this issue Sep 16, 2024 · 6 comments
Open

Assertion failure in Zend/zend_execute.c #15914

YuanchengJiang opened this issue Sep 16, 2024 · 6 comments

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
class TestHooks
{
public function __construct(string $first, public string $last) {
}
}
$o = new TestHooks('first', 'last');
$a = new ArrayObject($o);
foreach ($a as &$c) {
}

Resulted in this output:

/php-src/Zend/zend_execute.c:3955: zend_ref_del_type_source: Assertion `source_list->ptr == prop' failed.
Aborted (core dumped)

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

@cmb69
Copy link
Member

cmb69 commented Sep 16, 2024

Possible fix:

 Zend/zend_objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Zend/zend_objects.c b/Zend/zend_objects.c
index 348b3f3416..61dc629684 100644
--- a/Zend/zend_objects.c
+++ b/Zend/zend_objects.c
@@ -66,7 +66,7 @@ void zend_object_dtor_property(zend_object *object, zval *p)
 		if (UNEXPECTED(Z_ISREF_P(p)) &&
 				(ZEND_DEBUG || ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(p)))) {
 			zend_property_info *prop_info = zend_get_property_info_for_slot(object, p);
-			if (ZEND_TYPE_IS_SET(prop_info->type)) {
+			if (!ZEND_TYPE_IS_ONLY_MASK(prop_info->type)) {
 				ZEND_REF_DEL_TYPE_SOURCE(Z_REF_P(p), prop_info);
 			}
 		}

@arnaud-lb, what do you think?


PS: that patch doesn't work. It breaks Zend\tests\anon\005.phpt and likely more.

@iluuu1994
Copy link
Member

@cmb69 From my analysis in #15775 (comment), this was actually caused by a different issue. The ArrayObject iterator is currently broken for typed references. Let me see if I can find the patch I had.

@iluuu1994
Copy link
Member

Something like this should do.

diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
index dbe110f7c4..9ced01547e 100644
--- a/ext/spl/spl_array.c
+++ b/ext/spl/spl_array.c
@@ -100,6 +100,15 @@ static inline HashTable *spl_array_get_hash_table(spl_array_object* intern) { /*
 }
 /* }}} */
 
+static inline spl_array_object *spl_array_object_get_root(spl_array_object *intern)
+{
+	while ((intern->ar_flags & SPL_ARRAY_USE_OTHER)
+	 && intern != Z_SPLARRAY_P(&intern->array)) {
+		intern = Z_SPLARRAY_P(&intern->array);
+	}
+	return intern;
+}
+
 static inline bool spl_array_is_object(spl_array_object *intern) /* {{{ */
 {
 	while (intern->ar_flags & SPL_ARRAY_USE_OTHER) {
@@ -974,7 +983,8 @@ static zval *spl_array_it_get_current_data(zend_object_iterator *iter) /* {{{ */
 {
 	spl_array_iterator *array_iter = (spl_array_iterator*)iter;
 	spl_array_object *object = Z_SPLARRAY_P(&iter->data);
-	HashTable *aht = spl_array_get_hash_table(object);
+	spl_array_object *root_object = spl_array_object_get_root(object);
+	HashTable *aht = spl_array_get_hash_table(root_object);
 	zval *data = zend_hash_get_current_data_ex(aht, spl_array_get_pos_ptr(aht, object));
 	if (data && Z_TYPE_P(data) == IS_INDIRECT) {
 		data = Z_INDIRECT_P(data);
@@ -983,11 +993,11 @@ static zval *spl_array_it_get_current_data(zend_object_iterator *iter) /* {{{ */
 	// Typed properties must add a type source to the reference, and readonly properties must fail.
 	if (array_iter->by_ref
 	 && Z_TYPE_P(data) != IS_REFERENCE
-	 && Z_TYPE(object->array) == IS_OBJECT
-	 && !(object->ar_flags & (SPL_ARRAY_IS_SELF|SPL_ARRAY_USE_OTHER))) {
+	 && Z_TYPE(root_object->array) == IS_OBJECT
+	 && !(root_object->ar_flags & (SPL_ARRAY_IS_SELF|SPL_ARRAY_USE_OTHER))) {
 		zend_string *key;
 		zend_hash_get_current_key_ex(aht, &key, NULL, spl_array_get_pos_ptr(aht, object));
-		zend_class_entry *ce = Z_OBJCE(object->array);
+		zend_class_entry *ce = Z_OBJCE(root_object->array);
 		zend_property_info *prop_info = zend_get_property_info(ce, key, true);
 		ZEND_ASSERT(prop_info != ZEND_WRONG_PROPERTY_INFO);
 		if (EXPECTED(prop_info != NULL) && ZEND_TYPE_IS_SET(prop_info->type)) {
diff --git a/ext/spl/tests/gh15914.phpt b/ext/spl/tests/gh15914.phpt
new file mode 100644
index 0000000000..019b52e75f
--- /dev/null
+++ b/ext/spl/tests/gh15914.phpt
@@ -0,0 +1,23 @@
+--TEST--
+GH-15914: Assertion error when iterating ArrayObject of object with typed props by-ref
+--FILE--
+<?php
+
+class C
+{
+    public function __construct(string $param, public string $prop) {}
+}
+
+$c = new C('first', 'last');
+$a = new ArrayObject($c);
+foreach ($a as &$prop) {
+    $prop .= ' appended';
+}
+var_dump($c);
+
+?>
+--EXPECT--
+object(C)#1 (1) {
+  ["prop"]=>
+  &string(13) "last appended"
+}

@cmb69
Copy link
Member

cmb69 commented Sep 17, 2024

@iluuu1994, indeed, that is again an ArrayObject issue. Not sure if we should fix that. As often, some real code may actually rely on that broken behavior …

@iluuu1994
Copy link
Member

🤷‍♂️ It seems like you could easily run into segfaults here anyway. I don't really care either way, I'm happy soft-deprecating ArrayObject for now. It would be good to know what it is actually useful for, to see if we need to offer an alternative.

@cmb69
Copy link
Member

cmb69 commented Sep 17, 2024

@iluuu1994, if we start to (soft-)deprecate ArrayObject, I consider it quite likely that we will be informed about its use-cases. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants