Skip to content

Commit 27ed48c

Browse files
bwoebicataphract
bwoebi
and
cataphract
authored
Split the live-ranges of loop variables again (#20865)
* Fix use-after-free in FE_FREE with GC interaction When FE_FREE with ZEND_FREE_ON_RETURN frees the loop variable during an early return from a foreach loop, the live range for the loop variable was incorrectly extending past the FE_FREE to the normal loop end. This caused GC to access the already-freed loop variable when it ran after the RETURN opcode, resulting in use-after-free. Fix by splitting the ZEND_LIVE_LOOP range when an FE_FREE with ZEND_FREE_ON_RETURN is encountered: - One range covers the early return path up to the FE_FREE - A separate range covers the normal loop end FE_FREE - Multiple early returns create multiple separate ranges * Split the live-ranges of loop variables again b0af9ac removed the live-range splitting of foreach variables, however it only added handling to ZEND_HANDLE_EXCEPTION. This was sort-of elegant, until it was realized in 8258b77 that it would leak the return variable, requiring some more special handling. At some point we added live tmpvar rooting in 52cf7ab, but this did not take into account already freed loop variables, which also might happen during ZEND_RETURN, which cannot be trivially accounted for, without even more complicated handling in zend_gc_*_tmpvars() functions. This commit also proposes a simpler way of tracking the loop end in loopvar freeing ops: handle it directly during live range computation rather than during compilation, eliminating the need for opcache to handle it specifically. Further, opcache was using live_ranges in its basic block computation in the past, which it no longer does. Thus this complication is no longer necessary and this approach should be actually simpler now. Closes #20766. Signed-off-by: Bob Weinand <[email protected]> --------- Signed-off-by: Bob Weinand <[email protected]> Co-authored-by: Gustavo Lopes <[email protected]>
1 parent 2c112e3 commit 27ed48c

File tree

14 files changed

+193
-82
lines changed

14 files changed

+193
-82
lines changed

‎NEWS‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ PHP NEWS
99
. Fix OSS-Fuzz #472563272 (Borked block_pass JMP[N]Z optimization). (ilutov)
1010
. Fixed bug GH-GH-20914 (Internal enums can be cloned and compared). (Arnaud)
1111
. Fix OSS-Fuzz #474613951 (Leaked parent property default value). (ilutov)
12+
. Fixed bug GH-20766 (Use-after-free in FE_FREE with GC interaction). (Bob)
1213

1314
- Date:
1415
. Update timelib to 2022.16. (Derick)

‎Zend/Optimizer/zend_dump.c‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,10 @@ static void zend_dump_unused_op(const zend_op *opline, znode_op op, uint32_t fla
122122
if (op.num != (uint32_t)-1) {
123123
fprintf(stderr, " try-catch(%u)", op.num);
124124
}
125+
} else if (ZEND_VM_OP_LOOP_END == (flags & ZEND_VM_OP_MASK)) {
126+
if (opline->extended_value & ZEND_FREE_ON_RETURN) {
127+
fprintf(stderr, " loop-end(+%u)", op.num);
128+
}
125129
} else if (ZEND_VM_OP_THIS == (flags & ZEND_VM_OP_MASK)) {
126130
fprintf(stderr, " THIS");
127131
} else if (ZEND_VM_OP_NEXT == (flags & ZEND_VM_OP_MASK)) {

‎Zend/tests/gc_050.phpt‎

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,40 @@
11
--TEST--
2-
GC 050: Destructor are never called twice
2+
GC 050: Try/finally in foreach should create separate live ranges
33
--FILE--
44
<?php
55

6-
class G
7-
{
8-
public static $v;
6+
function f(int $n): object {
7+
try {
8+
foreach ((array) $n as $v) {
9+
if ($n === 1) {
10+
try {
11+
$a = new stdClass;
12+
return $a;
13+
} finally {
14+
return $ret = $a;
15+
}
16+
}
17+
if ($n === 2) {
18+
$b = new stdClass;
19+
return $ret = $b;
20+
}
21+
}
22+
} finally {
23+
$ret->v = 1;
24+
}
25+
return new stdClass;
926
}
1027

11-
class WithDestructor
12-
{
13-
public function __destruct()
14-
{
15-
echo "d\n";
28+
for ($i = 0; $i < 100000; $i++) {
29+
// Create cyclic garbage to trigger GC
30+
$a = new stdClass;
31+
$b = new stdClass;
32+
$a->r = $b;
33+
$b->r = $a;
1634

17-
G::$v = $this;
18-
}
35+
$r = f($i % 2 + 1);
1936
}
20-
21-
$o = new WithDestructor();
22-
$weakO = \WeakReference::create($o);
23-
echo "---\n";
24-
unset($o);
25-
echo "---\n";
26-
var_dump($weakO->get() !== null); // verify if kept allocated
27-
G::$v = null;
28-
echo "---\n";
29-
var_dump($weakO->get() !== null); // verify if released
37+
echo "OK\n";
3038
?>
3139
--EXPECT--
32-
---
33-
d
34-
---
35-
bool(true)
36-
---
37-
bool(false)
40+
OK

‎Zend/tests/gc_051.phpt‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
--TEST--
2+
GC 048: FE_FREE should mark variable as UNDEF to prevent use-after-free during GC
3+
--FILE--
4+
<?php
5+
// FE_FREE frees the iterator but doesn't set zval to UNDEF
6+
// When GC runs during RETURN, zend_gc_remove_root_tmpvars() may access freed memory
7+
8+
function test_foreach_early_return(string $s): object {
9+
foreach ((array) $s as $v) {
10+
$obj = new stdClass;
11+
// in the early return, the VAR for the cast result is still live
12+
return $obj; // the return may trigger GC
13+
}
14+
}
15+
16+
for ($i = 0; $i < 100000; $i++) {
17+
// create cyclic garbage to fill GC buffer
18+
$a = new stdClass;
19+
$b = new stdClass;
20+
$a->ref = $b;
21+
$b->ref = $a;
22+
23+
$result = test_foreach_early_return("x");
24+
}
25+
26+
echo "OK\n";
27+
?>
28+
--EXPECT--
29+
OK

‎Zend/tests/gc_052.phpt‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
--TEST--
2+
GC 049: Multiple early returns from foreach should create separate live ranges
3+
--FILE--
4+
<?php
5+
6+
function f(int $n): object {
7+
foreach ((array) $n as $v) {
8+
if ($n === 1) {
9+
$a = new stdClass;
10+
return $a;
11+
}
12+
if ($n === 2) {
13+
$b = new stdClass;
14+
return $b;
15+
}
16+
if ($n === 3) {
17+
$c = new stdClass;
18+
return $c;
19+
}
20+
}
21+
return new stdClass;
22+
}
23+
24+
for ($i = 0; $i < 100000; $i++) {
25+
// Create cyclic garbage to trigger GC
26+
$a = new stdClass;
27+
$b = new stdClass;
28+
$a->r = $b;
29+
$b->r = $a;
30+
31+
$r = f($i % 3 + 1);
32+
}
33+
echo "OK\n";
34+
?>
35+
--EXPECT--
36+
OK

‎Zend/tests/gc_053.phpt‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
--TEST--
2+
GC 050: Destructor are never called twice
3+
--FILE--
4+
<?php
5+
6+
class G
7+
{
8+
public static $v;
9+
}
10+
11+
class WithDestructor
12+
{
13+
public function __destruct()
14+
{
15+
echo "d\n";
16+
17+
G::$v = $this;
18+
}
19+
}
20+
21+
$o = new WithDestructor();
22+
$weakO = \WeakReference::create($o);
23+
echo "---\n";
24+
unset($o);
25+
echo "---\n";
26+
var_dump($weakO->get() !== null); // verify if kept allocated
27+
G::$v = null;
28+
echo "---\n";
29+
var_dump($weakO->get() !== null); // verify if released
30+
?>
31+
--EXPECT--
32+
---
33+
d
34+
---
35+
bool(true)
36+
---
37+
bool(false)

‎Zend/zend_execute.c‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4673,20 +4673,6 @@ static void cleanup_unfinished_calls(zend_execute_data *execute_data, uint32_t o
46734673
}
46744674
/* }}} */
46754675

4676-
static const zend_live_range *find_live_range(const zend_op_array *op_array, uint32_t op_num, uint32_t var_num) /* {{{ */
4677-
{
4678-
int i;
4679-
for (i = 0; i < op_array->last_live_range; i++) {
4680-
const zend_live_range *range = &op_array->live_range[i];
4681-
if (op_num >= range->start && op_num < range->end
4682-
&& var_num == (range->var & ~ZEND_LIVE_MASK)) {
4683-
return range;
4684-
}
4685-
}
4686-
return NULL;
4687-
}
4688-
/* }}} */
4689-
46904676
static void cleanup_live_vars(zend_execute_data *execute_data, uint32_t op_num, uint32_t catch_op_num) /* {{{ */
46914677
{
46924678
int i;
@@ -4702,6 +4688,16 @@ static void cleanup_live_vars(zend_execute_data *execute_data, uint32_t op_num,
47024688
uint32_t var_num = range->var & ~ZEND_LIVE_MASK;
47034689
zval *var = EX_VAR(var_num);
47044690

4691+
/* Handle the split range for loop vars */
4692+
if (catch_op_num) {
4693+
zend_op *final_op = EX(func)->op_array.opcodes + range->end;
4694+
if (final_op->extended_value & ZEND_FREE_ON_RETURN && (final_op->opcode == ZEND_FE_FREE || final_op->opcode == ZEND_FREE)) {
4695+
if (catch_op_num < range->end + final_op->op2.num) {
4696+
continue;
4697+
}
4698+
}
4699+
}
4700+
47054701
if (kind == ZEND_LIVE_TMPVAR) {
47064702
zval_ptr_dtor_nogc(var);
47074703
} else if (kind == ZEND_LIVE_NEW) {

‎Zend/zend_opcode.c‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -981,6 +981,35 @@ static void zend_calc_live_ranges(
981981
/* OP_DATA is really part of the previous opcode. */
982982
last_use[var_num] = opnum - (opline->opcode == ZEND_OP_DATA);
983983
}
984+
} else if ((opline->opcode == ZEND_FREE || opline->opcode == ZEND_FE_FREE) && opline->extended_value & ZEND_FREE_ON_RETURN) {
985+
int jump_offset = 1;
986+
while (((opline + jump_offset)->opcode == ZEND_FREE || (opline + jump_offset)->opcode == ZEND_FE_FREE)
987+
&& (opline + jump_offset)->extended_value & ZEND_FREE_ON_RETURN) {
988+
++jump_offset;
989+
}
990+
// loop var frees directly precede the jump (or return) operand, except that ZEND_VERIFY_RETURN_TYPE may happen first.
991+
if ((opline + jump_offset)->opcode == ZEND_VERIFY_RETURN_TYPE) {
992+
++jump_offset;
993+
}
994+
/* FREE with ZEND_FREE_ON_RETURN immediately followed by RETURN frees
995+
* the loop variable on early return. We need to split the live range
996+
* so GC doesn't access the freed variable after this FREE. */
997+
uint32_t opnum_last_use = last_use[var_num];
998+
zend_op *opline_last_use = op_array->opcodes + opnum_last_use;
999+
ZEND_ASSERT(opline_last_use->opcode == opline->opcode); // any ZEND_FREE_ON_RETURN must be followed by a FREE without
1000+
if (opnum + jump_offset + 1 != opnum_last_use) {
1001+
emit_live_range_raw(op_array, var_num, opline->opcode == ZEND_FE_FREE ? ZEND_LIVE_LOOP : ZEND_LIVE_TMPVAR,
1002+
opnum + jump_offset + 1, opnum_last_use);
1003+
}
1004+
1005+
/* Update last_use so next range includes this FREE */
1006+
last_use[var_num] = opnum;
1007+
1008+
/* Store opline offset to loop end */
1009+
opline->op2.opline_num = opnum_last_use - opnum;
1010+
if (opline_last_use->extended_value & ZEND_FREE_ON_RETURN) {
1011+
opline->op2.opline_num += opline_last_use->op2.opline_num;
1012+
}
9841013
}
9851014
}
9861015
if (opline->op2_type & (IS_TMP_VAR|IS_VAR)) {

‎Zend/zend_vm_def.h‎

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3193,7 +3193,7 @@ ZEND_VM_COLD_CONST_HANDLER(47, ZEND_JMPNZ_EX, CONST|TMPVAR|CV, JMP_ADDR)
31933193
ZEND_VM_JMP(opline);
31943194
}
31953195

3196-
ZEND_VM_HANDLER(70, ZEND_FREE, TMPVAR, ANY)
3196+
ZEND_VM_HANDLER(70, ZEND_FREE, TMPVAR, LOOP_END)
31973197
{
31983198
USE_OPLINE
31993199

@@ -3202,7 +3202,7 @@ ZEND_VM_HANDLER(70, ZEND_FREE, TMPVAR, ANY)
32023202
ZEND_VM_NEXT_OPCODE_CHECK_EXCEPTION();
32033203
}
32043204

3205-
ZEND_VM_HOT_HANDLER(127, ZEND_FE_FREE, TMPVAR, ANY)
3205+
ZEND_VM_HOT_HANDLER(127, ZEND_FE_FREE, TMPVAR, LOOP_END)
32063206
{
32073207
zval *var;
32083208
USE_OPLINE
@@ -8140,24 +8140,11 @@ ZEND_VM_HANDLER(149, ZEND_HANDLE_EXCEPTION, ANY, ANY)
81408140
&& throw_op->extended_value & ZEND_FREE_ON_RETURN) {
81418141
/* exceptions thrown because of loop var destruction on return/break/...
81428142
* are logically thrown at the end of the foreach loop, so adjust the
8143-
* throw_op_num.
8143+
* throw_op_num to the final loop variable FREE.
81448144
*/
8145-
const zend_live_range *range = find_live_range(
8146-
&EX(func)->op_array, throw_op_num, throw_op->op1.var);
8147-
/* free op1 of the corresponding RETURN */
8148-
for (i = throw_op_num; i < range->end; i++) {
8149-
if (EX(func)->op_array.opcodes[i].opcode == ZEND_FREE
8150-
|| EX(func)->op_array.opcodes[i].opcode == ZEND_FE_FREE) {
8151-
/* pass */
8152-
} else {
8153-
if (EX(func)->op_array.opcodes[i].opcode == ZEND_RETURN
8154-
&& (EX(func)->op_array.opcodes[i].op1_type & (IS_VAR|IS_TMP_VAR))) {
8155-
zval_ptr_dtor(EX_VAR(EX(func)->op_array.opcodes[i].op1.var));
8156-
}
8157-
break;
8158-
}
8159-
}
8160-
throw_op_num = range->end;
8145+
uint32_t new_throw_op_num = throw_op_num + throw_op->op2.opline_num;
8146+
cleanup_live_vars(execute_data, throw_op_num, new_throw_op_num);
8147+
throw_op_num = new_throw_op_num;
81618148
}
81628149

81638150
/* Find the innermost try/catch/finally the exception was thrown in */

‎Zend/zend_vm_execute.h‎

Lines changed: 4 additions & 17 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)