Fix use-after-free in FE_FREE with GC interaction

[cataphract]

function f(string $s) {
    foreach ((array) $s as $v) {
        $o = new stdClass;
        return $o;
    }
}

compiles to:

Live ranges:
  V4: 0003 - 0010 (loop)
  V5: 0005 - 0006 (new)

Opcodes:
 0  2  CV0($s) = ZEND_RECV
 1  3  T3 = ZEND_CAST CV0($s)
 2  3  V4 = ZEND_FE_RESET_R T3, ->10
 3  3  ZEND_FE_FETCH_R V4, CV1($v)
 4  4  V5 = ZEND_NEW string("stdClass")
 5  4  ZEND_DO_FCALL
 6  4  ZEND_ASSIGN CV2($o), V5
 7  5  ZEND_FE_FREE V4
 8  5  ZEND_RETURN CV2($o)
 9  3  ZEND_JMP ->3
10  3  ZEND_FE_FREE V4
11  7  ZEND_RETURN null

Since we're returning early, in instruction 8, V4 has been freed. However, ZEND_RETURN may start GC:

if (Z_OPT_REFCOUNTED_P(retval_ptr)) {
if (EXPECTED(!Z_OPT_ISREF_P(retval_ptr))) {
    if (EXPECTED(!(EX_CALL_INFO() & (ZEND_CALL_CODE|ZEND_CALL_OBSERVED)))) {
	zend_refcounted *ref = Z_COUNTED_P(retval_ptr);
	ZVAL_COPY_VALUE(return_value, retval_ptr);
	if (GC_MAY_LEAK(ref)) {
	    SAVE_OPLINE();
	    gc_possible_root(ref);

Eventually, zend_gc_collect_cycles calls zend_gc_remove_root_tmpvars. This function is slighly misnamed, because it also removes from the GC buffer VARs that are loop variables, like V4. And instruction 8 is within the live range of V4. Therefore, it attempts to remove V4 from the buffer. But at this point, the zend_refcount behind V4 has already been destroyed. Hence, we have a use-after free:

$ USE_ZEND_ALLOC=0 valgrind ~/php/8.4.15-release/bin/php test_gc_final.php
==3125745== Memcheck, a memory error detector
==3125745== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3125745== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==3125745== Command: /home/glopes/php/8.4.15-release/bin/php test_gc_final.php
==3125745==
==3125745== Invalid read of size 4
==3125745==    at 0x9E9A5C: zend_gc_remove_root_tmpvars (zend_gc.c:2209)
==3125745==    by 0x9E8983: zend_gc_collect_cycles (zend_gc.c:1921)
==3125745==    by 0x9E3C0E: gc_possible_root_when_full (zend_gc.c:664)
==3125745==    by 0x9E3F0C: gc_possible_root (zend_gc.c:714)
==3125745==    by 0x9D6CE4: execute_ex (zend_vm_execute.h:62997)
==3125745==    by 0x9DC630: zend_execute (zend_vm_execute.h:64319)
==3125745==    by 0xAB4C0B: zend_execute_script (zend.c:1934)
==3125745==    by 0x6F34DB: php_execute_script_ex (main.c:2577)
==3125745==    by 0x6F3659: php_execute_script (main.c:2617)
==3125745==    by 0xAB6D61: do_cli (php_cli.c:935)
==3125745==    by 0xAB7F38: main (php_cli.c:1310)
==3125745==  Address 0x6405c94 is 4 bytes inside a block of size 56 free'd
==3125745==    at 0x484B27F: free (vg_replace_malloc.c:872)
==3125745==    by 0x7F2049: __zend_free (zend_alloc.c:3322)
==3125745==    by 0x7EACFE: _efree_56 (zend_alloc.c:2710)
==3125745==    by 0x9FCF9C: zend_array_destroy (zend_hash.c:1873)
==3125745==    by 0xAA4E5B: rc_dtor_func (zend_variables.c:57)
==3125745==    by 0x9CC9F3: ZEND_FE_FREE_SPEC_TMPVAR_HANDLER (zend_vm_execute.h:15189)
==3125745==    by 0x9CC9F3: execute_ex (zend_vm_execute.h:60761)
==3125745==    by 0x9DC630: zend_execute (zend_vm_execute.h:64319)
==3125745==    by 0xAB4C0B: zend_execute_script (zend.c:1934)
==3125745==    by 0x6F34DB: php_execute_script_ex (main.c:2577)
==3125745==    by 0x6F3659: php_execute_script (main.c:2617)
==3125745==    by 0xAB6D61: do_cli (php_cli.c:935)
==3125745==    by 0xAB7F38: main (php_cli.c:1310)
==3125745==  Block was alloc'd at
==3125745==    at 0x4848899: malloc (vg_replace_malloc.c:381)
==3125745==    by 0x7F1E73: __zend_malloc (zend_alloc.c:3294)
==3125745==    by 0x7E77A6: _emalloc_56 (zend_alloc.c:2662)
==3125745==    by 0x9EF869: _zend_new_array_0 (zend_hash.c:284)
==3125745==    by 0x978D19: ZEND_CAST_SPEC_CV_HANDLER (zend_vm_execute.h:41066)
==3125745==    by 0x9D6E9C: execute_ex (zend_vm_execute.h:63068)
==3125745==    by 0x9DC630: zend_execute (zend_vm_execute.h:64319)
==3125745==    by 0xAB4C0B: zend_execute_script (zend.c:1934)
==3125745==    by 0x6F34DB: php_execute_script_ex (main.c:2577)
==3125745==    by 0x6F3659: php_execute_script (main.c:2617)
==3125745==    by 0xAB6D61: do_cli (php_cli.c:935)
==3125745==    by 0xAB7F38: main (php_cli.c:1310)

The solution adopted was to mark the loop variable zval as UNDEF.

[bwoebi]

This really is a bug in live_range computation :-(
The whole point of live ranges is to not have to zero freed temporaries.

Essentially a proper fix would be creating gaps in live ranges from any freeing op having ZEND_FREE_ON_RETURN as extended_value until the actual ZEND_RETURN; then marking the ZEND_RETURN as starting op for the new live range.

[cataphract]

@bwoebi OK, I've updated the live ranges calculation. Now I have:

Live ranges:
  V4: [0003, 0008) (loop)
  V5: [0005, 0006) (new)
  V4: [0010, 0011) (loop)

Opcodes:
 0   5  CV0($s) = ZEND_RECV
 1   6  T3 = ZEND_CAST CV0($s)  ; to array
 2   6  V4 = ZEND_FE_RESET_R T3, ->11
 3   6  ZEND_FE_FETCH_R V4, CV1($v)
 4   7  V5 = ZEND_NEW string("stdClass")
 5   7  ZEND_DO_FCALL
 6   7  ZEND_ASSIGN CV2($obj), V5
 7   9  ZEND_VERIFY_RETURN_TYPE CV2($obj)
 8   9  ZEND_FE_FREE V4
 9   9  ZEND_RETURN CV2($obj)
10   6  ZEND_JMP ->3
11   6  ZEND_FE_FREE V4
12  11  ZEND_VERIFY_RETURN_TYPE
13  11  ZEND_RETURN null

I need to look at the failures, but is this the direction?

[iluuu1994]

I agree that splitting the live-range is the best approch. This is complicated by finally, which will insert FAST_CALLs between FE_FREE and RETURN. https://3v4l.org/1quOm/vld

Furthermore, nested loops will insert multiple FE_FREE, and hence multiple live-ranges will need to be split. https://3v4l.org/QhpGk/vld

[cataphract]

OK, I'll leave it to someone more in the know, since my changes don't cover those cases and still break 3 opcache tests (assertions that the range is non-empty).

[arnaud-lb]

Exception handling is also affected by the incorrect live ranges when ZEND_FE_FREE throws, and this is handled with a special case in ZEND_HANDLE_EXCEPTION:

php-src/Zend/zend_vm_def.h

Lines 8184 to 8189 in ef52252

	if ((throw_op->opcode == ZEND_FREE || throw_op->opcode == ZEND_FE_FREE)
	&& throw_op->extended_value & ZEND_FREE_ON_RETURN) {
	/* exceptions thrown because of loop var destruction on return/break/...
	* are logically thrown at the end of the foreach loop, so adjust the
	* throw_op_num.
	*/

This updates throw_op_num to the end of the loop, so the variable is not considered live when freeing live vars later.

throw_in_fe_free.php

function f() {
    foreach (it() as $v) {
        return;
    }
}

function it() {
    return new class([0]) extends ArrayIterator {
        public function __destruct() {
            throw new Exception();
        }
    };
}

f();

This was introduced in b0af9ac. Apparently we used handle ZEND_FREE_ON_RETURN in live ranges, but the change was made for simplicity.

For GC, we could apply the same special case.

Delaying GC until a safe point would also work: #19787.

[bwoebi]

For GC, we could apply the same special case.

Applying the same special case isn't that easy, because, unlike normal exceptions (which are deferred to caller frame when happening during return), GC can actually trigger within the ZEND_RETURN opcode, which is not a FE_FREE.

This also goes against the original premise of b0af9ac to simplify. But I agree that the original handling before that commit was too complex.

Let me push a PR.

[bwoebi]

@arnaud-lb Feel free to have a look at #20865. That PR has about the same number of lines added than removed (aside from tests). I think It's fine from complexity perspective too.