php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Bug #69227 Use after free in zval_scan caused by spl_object_storage_get_gc
Submitted: 2015-03-12 07:50 UTC Modified: 2015-03-13 07:46 UTC
From: adam at vektah dot net Assigned: laruence (profile)
Status: Closed Package: SPL related
PHP Version: 5.5.22 OS: ubuntu 14.10
Private report: No CVE-ID: None
View Developer Edit
 [2015-03-12 07:50 UTC] 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)

Patches

Pull Requests

Pull requests:

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-03-12 09:04 UTC] laruence@php.net
-Status: Open +Status: Verified -Assigned To: +Assigned To: laruence
 [2015-03-12 09:04 UTC] laruence@php.net 
confirm, relates to recursively calls to get_gc.
 [2015-03-12 10:18 UTC] 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 10:31 UTC] laruence@php.net 
s ,going to be freed ,processing,
 [2015-03-12 10:38 UTC] adam at vektah dot net 
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
 [2015-03-13 04:10 UTC] adam at vektah dot net 
What about using the the *zval[] table, instead of modifying properties

eg: https://github.com/php/php-src/pull/1175
 [2015-03-13 17:01 UTC] laruence@php.net 
Automatic comment on behalf of adam.scarr@99designs.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=950d3d6e9b94b75b266c67bf9e3a85ae9c31905d
Log: Fix bug #69227 and #65967
 [2015-03-13 17:01 UTC] laruence@php.net
-Status: Verified +Status: Closed
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Thu Nov 21 08:01:29 2024 UTC