Bug #69227 [Com]: Use after free in zval_scan caused by spl_object_storage_get_gc

Edit report at https://bugs.php.net/bug.php?id=69227&edit=1 ID: 69227 Comment by: adam at vektah dot net Reported by: adam at vektah dot net Summary: Use after free in zval_scan caused by spl_object_storage_get_gc Status: Verified Type: Bug Package: SPL related Operating System: ubuntu 14.10 PHP Version: 5.5.22 Assigned To: laruence Block user comment: N Private report: N New Comment: Wouldn't the refcount always be zero during the sweep? mark has already decremented everything by 1. Does it need to be in props at all? It looks like it was tweaked back in 2012 from an implementation that needed to modify props: https://github.com/php/php-src/commit/df97c3aa0d331be668bd5d8f27fff96d4e3ac1d7 It also seems to be causing other issues eg: https://bugs.php.net/bug.php?id=65967 Previous Comments: ------------------------------------------------------------------------ [2015-03-12 10:31:55] laruence@php.net s ,going to be freed ,processing, ------------------------------------------------------------------------ [2015-03-12 10:18:40] laruence@php.net A quick fix could be: diff --git a/ext/spl/spl_observer.c b/ext/spl/spl_observer.c index 5e21088..4fc0b6f 100644 --- a/ext/spl/spl_observer.c +++ b/ext/spl/spl_observer.c @@ -378,6 +378,10 @@ static HashTable *spl_object_storage_get_gc(zval *obj, zval ***table, int *n TSR /* clean \x00gcdata, as it may be out of date */ if (zend_hash_find(props, "\x00gcdata", sizeof("\x00gcdata"), (void**) &gcdata_arr_pp) == SUCCESS) { + if (Z_REFCOUNT_PP(gcdata_arr_pp) == 0) { + /* this is going to be freed by gc, don't make a new one */ + return props; + } gcdata_arr = *gcdata_arr_pp; zend_hash_clean(Z_ARRVAL_P(gcdata_arr)); } but let me think of side affects... ------------------------------------------------------------------------ [2015-03-12 09:04:34] laruence@php.net confirm, relates to recursively calls to get_gc. ------------------------------------------------------------------------ [2015-03-12 07:50:09] adam at vektah dot net Description: ------------ When an SplObjectStorage instance contains a reference to itself the garbage the garbage collector uses memory that has already been freed. I have tested this on both % php --version PHP 5.5.12-2ubuntu4.2 (cli) (built: Feb 13 2015 18:56:49) Copyright (c) 1997-2014 The PHP Group Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2014, by Zend Technologies % php --version PHP 5.5.22-1+deb.sury.org~trusty+1 (cli) (built: Feb 20 2015 11:26:12) Copyright (c) 1997-2015 The PHP Group Zend Engine v2.5.0, Copyright (c) 1998-2015 Zend Technologies with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2015, by Zend Technologies It appears that the gc scan phase keeps a reference to the properties of the object while recursing over the children. The spl_object_storage_get_gc method then cleans the underlying hash leaving the caller with an invalid pointer. It is quite difficult to get a reliable segfault out in a short test, but valgrind can see the issue as long as USE_ZEND_ALLOC=0 Test script: --------------- <?php $s = new SplObjectStorage(); $s->attach($s); gc_collect_cycles(); Expected result: ---------------- No use after free, no segfault. Actual result: -------------- 5.5.22: % USE_ZEND_ALLOC=0 valgrind --track-origins=yes php test.php ==7739== Memcheck, a memory error detector ==7739== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. ==7739== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info ==7739== Command: php test.php ==7739== ==7739== Warning: set address range perms: large range [0x1614c000, 0x3614c000) (defined) ==7739== Invalid read of size 8 ==7739== at 0x6F066D: zval_scan (in /usr/bin/php5) ==7739== by 0x6F0B9C: gc_collect_cycles (in /usr/bin/php5) ==7739== by 0x6E2628: zif_gc_collect_cycles (in /usr/bin/php5) ==7739== by 0x6C19DA: dtrace_execute_internal (in /usr/bin/php5) ==7739== by 0x781FC4: zend_do_fcall_common_helper_SPEC (in /usr/bin/php5) ==7739== by 0x6FBC97: execute_ex (in /usr/bin/php5) ==7739== by 0x6C18D8: dtrace_execute_ex (in /usr/bin/php5) ==7739== by 0x78260F: zend_do_fcall_common_helper_SPEC (in /usr/bin/php5) ==7739== by 0x6FBC97: execute_ex (in /usr/bin/php5) ==7739== by 0x6C18D8: dtrace_execute_ex (in /usr/bin/php5) ==7739== by 0x6D353F: zend_execute_scripts (in /usr/bin/php5) ==7739== by 0x673084: php_execute_script (in /usr/bin/php5) ==7739== Address 0x156fe700 is 32 bytes inside a block of size 72 free'd ==7739== at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==7739== by 0x6DF8BD: zend_hash_clean (in /usr/bin/php5) ==7739== by 0x5DB66C: spl_object_storage_get_gc (in /usr/bin/php5) ==7739== by 0x6F0093: zval_scan_black (in /usr/bin/php5) ==7739== by 0x6F0574: zval_scan (in /usr/bin/php5) ==7739== by 0x6F066C: zval_scan (in /usr/bin/php5) ==7739== by 0x6F0B9C: gc_collect_cycles (in /usr/bin/php5) ==7739== by 0x6E2628: zif_gc_collect_cycles (in /usr/bin/php5) ==7739== by 0x6C19DA: dtrace_execute_internal (in /usr/bin/php5) ==7739== by 0x781FC4: zend_do_fcall_common_helper_SPEC (in /usr/bin/php5) ==7739== by 0x6FBC97: execute_ex (in /usr/bin/php5) ==7739== by 0x6C18D8: dtrace_execute_ex (in /usr/bin/php5) 5.5.12: % USE_ZEND_ALLOC=0 valgrind --track-origins=yes php test2.php ==10230== Memcheck, a memory error detector ==10230== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. ==10230== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info ==10230== Command: php test2.php ==10230== ==10230== Invalid read of size 8 ==10230== at 0x71CE25: zval_scan (zend_gc.c:570) ==10230== by 0x71D39C: zobj_scan (zend_gc.c:605) ==10230== by 0x71D39C: gc_scan_roots (zend_gc.c:625) ==10230== by 0x71D39C: gc_collect_cycles (zend_gc.c:796) ==10230== by 0x70E098: zif_gc_collect_cycles (zend_builtin_functions.c:361) ==10230== by 0x6EC6A9: dtrace_execute_internal (zend_dtrace.c:97) ==10230== by 0x7B67DD: zend_do_fcall_common_helper_SPEC (zend_vm_execute.h:552) ==10230== by 0x728D2F: execute_ex (zend_vm_execute.h:363) ==10230== by 0x6EC547: dtrace_execute_ex (zend_dtrace.c:73) ==10230== by 0x6FE27F: zend_execute_scripts (zend.c:1316) ==10230== by 0x69C08A: php_execute_script (main.c:2506) ==10230== by 0x7B887F: do_cli (php_cli.c:994) ==10230== by 0x461AAC: main (php_cli.c:1378) ==10230== Address 0xfdd00c0 is 32 bytes inside a block of size 72 free'd ==10230== at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==10230== by 0x70AFCD: zend_hash_clean (zend_hash.c:601) ==10230== by 0x5FE384: spl_object_storage_get_gc (spl_observer.c:382) ==10230== by 0x71C7BB: zval_scan_black (zend_gc.c:288) ==10230== by 0x71CD34: zval_scan (zend_gc.c:519) ==10230== by 0x71CE24: zval_scan (zend_gc.c:568) ==10230== by 0x71D39C: zobj_scan (zend_gc.c:605) ==10230== by 0x71D39C: gc_scan_roots (zend_gc.c:625) ==10230== by 0x71D39C: gc_collect_cycles (zend_gc.c:796) ==10230== by 0x70E098: zif_gc_collect_cycles (zend_builtin_functions.c:361) ==10230== by 0x6EC6A9: dtrace_execute_internal (zend_dtrace.c:97) ==10230== by 0x7B67DD: zend_do_fcall_common_helper_SPEC (zend_vm_execute.h:552) ==10230== by 0x728D2F: execute_ex (zend_vm_execute.h:363) ==10230== by 0x6EC547: dtrace_execute_ex (zend_dtrace.c:73) ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=69227&edit=1