Bug #79259 [Ver]: Segfault in php_array_element_dump

Edit report at https://bugs.php.net/bug.php?id=79259&edit=1 ID: 79259 Updated by: laruence@php.net Reported by: changochen1 at gmail dot com Summary: Segfault in php_array_element_dump Status: Verified Type: Bug Package: Scripting Engine problem Operating System: ALL PHP Version: master-Git-2020-02-11 (Git) Block user comment: N Private report: N New Comment: because if we don’t keep the gc_protect flag, it may lead to a infinite loop if the array has recursive ref, like: var_dump : 1 if (is_recursion(arr)) return; gc_protected(arr); ++refcount(arr) 2 var_dump_arr_element if arr has a ref to itsef, then it will goto 1 with arr again, if array_dup is called in 2: and we didn’t keep the gc_proteced, then 1: will take effect , infinite loop will occur. Previous Comments: ------------------------------------------------------------------------ [2020-02-12 15:23:36] nikic@php.net @laruence I'm not sure I understand why recursion protection should be inherited. If we addref the array (with GC recursion flag set) in var_dump, and then someone modifies the array, it will be separated and they should get a copy without the recursion flag, while var_dump is going to keep working on the original array with the recursion flag, so it should all work out in the end. Or not? ------------------------------------------------------------------------ [2020-02-12 15:18:13] laruence@php.net okey, made a patch against 7.4: diff --git a/Zend/zend_hash.c b/Zend/zend_hash.c index 7a251ec..5dd4983 100644 --- a/Zend/zend_hash.c +++ b/Zend/zend_hash.c @@ -2073,6 +2073,9 @@ ZEND_API HashTable* ZEND_FASTCALL zend_array_dup(HashTable *source) target->nInternalPointer = source->nInternalPointer; memcpy(HT_GET_DATA_ADDR(target), HT_GET_DATA_ADDR(source), HT_USED_SIZE(source)); } else if (HT_FLAGS(source) & HASH_FLAG_PACKED) { + if (UNEXPECTED(GC_IS_RECURSIVE(source))) { + GC_PROTECT_RECURSION(target); + } HT_FLAGS(target) = HT_FLAGS(source) & HASH_FLAG_MASK; target->nTableMask = HT_MIN_MASK; target->nNumUsed = source->nNumUsed; @@ -2092,6 +2095,9 @@ ZEND_API HashTable* ZEND_FASTCALL zend_array_dup(HashTable *source) zend_array_dup_packed_elements(source, target, 1); } } else { + if (UNEXPECTED(GC_IS_RECURSIVE(source))) { + GC_PROTECT_RECURSION(target); + } HT_FLAGS(target) = HT_FLAGS(source) & HASH_FLAG_MASK; target->nTableMask = source->nTableMask; target->nNextFreeElement = source->nNextFreeElement; diff --git a/ext/standard/var.c b/ext/standard/var.c index 3f7db8f..c1fe15a 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -129,6 +129,7 @@ again: return; } GC_PROTECT_RECURSION(myht); + GC_ADDREF(myht); } count = zend_array_count(myht); php_printf("%sarray(%d) {\n", COMMON, count); @@ -138,6 +139,9 @@ again: } ZEND_HASH_FOREACH_END(); if (!(GC_FLAGS(myht) & GC_IMMUTABLE)) { GC_UNPROTECT_RECURSION(myht); + if (GC_DELREF(myht) == 0) { + zend_array_destroy(myht); + } } if (level > 1) { php_printf("%*c", level-1, ' '); running test, not sure about the gc_protected inheritance part, @nikic do you see any problems? ------------------------------------------------------------------------ [2020-02-12 15:11:34] laruence@php.net hmm, nop: $obj = new Stdclass(); $obj->arr = [1, 2, 3, 4, 5]; $obj->arr = &$obj->arr; ob_start ( function ( $name ) use($obj){ $obj->arr[] = 1; } , 1) ; var_dump($obj); ------------------------------------------------------------------------ [2020-02-12 15:08:45] nikic@php.net @laruence I don't think this is $GLOBALS specific. For example: $obj = range(0, 5); ob_start(function($name) use (&$obj) { static $i = 0; if ($i++ == 5) $obj["foo"] = "bar"; }, 1); var_dump([&$obj]); ------------------------------------------------------------------------ [2020-02-12 15:05:01] laruence@php.net hmm, probably right, but we also need handle the GC_PROTECTED inheritance , and I am also thinking , seems only $GLOBALS array could trigger this problem, in that case, the solution might be easy... ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=79259 -- Edit this bug report at https://bugs.php.net/bug.php?id=79259&edit=1