PHP :: Bug #78379 :: Cast to object confuses GC, causes crash

Bug #78379

Cast to object confuses GC, causes crash

Submitted:

2019-08-06 05:34 UTC

Modified:

2021-07-12 10:56 UTC

Votes:	4
Avg. Score:	3.8 ± 1.3
Reproduced:	2 of 2 (100.0%)
Same Version:	2 (100.0%)
Same OS:	2 (100.0%)

From:

tstarling@php.net

Assigned:

dmitry (profile)

Status:

Closed

Package:

Reproducible crash

PHP Version:

7.2Git-2019-08-06 (Git)

OS:

Linux

Private report:

CVE-ID:

None

View Developer Edit

[2019-08-06 05:34 UTC] tstarling@php.net

Description:
------------
I isolated the reproducible crash described at https://phabricator.wikimedia.org/T228346 . I made a small test case which demonstrates the issue. Although the reduced test case doesn't crash with a normal build, I hope the output demonstrates the severity of the situation. My short test case aborts when run with AddressSanitizer, reporting "heap-use-after-free".

The issue is that when you cast from an array to an object, the array is referenced rather than duplicated, so multiple objects may refer to a single array. When the GC runs on the resulting objects, it does not check the reference count of the property array. So elements of the property array have their reference counts decremented and not incremented again, even though the property array that contains them is still live.

To correctly apply the garbage collection algorithm described in the paper referenced in zend_gc.c, the property array should be considered a child of the object. Declared properties in properties_table are children, but undeclared properties are not.

In the test script, the GC frees the empty array, which is a compile-time constant. Then that memory location is reused during the subsequent "new C", causing var_dump() to show "*RECURSION*".

Test script:
---------------
<?php

class C {
	public function __construct() {
		$this->p = (object)["x" => []];
	}
}
class E {
}
$e = new E;
$e->f = new E;
$e->f->e = $e;
$e->a = new C;
$e = null;
gc_collect_cycles();
var_dump( new C );


Expected result:
----------------
object(C)#2 (1) {
  ["p"]=>
  object(stdClass)#1 (1) {
    ["x"]=>
    array(0) {
    }
  }
}


Actual result:
--------------
object(C)#2 (1) {
  ["p"]=>
  object(stdClass)#1 (1) {
    ["x"]=>
    array(1) {
      ["p"]=>
      *RECURSION*
    }
  }
}

Or with USE_ZEND_ALLOC=0 and AddressSanitizer:

    =================================================================
==25026==ERROR: AddressSanitizer: heap-use-after-free on address 0x60600000a168 at pc 0x55fd7492b790 bp 0x7ffe52626d30 sp 0x7ffe52626d20
READ of size 4 at 0x60600000a168 thread T0
    #0 0x55fd7492b78f in php_var_dump /srv/php/git/ext/standard/var.c:122
    #1 0x55fd7492b2d9 in php_object_property_dump /srv/php/git/ext/standard/var.c:79
    #2 0x55fd7492c367 in php_var_dump /srv/php/git/ext/standard/var.c:164
    #3 0x55fd7492b2d9 in php_object_property_dump /srv/php/git/ext/standard/var.c:79
    #4 0x55fd7492c367 in php_var_dump /srv/php/git/ext/standard/var.c:164
    #5 0x55fd7492c931 in zif_var_dump /srv/php/git/ext/standard/var.c:210
    #6 0x55fd74d5b961 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER /srv/php/git/Zend/zend_vm_execute.h:573
    #7 0x55fd74ef1977 in execute_ex /srv/php/git/Zend/zend_vm_execute.h:59747
    #8 0x55fd74f01595 in zend_execute /srv/php/git/Zend/zend_vm_execute.h:63776
    #9 0x55fd74c624af in zend_execute_scripts /srv/php/git/Zend/zend.c:1498
    #10 0x55fd74b06a4d in php_execute_script /srv/php/git/main/main.c:2599
    #11 0x55fd74f07d72 in do_cli /srv/php/git/sapi/cli/php_cli.c:1011
    #12 0x55fd74f0a011 in main /srv/php/git/sapi/cli/php_cli.c:1403
    #13 0x7f53cf173b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #14 0x55fd73e6a509 in _start (/opt/php7.2/bin/php+0x48f509)

0x60600000a168 is located 8 bytes inside of 56-byte region [0x60600000a160,0x60600000a198)
freed by thread T0 here:
    #0 0x7f53d03e87b8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7b8)
    #1 0x55fd74bcdfb5 in _efree /srv/php/git/Zend/zend_alloc.c:2446
    #2 0x55fd74cf328e in zend_gc_collect_cycles /srv/php/git/Zend/zend_gc.c:1193
    #3 0x55fd74ca853a in zif_gc_collect_cycles /srv/php/git/Zend/zend_builtin_functions.c:356
    #4 0x55fd74d5b961 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER /srv/php/git/Zend/zend_vm_execute.h:573
    #5 0x55fd74ef1977 in execute_ex /srv/php/git/Zend/zend_vm_execute.h:59747
    #6 0x55fd74f01595 in zend_execute /srv/php/git/Zend/zend_vm_execute.h:63776
    #7 0x55fd74c624af in zend_execute_scripts /srv/php/git/Zend/zend.c:1498
    #8 0x55fd74b06a4d in php_execute_script /srv/php/git/main/main.c:2599
    #9 0x55fd74f07d72 in do_cli /srv/php/git/sapi/cli/php_cli.c:1011
    #10 0x55fd74f0a011 in main /srv/php/git/sapi/cli/php_cli.c:1403
    #11 0x7f53cf173b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

previously allocated by thread T0 here:
    #0 0x7f53d03e8b50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
    #1 0x55fd74bcfb28 in __zend_malloc /srv/php/git/Zend/zend_alloc.c:2830
    #2 0x55fd74bcde60 in _emalloc /srv/php/git/Zend/zend_alloc.c:2431
    #3 0x55fd74c6e9d3 in _array_init /srv/php/git/Zend/zend_API.c:1090
    #4 0x55fd74c0488b in zend_try_ct_eval_array /srv/php/git/Zend/zend_compile.c:6994
    #5 0x55fd74c12048 in zend_eval_const_expr /srv/php/git/Zend/zend_compile.c:8628
    #6 0x55fd74c0469f in zend_try_ct_eval_array /srv/php/git/Zend/zend_compile.c:6978
    #7 0x55fd74c09bc4 in zend_compile_array /srv/php/git/Zend/zend_compile.c:7603
    #8 0x55fd74c0ff69 in zend_compile_expr /srv/php/git/Zend/zend_compile.c:8350
    #9 0x55fd74c07115 in zend_compile_cast /srv/php/git/Zend/zend_compile.c:7274
    #10 0x55fd74c0fe49 in zend_compile_expr /srv/php/git/Zend/zend_compile.c:8313
    #11 0x55fd74be4dcf in zend_compile_assign /srv/php/git/Zend/zend_compile.c:3022
    #12 0x55fd74c0fd29 in zend_compile_expr /srv/php/git/Zend/zend_compile.c:8272
    #13 0x55fd74c0f786 in zend_compile_stmt /srv/php/git/Zend/zend_compile.c:8238
    #14 0x55fd74bf7e0c in zend_compile_stmt_list /srv/php/git/Zend/zend_compile.c:5378
    #15 0x55fd74c0f559 in zend_compile_stmt /srv/php/git/Zend/zend_compile.c:8150
    #16 0x55fd74bfcc9b in zend_compile_func_decl /srv/php/git/Zend/zend_compile.c:5990
    #17 0x55fd74c0f6c6 in zend_compile_stmt /srv/php/git/Zend/zend_compile.c:8206
    #18 0x55fd74bf7e0c in zend_compile_stmt_list /srv/php/git/Zend/zend_compile.c:5378
    #19 0x55fd74c0f559 in zend_compile_stmt /srv/php/git/Zend/zend_compile.c:8150
    #20 0x55fd74c0009b in zend_compile_class_decl /srv/php/git/Zend/zend_compile.c:6416
    #21 0x55fd74c0f716 in zend_compile_stmt /srv/php/git/Zend/zend_compile.c:8218
    #22 0x55fd74c0f175 in zend_compile_top_stmt /srv/php/git/Zend/zend_compile.c:8124
    #23 0x55fd74c0f11d in zend_compile_top_stmt /srv/php/git/Zend/zend_compile.c:8119
    #24 0x55fd74b8c0d0 in zend_compile Zend/zend_language_scanner.l:602
    #25 0x55fd74b8c569 in compile_file Zend/zend_language_scanner.l:636
    #26 0x55fd746527de in phar_compile_file /srv/php/git/ext/phar/phar.c:3332
    #27 0x55fd74c62419 in zend_execute_scripts /srv/php/git/Zend/zend.c:1492
    #28 0x55fd74b06a4d in php_execute_script /srv/php/git/main/main.c:2599
    #29 0x55fd74f07d72 in do_cli /srv/php/git/sapi/cli/php_cli.c:1011

SUMMARY: AddressSanitizer: heap-use-after-free /srv/php/git/ext/standard/var.c:122 in php_var_dump
Shadow bytes around the buggy address:
  0x0c0c7fff93d0: 00 00 00 fa fa fa fa fa 00 00 00 00 00 00 00 fa
  0x0c0c7fff93e0: fa fa fa fa 00 00 00 00 00 00 00 fa fa fa fa fa
  0x0c0c7fff93f0: 00 00 00 00 00 00 00 fa fa fa fa fa 00 00 00 00
  0x0c0c7fff9400: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 fa
  0x0c0c7fff9410: fa fa fa fa fd fd fd fd fd fd fd fd fa fa fa fa
=>0x0c0c7fff9420: 00 00 00 00 00 00 00 00 fa fa fa fa fd[fd]fd fd
  0x0c0c7fff9430: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 fa
  0x0c0c7fff9440: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0c7fff9450: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
  0x0c0c7fff9460: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
  0x0c0c7fff9470: fa fa fa fa 00 00 00 00 00 00 00 fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==25026==ABORTING

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports

[2019-08-06 05:43 UTC] 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)

[2019-08-07 00:59 UTC] 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-07 07:48 UTC] 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 10:48 UTC] cmb@php.net

-Status: Open +Status: Verified

[2019-08-07 11:36 UTC] dmitry@php.net

This patch seems to fix the problem, but it may be incomplete.

https://gist.github.com/dstogov/43a992d481f65ac16c454e1a292be38e

[2019-08-08 03:13 UTC] 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-08 07:09 UTC] dmitry@php.net

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)

[2019-08-08 07:09 UTC] dmitry@php.net

-Status: Verified +Status: Closed

[2019-08-08 08:03 UTC] dmitry@php.net

-Status: Closed +Status: Re-Opened

[2019-08-08 08:03 UTC] dmitry@php.net

The committed patch https://github.com/php/php-src/commit/358379be22c4e20f4942737e0e90422977355c63 doesn't fix the second problem.

[2019-08-08 14:18 UTC] dmitry@php.net

Proposed patches https://gist.github.com/dstogov/43a992d481f65ac16c454e1a292be38e
seem to fix the second problem

[2019-08-09 03:33 UTC] tstarling@php.net

I guess it's good enough. The trace output is certainly better than it was.

I did start work on a patch which tries to make property tables be children of objects. The problem I ran into is making sure property tables are not directly destroyed by the garbage collector, since zend_object_std_dtor() etc. doesn't check if the property array has already been destroyed before destroying it, leading to an assertion failure. Maybe it is a thing to try against master rather than in a bugfix for all branches.

[2019-08-09 13:09 UTC] dmitry@php.net

-Status: Re-Opened +Status: Closed -Assigned To: +Assigned To: dmitry

[2019-08-09 13:09 UTC] dmitry@php.net

Should be fixed now.

[2019-08-13 08:01 UTC] nikic@php.net

-Status: Closed +Status: Re-Opened

[2019-08-13 08:01 UTC] nikic@php.net

I've applied an additional fix at https://github.com/php/php-src/commit/18f2918a0fcf66562a5e7d964188c188660e9728 for a crash observed on 7.4.

After looking at this more, the current solution is still incomplete. Looking at a GC trace for https://github.com/php/php-src/blob/6b1cc1252e73e51e53194c8c65e3d2302bc83dca/Zend/tests/bug78379_2.phpt we see that the first GC run does *not* collect the cycle, even though it should. We mark the array root grey first and miss a refcount decrement due to that (as we're not decrementing if it's a property array).

I think the current code may be okay to prevent crashes for 7.2, but we probably need to implement the proper refcount based variant for 7.4 to actually make this work correctly.

[2021-07-12 10:56 UTC] nikic@php.net

-Status: Re-Opened +Status: Closed

[2021-07-12 10:56 UTC] nikic@php.net

I believe the remaining issue here should be fixed by https://github.com/php/php-src/commit/5f8ed7765ab2a0ec372fce90701c51ce75312541.

	php.net \| support \| documentation \| report a bug \| advanced search \| search howto \| statistics \| random bug \| login
go to bug id or search bugs for


Copyright © 2001-2024 The PHP Group All rights reserved.	Last updated: Sat Dec 21 13:01:31 2024 UTC