From de4f7df1ed5d0a21786e8da4f79ba1c672dc32d4 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 8 Feb 2025 12:44:58 +0100 Subject: [PATCH 1/4] Fix GH-17736: Assertion failure zend_reference_destroy() The cache slot for FETCH_OBJ_W in function `test` is primed with the class for C. The next call uses a simplexml instance and reuses the same cache slot. simplexml's get_property_ptr handler does not use the cache slot, so the old values remain in the cache slot. When `zend_handle_fetch_obj_flags` is called this is not guarded by a check for the class entry. So we end up using the prop_info from the property C::$a instead of the simplexml property. This patch adds a check for the class entry. I placed the check as late as possible to avoid as much overhead as possible. An alternative solution is to write NULLs to the cache slot in the get_property_ptr handlers of extensions that don't use the cache slot, but that is not general: not only simplexml would need changes, maybe even third party extensions would need changes as well. --- Zend/zend_execute.c | 2 +- ext/simplexml/tests/gh17736.phpt | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 ext/simplexml/tests/gh17736.phpt diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index dc28578f2c1a3..8abd21b1f42c7 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -3267,7 +3267,7 @@ static zend_always_inline void zend_fetch_property_address(zval *result, zval *c if (prop_op_type == IS_CONST) { prop_info = CACHED_PTR_EX(cache_slot + 2); - if (prop_info) { + if (prop_info && EXPECTED(zobj->ce == CACHED_PTR_EX(cache_slot))) { if (UNEXPECTED(!zend_handle_fetch_obj_flags(result, ptr, NULL, prop_info, flags))) { goto end; } diff --git a/ext/simplexml/tests/gh17736.phpt b/ext/simplexml/tests/gh17736.phpt new file mode 100644 index 0000000000000..6f4d60f66f5d8 --- /dev/null +++ b/ext/simplexml/tests/gh17736.phpt @@ -0,0 +1,20 @@ +--TEST-- +GH-17736 (Assertion failure zend_reference_destroy()) +--EXTENSIONS-- +simplexml +--FILE-- +'); +class C { + public int $a = 1; +} +function test($obj) { + $ref =& $obj->a; +} +$obj = new C; +test($obj); +test($o1); +echo "Done\n"; +?> +--EXPECT-- +Done From 81f1465d75663fc76fe6a7432cd1423709fcd80d Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 8 Feb 2025 17:23:31 +0100 Subject: [PATCH 2/4] Also fix JIT --- ext/opcache/jit/zend_jit_helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/opcache/jit/zend_jit_helpers.c b/ext/opcache/jit/zend_jit_helpers.c index a79f2b5173d53..f68520cec7c9f 100644 --- a/ext/opcache/jit/zend_jit_helpers.c +++ b/ext/opcache/jit/zend_jit_helpers.c @@ -2084,7 +2084,7 @@ static void ZEND_FASTCALL zend_jit_fetch_obj_w_slow(zend_object *zobj) if (opline->op2_type == IS_CONST) { prop_info = CACHED_PTR_EX(cache_slot + 2); - if (!prop_info) { + if (!prop_info || !EXPECTED(zobj->ce == CACHED_PTR_EX(cache_slot))) { break; } } From bf858a849c0263ca38e3f1d9758712b77b44f7f3 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 23 Feb 2025 18:25:07 +0100 Subject: [PATCH 3/4] Clear after first failed check --- Zend/zend_execute.c | 5 ++++- ext/opcache/jit/zend_jit_helpers.c | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 8abd21b1f42c7..99da3a4a051f3 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -3232,6 +3232,9 @@ static zend_always_inline void zend_fetch_property_address(zval *result, zval *c return; } } + } else if (prop_op_type == IS_CONST) { + /* CE mismatch, make cache slot consistent */ + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; } /* Pointer on property callback is required */ @@ -3267,7 +3270,7 @@ static zend_always_inline void zend_fetch_property_address(zval *result, zval *c if (prop_op_type == IS_CONST) { prop_info = CACHED_PTR_EX(cache_slot + 2); - if (prop_info && EXPECTED(zobj->ce == CACHED_PTR_EX(cache_slot))) { + if (prop_info) { if (UNEXPECTED(!zend_handle_fetch_obj_flags(result, ptr, NULL, prop_info, flags))) { goto end; } diff --git a/ext/opcache/jit/zend_jit_helpers.c b/ext/opcache/jit/zend_jit_helpers.c index f68520cec7c9f..a79f2b5173d53 100644 --- a/ext/opcache/jit/zend_jit_helpers.c +++ b/ext/opcache/jit/zend_jit_helpers.c @@ -2084,7 +2084,7 @@ static void ZEND_FASTCALL zend_jit_fetch_obj_w_slow(zend_object *zobj) if (opline->op2_type == IS_CONST) { prop_info = CACHED_PTR_EX(cache_slot + 2); - if (!prop_info || !EXPECTED(zobj->ce == CACHED_PTR_EX(cache_slot))) { + if (!prop_info) { break; } } From d26a678fbc67cb669347f7f47b471c59a05a6a6f Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 23 Feb 2025 19:18:18 +0100 Subject: [PATCH 4/4] Clear cache slots --- ext/date/php_date.c | 1 + ext/dom/php_dom.c | 1 + ext/pdo/pdo_stmt.c | 1 + ext/simplexml/simplexml.c | 2 ++ ext/simplexml/tests/gh17736.phpt | 2 +- ext/snmp/snmp.c | 1 + ext/spl/spl_array.c | 2 ++ ext/xmlreader/php_xmlreader.c | 2 ++ ext/zip/php_zip.c | 2 ++ 9 files changed, 13 insertions(+), 1 deletion(-) diff --git a/ext/date/php_date.c b/ext/date/php_date.c index 436af32f93536..6f7796eec1250 100644 --- a/ext/date/php_date.c +++ b/ext/date/php_date.c @@ -4399,6 +4399,7 @@ static zval *date_interval_get_property_ptr_ptr(zend_object *object, zend_string zend_string_equals_literal(name, "days") || zend_string_equals_literal(name, "invert") ) { /* Fallback to read_property. */ + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; ret = NULL; } else { ret = zend_std_get_property_ptr_ptr(object, name, type, cache_slot); diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index 7ec107dd712e3..1da53bae64b51 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -303,6 +303,7 @@ static zval *dom_get_property_ptr_ptr(zend_object *object, zend_string *name, in return zend_std_get_property_ptr_ptr(object, name, type, cache_slot); } + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; return NULL; } diff --git a/ext/pdo/pdo_stmt.c b/ext/pdo/pdo_stmt.c index 3998b64ab7e13..6aec99026232b 100644 --- a/ext/pdo/pdo_stmt.c +++ b/ext/pdo/pdo_stmt.c @@ -2495,6 +2495,7 @@ static zval *pdo_row_get_property_ptr_ptr(zend_object *object, zend_string *name ZEND_IGNORE_VALUE(type); ZEND_IGNORE_VALUE(cache_slot); + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; return NULL; } diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index 18bfa31271e19..46ec3a2a788ab 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -635,6 +635,8 @@ static zval *sxe_property_get_adr(zend_object *object, zend_string *zname, int f SXE_ITER type; zval member; + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; + sxe = php_sxe_fetch_object(object); GET_NODE(sxe, node); if (UNEXPECTED(!node)) { diff --git a/ext/simplexml/tests/gh17736.phpt b/ext/simplexml/tests/gh17736.phpt index 6f4d60f66f5d8..78561f6ab0293 100644 --- a/ext/simplexml/tests/gh17736.phpt +++ b/ext/simplexml/tests/gh17736.phpt @@ -4,7 +4,7 @@ GH-17736 (Assertion failure zend_reference_destroy()) simplexml --FILE-- '); +$o1 = new SimpleXMLElement(''); class C { public int $a = 1; } diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index b5bb9f91745c6..02348a5fbca00 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -1861,6 +1861,7 @@ static zval *php_snmp_get_property_ptr_ptr(zend_object *object, zend_string *nam return zend_std_get_property_ptr_ptr(object, name, type, cache_slot); } + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; return NULL; } diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c index 10407bee6a1a8..bd03a8aae0189 100644 --- a/ext/spl/spl_array.c +++ b/ext/spl/spl_array.c @@ -844,6 +844,8 @@ static zval *spl_array_get_property_ptr_ptr(zend_object *object, zend_string *na if ((intern->ar_flags & SPL_ARRAY_ARRAY_AS_PROPS) != 0 && !zend_std_has_property(object, name, ZEND_PROPERTY_EXISTS, NULL)) { + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; + /* If object has offsetGet() overridden, then fallback to read_property, * which will call offsetGet(). */ zval member; diff --git a/ext/xmlreader/php_xmlreader.c b/ext/xmlreader/php_xmlreader.c index ba540692804ef..569058e91a454 100644 --- a/ext/xmlreader/php_xmlreader.c +++ b/ext/xmlreader/php_xmlreader.c @@ -121,6 +121,8 @@ zval *xmlreader_get_property_ptr_ptr(zend_object *object, zend_string *name, int zval *retval = NULL; xmlreader_prop_handler *hnd = NULL; + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; + obj = php_xmlreader_fetch_object(object); if (obj->prop_handler != NULL) { diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 54bdffbecb03b..e28cf688dcff8 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -890,6 +890,8 @@ static zval *php_zip_get_property_ptr_ptr(zend_object *object, zend_string *name zval *retval = NULL; zip_prop_handler *hnd = NULL; + cache_slot[0] = cache_slot[1] = cache_slot[2] = NULL; + obj = php_zip_fetch_object(object); if (obj->prop_handler != NULL) {