From 19c95aca1a54f2d649bb2e1ab359c911e3da6384 Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sat, 5 Feb 2022 15:28:21 -0500 Subject: [PATCH] Make var_export/debug_zval_dump check for infinite recursion on the *object* Switch the recursion check from the result of `get_properties_for` (the returned hash table of properties) to just checking for infinite recursion on the object. - In order for a native datastructure to correctly implement `*get_properties_for` for var_export's cycle detection, it would need to return the exact same array every time prior to this PR. Prior to this commit, the requirements for cycle detection would prevent SplFixedArray or similar classes from returning a temporary array that: 1. Wouldn't be affected by unexpected mutations from error handlers 2. Could be garbage collected instead. Note that SplFixedArray continues to need to return `object->properties` ~~until php 9.0, when dynamic properties are forbidden.~~ (even after php 9.0, because `#[\AllowDynamicProperties]` can be used in subclasses?) --- ext/spl/tests/gh8044.phpt | 20 +++++++++++++++++++ ext/standard/var.c | 42 ++++++++++++++++++++------------------- 2 files changed, 42 insertions(+), 20 deletions(-) create mode 100644 ext/spl/tests/gh8044.phpt diff --git a/ext/spl/tests/gh8044.phpt b/ext/spl/tests/gh8044.phpt new file mode 100644 index 0000000000000..3567646954634 --- /dev/null +++ b/ext/spl/tests/gh8044.phpt @@ -0,0 +1,20 @@ +--TEST-- +Bug GH-8044 (var_export/debug_zval_dump HT_ASSERT_RC1 debug failure for SplFixedArray) +--FILE-- + +--EXPECTF-- +Warning: var_export does not handle circular references in %s on line 5 +\SplFixedArray::__set_state(array( + 0 => NULL, +)) +object(SplFixedArray)#2 (1) refcount(4){ + [0]=> + *RECURSION* +} diff --git a/ext/standard/var.c b/ext/standard/var.c index 9421a5bb74dce..268c535a80242 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -343,15 +343,17 @@ PHPAPI void php_debug_zval_dump(zval *struc, int level) /* {{{ */ PUTS("}\n"); break; case IS_OBJECT: - myht = zend_get_properties_for(struc, ZEND_PROP_PURPOSE_DEBUG); - if (myht) { - if (GC_IS_RECURSIVE(myht)) { - PUTS("*RECURSION*\n"); - zend_release_properties(myht); - return; - } - GC_PROTECT_RECURSION(myht); + /* Check if this is already recursing on the object before calling zend_get_properties_for, + * to allow infinite recursion detection to work even if classes return temporary arrays, + * and to avoid the need to update the properties table in place to reflect the state + * if the result won't be used. (https://github.com/php/php-src/issues/8044) */ + if (Z_IS_RECURSIVE_P(struc)) { + PUTS("*RECURSION*\n"); + return; } + Z_PROTECT_RECURSION_P(struc); + + myht = zend_get_properties_for(struc, ZEND_PROP_PURPOSE_DEBUG); class_name = Z_OBJ_HANDLER_P(struc, get_class_name)(Z_OBJ_P(struc)); php_printf("object(%s)#%d (%d) refcount(%u){\n", ZSTR_VAL(class_name), Z_OBJ_HANDLE_P(struc), myht ? zend_array_count(myht) : 0, Z_REFCOUNT_P(struc)); zend_string_release_ex(class_name, 0); @@ -370,13 +372,13 @@ PHPAPI void php_debug_zval_dump(zval *struc, int level) /* {{{ */ zval_object_property_dump(prop_info, val, index, key, level); } } ZEND_HASH_FOREACH_END(); - GC_UNPROTECT_RECURSION(myht); zend_release_properties(myht); } if (level > 1) { php_printf("%*c", level - 1, ' '); } PUTS("}\n"); + Z_UNPROTECT_RECURSION_P(struc); break; case IS_RESOURCE: { const char *type_name = zend_rsrc_list_get_rsrc_type(Z_RES_P(struc)); @@ -552,17 +554,17 @@ PHPAPI void php_var_export_ex(zval *struc, int level, smart_str *buf) /* {{{ */ break; case IS_OBJECT: - myht = zend_get_properties_for(struc, ZEND_PROP_PURPOSE_VAR_EXPORT); - if (myht) { - if (GC_IS_RECURSIVE(myht)) { - smart_str_appendl(buf, "NULL", 4); - zend_error(E_WARNING, "var_export does not handle circular references"); - zend_release_properties(myht); - return; - } else { - GC_TRY_PROTECT_RECURSION(myht); - } + /* Check if this is already recursing on the object before calling zend_get_properties_for, + * to allow infinite recursion detection to work even if classes return temporary arrays, + * and to avoid the need to update the properties table in place to reflect the state + * if the result won't be used. (https://github.com/php/php-src/issues/8044) */ + if (Z_IS_RECURSIVE_P(struc)) { + smart_str_appendl(buf, "NULL", 4); + zend_error(E_WARNING, "var_export does not handle circular references"); + return; } + Z_PROTECT_RECURSION_P(struc); + myht = zend_get_properties_for(struc, ZEND_PROP_PURPOSE_VAR_EXPORT); if (level > 1) { smart_str_appendc(buf, '\n'); buffer_append_spaces(buf, level - 1); @@ -593,9 +595,9 @@ PHPAPI void php_var_export_ex(zval *struc, int level, smart_str *buf) /* {{{ */ php_object_element_export(val, index, key, level, buf); } ZEND_HASH_FOREACH_END(); } - GC_TRY_UNPROTECT_RECURSION(myht); zend_release_properties(myht); } + Z_UNPROTECT_RECURSION_P(struc); if (level > 1 && !is_enum) { buffer_append_spaces(buf, level - 1); }