From 54ec50497b79fd6f14d3c8110d28f0cd19bfeec7 Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sat, 5 Feb 2022 15:28:21 -0500 Subject: [PATCH 1/3] Make var_export/debug_zval_dump first check for infinite recursion on the object This test case previously failed with an assertion error in debug builds (for updating an index of the array to set it to the same value it already had) Then repeat the existing check on the return value of `get_properties_for` in case there was a reason for doing it that way. 1. `HT_ASSERT_RC1(ht);` would fail for SplFixedArray and related datastructures. 2. 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. This 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. (GitHub wasn't updating the previous PR despite pushing the branch to the right repo earlier, creating a new branch) Closes GH-8044 --- ext/spl/tests/gh8044.phpt | 20 ++++++++++++++++++++ ext/standard/var.c | 24 ++++++++++++++++++++++++ 2 files changed, 44 insertions(+) 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..bd189ebb79ac9 --- /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..17ed0d1f49fad 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -343,11 +343,22 @@ PHPAPI void php_debug_zval_dump(zval *struc, int level) /* {{{ */ PUTS("}\n"); break; case IS_OBJECT: + /* 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); if (myht) { if (GC_IS_RECURSIVE(myht)) { PUTS("*RECURSION*\n"); zend_release_properties(myht); + Z_UNPROTECT_RECURSION_P(struc); return; } GC_PROTECT_RECURSION(myht); @@ -377,6 +388,7 @@ PHPAPI void php_debug_zval_dump(zval *struc, int level) /* {{{ */ 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,9 +564,20 @@ PHPAPI void php_var_export_ex(zval *struc, int level, smart_str *buf) /* {{{ */ break; case IS_OBJECT: + /* 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 (myht) { if (GC_IS_RECURSIVE(myht)) { + Z_UNPROTECT_RECURSION_P(struc); smart_str_appendl(buf, "NULL", 4); zend_error(E_WARNING, "var_export does not handle circular references"); zend_release_properties(myht); @@ -596,6 +619,7 @@ PHPAPI void php_var_export_ex(zval *struc, int level, smart_str *buf) /* {{{ */ 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); } From e2b96e2eaf994201b0336d50e4e01a407b0b2c1f Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sat, 27 Aug 2022 15:48:15 -0400 Subject: [PATCH 2/3] Add backslash to test expectation for var_export change --- ext/spl/tests/gh8044.phpt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/spl/tests/gh8044.phpt b/ext/spl/tests/gh8044.phpt index bd189ebb79ac9..3567646954634 100644 --- a/ext/spl/tests/gh8044.phpt +++ b/ext/spl/tests/gh8044.phpt @@ -11,7 +11,7 @@ call_user_func(function () { ?> --EXPECTF-- Warning: var_export does not handle circular references in %s on line 5 -SplFixedArray::__set_state(array( +\SplFixedArray::__set_state(array( 0 => NULL, )) object(SplFixedArray)#2 (1) refcount(4){ From a9302fc1937eb59f4f5905432bac654e7edd3baf Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sun, 28 Aug 2022 08:03:49 -0400 Subject: [PATCH 3/3] Remove recursion check on object **properties** The previous commit checked for recursion on the object itself. Normally, different objects shouldn't share hash tables. --- ext/standard/var.c | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/ext/standard/var.c b/ext/standard/var.c index 17ed0d1f49fad..268c535a80242 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -354,15 +354,6 @@ PHPAPI void php_debug_zval_dump(zval *struc, int level) /* {{{ */ Z_PROTECT_RECURSION_P(struc); myht = zend_get_properties_for(struc, ZEND_PROP_PURPOSE_DEBUG); - if (myht) { - if (GC_IS_RECURSIVE(myht)) { - PUTS("*RECURSION*\n"); - zend_release_properties(myht); - Z_UNPROTECT_RECURSION_P(struc); - return; - } - GC_PROTECT_RECURSION(myht); - } 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); @@ -381,7 +372,6 @@ 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) { @@ -575,17 +565,6 @@ PHPAPI void php_var_export_ex(zval *struc, int level, smart_str *buf) /* {{{ */ } Z_PROTECT_RECURSION_P(struc); myht = zend_get_properties_for(struc, ZEND_PROP_PURPOSE_VAR_EXPORT); - if (myht) { - if (GC_IS_RECURSIVE(myht)) { - Z_UNPROTECT_RECURSION_P(struc); - 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); - } - } if (level > 1) { smart_str_appendc(buf, '\n'); buffer_append_spaces(buf, level - 1); @@ -616,7 +595,6 @@ 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);