Skip to content

Commit 7192664

Browse files
committed
Fix phpGH-21927: Use-after-free of self-freeing MultipleIterator children.
Add a refcount on the child iterator across rewind/next/valid/current/key calls so user methods can detach themselves without freeing the object mid-call. close phpGH-21933
1 parent cb3dc62 commit 7192664

3 files changed

Lines changed: 173 additions & 3 deletions

File tree

NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ PHP NEWS
1212
- SPL:
1313
. Fix SplFixedArray::setSize leak when destructor grows during clear.
1414
(David Carlier)
15+
. Fixed bug GH-21933 (use after free of self-freeing MultipleIterator
16+
children). (David Carlier)
1517

1618
- Standard:
1719
. Fixed bug GH-21689 (version_compare() incorrectly handles versions ending

ext/spl/spl_observer.c

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1232,7 +1232,9 @@ PHP_METHOD(MultipleIterator, rewind)
12321232
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
12331233
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
12341234
zend_object *it = element->obj;
1235+
GC_ADDREF(it);
12351236
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_rewind, it, NULL);
1237+
OBJ_RELEASE(it);
12361238
zend_hash_move_forward_ex(&intern->storage, &intern->pos);
12371239
}
12381240
}
@@ -1253,7 +1255,9 @@ PHP_METHOD(MultipleIterator, next)
12531255
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
12541256
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
12551257
zend_object *it = element->obj;
1258+
GC_ADDREF(it);
12561259
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_next, it, NULL);
1260+
OBJ_RELEASE(it);
12571261
zend_hash_move_forward_ex(&intern->storage, &intern->pos);
12581262
}
12591263
}
@@ -1282,7 +1286,9 @@ PHP_METHOD(MultipleIterator, valid)
12821286
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
12831287
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
12841288
zend_object *it = element->obj;
1289+
GC_ADDREF(it);
12851290
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_valid, it, &retval);
1291+
OBJ_RELEASE(it);
12861292

12871293
if (!Z_ISUNDEF(retval)) {
12881294
valid = (Z_TYPE(retval) == IS_TRUE);
@@ -1320,6 +1326,9 @@ static void spl_multiple_iterator_get_all(spl_SplObjectStorage *intern, int get_
13201326
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
13211327
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
13221328
zend_object *it = element->obj;
1329+
zval inf;
1330+
GC_ADDREF(it);
1331+
ZVAL_COPY(&inf, &element->inf);
13231332
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_valid, it, &retval);
13241333

13251334
if (!Z_ISUNDEF(retval)) {
@@ -1336,10 +1345,14 @@ static void spl_multiple_iterator_get_all(spl_SplObjectStorage *intern, int get_
13361345
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_key, it, &retval);
13371346
}
13381347
if (Z_ISUNDEF(retval)) {
1348+
OBJ_RELEASE(it);
1349+
zval_ptr_dtor(&inf);
13391350
zend_throw_exception(spl_ce_RuntimeException, "Failed to call sub iterator method", 0);
13401351
return;
13411352
}
13421353
} else if (intern->flags & MIT_NEED_ALL) {
1354+
OBJ_RELEASE(it);
1355+
zval_ptr_dtor(&inf);
13431356
if (SPL_MULTIPLE_ITERATOR_GET_ALL_CURRENT == get_type) {
13441357
zend_throw_exception(spl_ce_RuntimeException, "Called current() with non valid sub iterator", 0);
13451358
} else {
@@ -1351,22 +1364,26 @@ static void spl_multiple_iterator_get_all(spl_SplObjectStorage *intern, int get_
13511364
}
13521365

13531366
if (intern->flags & MIT_KEYS_ASSOC) {
1354-
switch (Z_TYPE(element->inf)) {
1367+
switch (Z_TYPE(inf)) {
13551368
case IS_LONG:
1356-
add_index_zval(return_value, Z_LVAL(element->inf), &retval);
1369+
add_index_zval(return_value, Z_LVAL(inf), &retval);
13571370
break;
13581371
case IS_STRING:
1359-
zend_symtable_update(Z_ARRVAL_P(return_value), Z_STR(element->inf), &retval);
1372+
zend_symtable_update(Z_ARRVAL_P(return_value), Z_STR(inf), &retval);
13601373
break;
13611374
default:
13621375
zval_ptr_dtor(&retval);
1376+
OBJ_RELEASE(it);
1377+
zval_ptr_dtor(&inf);
13631378
zend_throw_exception(spl_ce_InvalidArgumentException, "Sub-Iterator is associated with NULL", 0);
13641379
return;
13651380
}
13661381
} else {
13671382
add_next_index_zval(return_value, &retval);
13681383
}
13691384

1385+
OBJ_RELEASE(it);
1386+
zval_ptr_dtor(&inf);
13701387
zend_hash_move_forward_ex(&intern->storage, &intern->pos);
13711388
}
13721389
}

ext/spl/tests/gh21927.phpt

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
--TEST--
2+
GH-21927: Use-after-free of self-freeing MultipleIterator children
3+
--FILE--
4+
<?php
5+
6+
class DetachOnRewind implements Iterator {
7+
public function __construct(private MultipleIterator $parent) {}
8+
public function rewind(): void {
9+
$this->parent->detachIterator($this);
10+
echo "rewind: still alive\n";
11+
}
12+
public function next(): void {}
13+
public function current(): mixed { return 0; }
14+
public function key(): mixed { return 0; }
15+
public function valid(): bool { return false; }
16+
}
17+
18+
class DetachOnNext implements Iterator {
19+
public function __construct(private MultipleIterator $parent) {}
20+
public function rewind(): void {}
21+
public function next(): void {
22+
$this->parent->detachIterator($this);
23+
echo "next: still alive\n";
24+
}
25+
public function current(): mixed { return 0; }
26+
public function key(): mixed { return 0; }
27+
public function valid(): bool { return true; }
28+
}
29+
30+
class DetachOnValid implements Iterator {
31+
public function __construct(private MultipleIterator $parent) {}
32+
public function rewind(): void {}
33+
public function next(): void {}
34+
public function current(): mixed { return 0; }
35+
public function key(): mixed { return 0; }
36+
public function valid(): bool {
37+
$this->parent->detachIterator($this);
38+
echo "valid: still alive\n";
39+
return true;
40+
}
41+
}
42+
43+
class DetachOnCurrent implements Iterator {
44+
public function __construct(private MultipleIterator $parent) {}
45+
public function rewind(): void {}
46+
public function next(): void {}
47+
public function current(): mixed {
48+
$this->parent->detachIterator($this);
49+
echo "current: still alive\n";
50+
return 'C';
51+
}
52+
public function key(): mixed { return 'k'; }
53+
public function valid(): bool { return true; }
54+
}
55+
56+
class DetachOnKey implements Iterator {
57+
public function __construct(private MultipleIterator $parent) {}
58+
public function rewind(): void {}
59+
public function next(): void {}
60+
public function current(): mixed { return 'C'; }
61+
public function key(): mixed {
62+
$this->parent->detachIterator($this);
63+
echo "key: still alive\n";
64+
return 'K';
65+
}
66+
public function valid(): bool { return true; }
67+
}
68+
69+
echo "-- detach inside rewind --\n";
70+
$mi = new MultipleIterator();
71+
$mi->attachIterator(new DetachOnRewind($mi));
72+
$mi->rewind();
73+
var_dump($mi->countIterators());
74+
75+
echo "-- detach inside next --\n";
76+
$mi = new MultipleIterator();
77+
$mi->attachIterator(new DetachOnNext($mi));
78+
$mi->rewind();
79+
$mi->next();
80+
var_dump($mi->countIterators());
81+
82+
echo "-- detach inside valid --\n";
83+
$mi = new MultipleIterator();
84+
$mi->attachIterator(new DetachOnValid($mi));
85+
var_dump($mi->valid());
86+
var_dump($mi->countIterators());
87+
88+
echo "-- detach inside current (numeric keys) --\n";
89+
$mi = new MultipleIterator();
90+
$mi->attachIterator(new DetachOnCurrent($mi));
91+
var_dump($mi->current());
92+
var_dump($mi->countIterators());
93+
94+
echo "-- detach inside key (numeric keys) --\n";
95+
$mi = new MultipleIterator();
96+
$mi->attachIterator(new DetachOnKey($mi));
97+
var_dump($mi->key());
98+
var_dump($mi->countIterators());
99+
100+
echo "-- detach inside current (assoc keys, refcounted inf) --\n";
101+
$mi = new MultipleIterator(MultipleIterator::MIT_NEED_ALL | MultipleIterator::MIT_KEYS_ASSOC);
102+
$mi->attachIterator(new DetachOnCurrent($mi), 'cur_info_string');
103+
var_dump($mi->current());
104+
var_dump($mi->countIterators());
105+
106+
echo "-- detach inside key (assoc keys, refcounted inf) --\n";
107+
$mi = new MultipleIterator(MultipleIterator::MIT_NEED_ALL | MultipleIterator::MIT_KEYS_ASSOC);
108+
$mi->attachIterator(new DetachOnKey($mi), 'key_info_string');
109+
var_dump($mi->key());
110+
var_dump($mi->countIterators());
111+
112+
?>
113+
--EXPECT--
114+
-- detach inside rewind --
115+
rewind: still alive
116+
int(0)
117+
-- detach inside next --
118+
next: still alive
119+
int(0)
120+
-- detach inside valid --
121+
valid: still alive
122+
bool(true)
123+
int(0)
124+
-- detach inside current (numeric keys) --
125+
current: still alive
126+
array(1) {
127+
[0]=>
128+
string(1) "C"
129+
}
130+
int(0)
131+
-- detach inside key (numeric keys) --
132+
key: still alive
133+
array(1) {
134+
[0]=>
135+
string(1) "K"
136+
}
137+
int(0)
138+
-- detach inside current (assoc keys, refcounted inf) --
139+
current: still alive
140+
array(1) {
141+
["cur_info_string"]=>
142+
string(1) "C"
143+
}
144+
int(0)
145+
-- detach inside key (assoc keys, refcounted inf) --
146+
key: still alive
147+
array(1) {
148+
["key_info_string"]=>
149+
string(1) "K"
150+
}
151+
int(0)

0 commit comments

Comments
 (0)