Commit ae65a02
committed
Fix use-after-free in FE_FREE + RETURN with GC
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.1 parent f3b9482 commit ae65a02
3 files changed
+33
-0
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3155 | 3155 | | |
3156 | 3156 | | |
3157 | 3157 | | |
| 3158 | + | |
3158 | 3159 | | |
3159 | 3160 | | |
3160 | 3161 | | |
| |||
3163 | 3164 | | |
3164 | 3165 | | |
3165 | 3166 | | |
| 3167 | + | |
3166 | 3168 | | |
3167 | 3169 | | |
3168 | 3170 | | |
| |||
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
0 commit comments