[php-src] Issue #8079: Recent patch to SplFixedArray causes array_walk + json encode to hang

Issue: https://github.com/php/php-src/issues/8079 Comment Author: TysonAndre Thanks, I'll continue looking at this tomorrow to see if I have a better test case for the latest patches in 8.2. > We have infinity recursion because of recursive data and incompatibility of recursion > protection mechanism with SplFixedArray. I'd suggested updating the infinite recursion to first check for infinite recursion on the object before checking for infinite recursion on the properties table because of that. I'm not very concerned about SplFixedArray and the original bug specifically due to use cases being rare, these are mostly to illustrate the point. but I do want to know how it'd be solved in the general case if var_export is to be readable for new or PECL data structures (instead of just returning an empty array making it look like an empty data structures, like SplDoublyLinkedList) - avoiding mutating obj->properties after creating it (except maybe for clearing) seems like it may be safer to me, and also overriding get_properties_for for debug/json/serialize/etc. ----- I'm still looking into whether this is only mitigated because performance improvements in 8.1 make all the examples I've tried avoid incrementing the hash table reference count to 2 and leaking/improperly reference counting the original. 1. gc during iteration may cause problems if there are side effects while properties table reference count is > 1 2. Maybe __sleep in SplFixedArray subclasses looks like zend_std_get_properties_for increments it, then php_var_serialize_get_sleep_props decrements it. And in fa14eedbeaf07129b6875fa723677a6702e15926 Optimized object serialization without rebulding properties HashTable for mitigations. ```c if (UNEXPECTED(ht)) { GC_ADDREF(ht); ``` ----- Looking at what remains that would possibly add a reference to hash tables for SplFixedArray: Zend/zend_gc.c has code such as this throughout for get_gc. So I'm concerned that if json_encode and so on are called (e.g. due to __destruct, even in normal applications not deliberately trying to trigger this), the calls to Z_OBJPROP_P will now leak HashTable instances due to this change, and possibly prematurely free the original. 1. var_export/other starts on SplFixedArray $splArray with refcount 1 2. gc is triggered, whether by gc_collect_cycles or normal gc timing, temporarily incrementing refcount from 1 to 2. 3. A destructor of some other class is called 4. That destructor calls json_encode($splArray) directly or indirectly, while obj->properties has a refcount of 2 5. json_encode gets (and leaks) a copy of the original array and decrements the reference count of the original array, which may later cause issues. ```c static int php_var_serialize_get_sleep_props( HashTable *ht, zval *struc, HashTable *sleep_retval) /* {{{ */ { zend_class_entry *ce = Z_OBJCE_P(struc); HashTable *props = zend_get_properties_for(struc, ZEND_PROP_PURPOSE_SERIALIZE); ``` Overrides of __sleep also increase reference counts in zend_get_properties_for ```c // ext/spl/spl_fixedarray.c static HashTable* spl_fixedarray_object_get_gc(zend_object *obj, zval **table, int *n) { spl_fixedarray_object *intern = spl_fixed_array_from_obj(obj); HashTable *ht = zend_std_get_properties(obj); *table = intern->array.elements; *n = (int)intern->array.size; return ht; } ```