php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #79259 Segfault in php_array_element_dump
Submitted: 2020-02-11 17:15 UTC Modified: 2020-07-12 04:37 UTC
From: changochen1 at gmail dot com Assigned:
Status: Closed Package: Scripting Engine problem
PHP Version: master-Git-2020-02-11 (Git) OS: ALL
Private report: No CVE-ID: None
 [2020-02-11 17:15 UTC] changochen1 at gmail dot com
Description:
------------
Segfault in php_array_element_dump.

PHP version:
```
PHP 8.0.0-dev (cli) (built: Jan 31 2020 21:52:09) ( NTS )
```

Run script:
```
php -f poc.php
```

Stack dump:
```
=================================================================
==301720==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000014 (pc 0x000000bc05f1 bp 0x7ffcdd24dec0 sp 0x7ffcdd24dea0 T0)
    #0 0xbc05f0 in php_array_element_dump (/home/rxz226/php-src/bld_asan/sapi/cli/php+0xbc05f0)
    #1 0xbc12df in php_var_dump (/home/rxz226/php-src/bld_asan/sapi/cli/php+0xbc12df)
    #2 0xbc062a in php_array_element_dump (/home/rxz226/php-src/bld_asan/sapi/cli/php+0xbc062a)
    #3 0xbc12df in php_var_dump (/home/rxz226/php-src/bld_asan/sapi/cli/php+0xbc12df)
    #4 0xbc2184 in zif_var_dump (/home/rxz226/php-src/bld_asan/sapi/cli/php+0xbc2184)
    #5 0x123b7e9 in execute_ex (/home/rxz226/php-src/bld_asan/sapi/cli/php+0x123b7e9)
    #6 0x127aab7 in zend_execute (/home/rxz226/php-src/bld_asan/sapi/cli/php+0x127aab7)
    #7 0xe43dfb in zend_execute_scripts (/home/rxz226/php-src/bld_asan/sapi/cli/php+0xe43dfb)
    #8 0xcab3b7 in php_execute_script (/home/rxz226/php-src/bld_asan/sapi/cli/php+0xcab3b7)
    #9 0x1280971 in do_cli (/home/rxz226/php-src/bld_asan/sapi/cli/php+0x1280971)
    #10 0x1282acb in main (/home/rxz226/php-src/bld_asan/sapi/cli/php+0x1282acb)
    #11 0x7f55f1ee282f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #12 0x428a78 in _start (/home/rxz226/php-src/bld_asan/sapi/cli/php+0x428a78)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ??:0 php_array_element_dump
```

Test script:
---------------
<?php
$strings = array ( 'into' , 'info' , 'inf' , 'infinity' , 'infin' , 'inflammable' ) ;
foreach ( $strings as $v ) { var_dump ( [ ++ $GLOBALS ] ) ;
        $a [ $b = 0 ] [ ] [ 2 ] ++ ;
        unset ( $GLOBALS [ is_subclass_of ( ob_start ( function ( $name ) { $GLOBALS [ ] = & $obj ;
                                        var_dump ( $name ) ;
                                        }
                                        , 1 ) , 'dooh' ) [ ++ $i ] ] ) ;
}


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-02-11 19:44 UTC] stas@php.net
-Type: Security +Type: Bug
 [2020-02-12 06:40 UTC] laruence@php.net
-Status: Open +Status: Verified
 [2020-02-12 06:40 UTC] laruence@php.net
this is simply because the dumping array is resized(reallocaed).

a simple reproduced script is:
$obj = range(0, 5);
ob_start ( function ( $name ) { $GLOBALS[] = &$obj; } , 1 ) ;
var_dump ( $GLOBALS) ;


I am not sure how to fix this with a reasonable cost
 [2020-02-12 09:19 UTC] nikic@php.net
Just increasing the refcount while dumping should probably work? We do things like that in various recursive walks due to issues like this.
 [2020-02-12 15:05 UTC] 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...
 [2020-02-12 15:08 UTC] 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:11 UTC] 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:18 UTC] 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:23 UTC] 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:49 UTC] laruence@php.net
all tests pass, 
anyway, to be honest I agree the gc_protected part seems weird , maybe you could have a better slotion.
 [2020-02-12 15:55 UTC] laruence@php.net
sorry for my poor english, let me try to explain the gc_protected part agian:

if we don’t keep the gc_protected flag, it will lead to a infinite loop in case the array has a recursive ref to itself, some thing like:
 arr[1] = &arr;
 var_dump(arr);
 ++refcount(arr);
   var_dump(arr[1]);
   ob_start called , separate arr[1], arr[1] became a new array also have arr[1] is it’self
  then var_dump(arr) lead to a infinte loop.

 if I still din’t make myself clear,  you may try with the test script without the gc_protcted inheritance part.
 [2020-07-12 04:37 UTC] changochen1 at gmail dot com
-Status: Verified +Status: Closed
 [2020-07-12 04:37 UTC] changochen1 at gmail dot com
Patched
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Tue Jan 26 09:01:23 2021 UTC