Bug #78379 [Ver->Csd]: Cast to object confuses GC, causes crash

Edit report at https://bugs.php.net/bug.php?id=78379&edit=1 ID: 78379 Updated by: dmitry@php.net Reported by: tstarling@php.net Summary: Cast to object confuses GC, causes crash -Status: Verified +Status: Closed Type: Bug Package: Reproducible crash Operating System: Linux PHP Version: 7.2Git-2019-08-06 (Git) Block user comment: N Private report: N New Comment: Automatic comment on behalf of dmitry@zend.com Revision: http://git.php.net/?p=php-src.git;a=commit;h=358379be22c4e20f4942737e0e90422977355c63 Log: Fixed bug #78379 (Cast to object confuses GC, causes crash) Previous Comments: ------------------------------------------------------------------------ [2019-08-08 03:13:37] tstarling@php.net Your patch prevents the array being freed, but it doesn't stop its reference count from going negative, since gc_mark_grey() is still the same, and it won't collect cycles that include the shared array as part of the loop. Both issues can be seen in this test case: class E {} function f() { $e1 = new E; $e2 = new E; $a = ['e2' => $e2]; $e1->a = (object)$a; $e2->e1 = $e1; $e2->a = (object)$a; } f(); gc_collect_cycles(); echo "End\n"; With ZEND_GC_DEBUG=2 you can see it report rc=-1 for object(E)#2, then it says there is "Nothing to free". It's just a memory leak rather than a crash, but I'm considering doing a patch of my own along the lines of the third paragraph of the bug description, so we can see if that looks better. ------------------------------------------------------------------------ [2019-08-07 11:36:23] dmitry@php.net This patch seems to fix the problem, but it may be incomplete. https://gist.github.com/dstogov/43a992d481f65ac16c454e1a292be38e ------------------------------------------------------------------------ [2019-08-07 07:48:46] dmitry@php.net Confirmed. With [1] instead of [], valgrind reports use-after-free problem on PHP-7.2/master (only without opcache). ==13484== Invalid read of size 4 ==13484== at 0x85ED763: php_var_dump (var.c:130) ==13484== by 0x85ED5A6: php_object_property_dump (var.c:87) ==13484== by 0x85EDADE: php_var_dump (var.c:179) ==13484== by 0x85ED5A6: php_object_property_dump (var.c:87) ==13484== by 0x85EDADE: php_var_dump (var.c:179) ==13484== by 0x85EDDAA: zif_var_dump (var.c:223) ==13484== by 0x8765BEA: ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1268) ==13484== by 0x87B8966: execute_ex (zend_vm_execute.h:54223) ==13484== by 0x87BC23A: zend_execute (zend_vm_execute.h:58327) ==13484== by 0x871039F: zend_execute_scripts (zend.c:1631) ==13484== by 0x869EFFE: php_execute_script (main.c:2585) ==13484== by 0x87BE502: do_cli (php_cli.c:962) ==13484== Address 0xa61718c is 4 bytes inside a block of size 44 free'd ==13484== at 0x4036729: free (vg_replace_malloc.c:540) ==13484== by 0x86E5407: _efree_custom (zend_alloc.c:2411) ==13484== by 0x86E5504: _efree (zend_alloc.c:2531) ==13484== by 0x873B0EA: zend_gc_collect_cycles (zend_gc.c:1560) ==13484== by 0x87256B1: zif_gc_collect_cycles (zend_builtin_functions.c:362) ==13484== by 0x8765BEA: ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1268) ==13484== by 0x87B8966: execute_ex (zend_vm_execute.h:54223) ==13484== by 0x87BC23A: zend_execute (zend_vm_execute.h:58327) ==13484== by 0x871039F: zend_execute_scripts (zend.c:1631) ==13484== by 0x869EFFE: php_execute_script (main.c:2585) ==13484== by 0x87BE502: do_cli (php_cli.c:962) ==13484== by 0x87BF11F: main (php_cli.c:1352) ==13484== Block was alloc'd at ==13484== at 0x40356A4: malloc (vg_replace_malloc.c:309) ==13484== by 0x86E60DC: __zend_malloc (zend_alloc.c:2961) ==13484== by 0x86E53B1: _malloc_custom (zend_alloc.c:2402) ==13484== by 0x86E54A3: _emalloc (zend_alloc.c:2521) ==13484== by 0x871CC8D: _zend_new_array (zend_hash.c:256) ==13484== by 0x86F66FB: zend_try_ct_eval_array (zend_compile.c:6965) ==13484== by 0x86FAD01: zend_eval_const_expr (zend_compile.c:8774) ------------------------------------------------------------------------ [2019-08-07 00:59:30] tstarling@php.net Changing the empty array in the test case to a non-empty compile-time constant like [1] allows it to be reproduced in PHP 7.3 and git master. It was just the immutable empty array that stopped the test from working. ------------------------------------------------------------------------ [2019-08-06 05:43:29] tstarling@php.net I also noticed that in related small test cases, the elements of the property array were visited more than once by gc_mark_grey(), and thus had their reference counts decremented below zero, because the property array was not marked grey and did not have its colour checked. A negative reference count protected the elements from deletion, hiding the bug. (The field is unsigned, by negative I mean 0xffffffff) ------------------------------------------------------------------------ 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=78379 -- Edit this bug report at https://bugs.php.net/bug.php?id=78379&edit=1