Skip to content

Fix GH-21927: Use-after-free of self-freeing MultipleIterator children. #21933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devnexen wants to merge 1 commit into
base: PHP-8.4
Choose a base branch
from
+171 −3
Open

Fix GH-21927: Use-after-free of self-freeing MultipleIterator children. #21933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions ext/spl/spl_observer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1232,7 +1232,9 @@ PHP_METHOD(MultipleIterator, rewind)
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
zend_object *it = element->obj;
GC_ADDREF(it);
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_rewind, it, NULL);
OBJ_RELEASE(it);
zend_hash_move_forward_ex(&intern->storage, &intern->pos);
}
}
Expand All @@ -1253,7 +1255,9 @@ PHP_METHOD(MultipleIterator, next)
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
zend_object *it = element->obj;
GC_ADDREF(it);
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_next, it, NULL);
OBJ_RELEASE(it);
zend_hash_move_forward_ex(&intern->storage, &intern->pos);
}
}
Expand Down Expand Up @@ -1282,7 +1286,9 @@ PHP_METHOD(MultipleIterator, valid)
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
zend_object *it = element->obj;
GC_ADDREF(it);
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_valid, it, &retval);
OBJ_RELEASE(it);

if (!Z_ISUNDEF(retval)) {
valid = (Z_TYPE(retval) == IS_TRUE);
Expand Down Expand Up @@ -1320,6 +1326,9 @@ static void spl_multiple_iterator_get_all(spl_SplObjectStorage *intern, int get_
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
while ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) != NULL && !EG(exception)) {
zend_object *it = element->obj;
zval inf;
GC_ADDREF(it);
ZVAL_COPY(&inf, &element->inf);
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_valid, it, &retval);

if (!Z_ISUNDEF(retval)) {
Expand All @@ -1336,10 +1345,14 @@ static void spl_multiple_iterator_get_all(spl_SplObjectStorage *intern, int get_
zend_call_known_instance_method_with_0_params(it->ce->iterator_funcs_ptr->zf_key, it, &retval);
}
if (Z_ISUNDEF(retval)) {
OBJ_RELEASE(it);
zval_ptr_dtor(&inf);
zend_throw_exception(spl_ce_RuntimeException, "Failed to call sub iterator method", 0);
return;
}
} else if (intern->flags & MIT_NEED_ALL) {
OBJ_RELEASE(it);
zval_ptr_dtor(&inf);
if (SPL_MULTIPLE_ITERATOR_GET_ALL_CURRENT == get_type) {
zend_throw_exception(spl_ce_RuntimeException, "Called current() with non valid sub iterator", 0);
} else {
Expand All @@ -1351,22 +1364,26 @@ static void spl_multiple_iterator_get_all(spl_SplObjectStorage *intern, int get_
}

if (intern->flags & MIT_KEYS_ASSOC) {
switch (Z_TYPE(element->inf)) {
switch (Z_TYPE(inf)) {
case IS_LONG:
add_index_zval(return_value, Z_LVAL(element->inf), &retval);
add_index_zval(return_value, Z_LVAL(inf), &retval);
break;
case IS_STRING:
zend_symtable_update(Z_ARRVAL_P(return_value), Z_STR(element->inf), &retval);
zend_symtable_update(Z_ARRVAL_P(return_value), Z_STR(inf), &retval);
break;
default:
zval_ptr_dtor(&retval);
OBJ_RELEASE(it);
zval_ptr_dtor(&inf);
zend_throw_exception(spl_ce_InvalidArgumentException, "Sub-Iterator is associated with NULL", 0);
return;
}
} else {
add_next_index_zval(return_value, &retval);
}

OBJ_RELEASE(it);
zval_ptr_dtor(&inf);
zend_hash_move_forward_ex(&intern->storage, &intern->pos);
}
}
Expand Down
151 changes: 151 additions & 0 deletions ext/spl/tests/gh21927.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
--TEST--
GH-21927: Use-after-free of self-freeing MultipleIterator children
--FILE--
<?php

class DetachOnRewind implements Iterator {
public function __construct(private MultipleIterator $parent) {}
public function rewind(): void {
$this->parent->detachIterator($this);
echo "rewind: still alive\n";
}
public function next(): void {}
public function current(): mixed { return 0; }
public function key(): mixed { return 0; }
public function valid(): bool { return false; }
}

class DetachOnNext implements Iterator {
public function __construct(private MultipleIterator $parent) {}
public function rewind(): void {}
public function next(): void {
$this->parent->detachIterator($this);
echo "next: still alive\n";
}
public function current(): mixed { return 0; }
public function key(): mixed { return 0; }
public function valid(): bool { return true; }
}

class DetachOnValid implements Iterator {
public function __construct(private MultipleIterator $parent) {}
public function rewind(): void {}
public function next(): void {}
public function current(): mixed { return 0; }
public function key(): mixed { return 0; }
public function valid(): bool {
$this->parent->detachIterator($this);
echo "valid: still alive\n";
return true;
}
}

class DetachOnCurrent implements Iterator {
public function __construct(private MultipleIterator $parent) {}
public function rewind(): void {}
public function next(): void {}
public function current(): mixed {
$this->parent->detachIterator($this);
echo "current: still alive\n";
return 'C';
}
public function key(): mixed { return 'k'; }
public function valid(): bool { return true; }
}

class DetachOnKey implements Iterator {
public function __construct(private MultipleIterator $parent) {}
public function rewind(): void {}
public function next(): void {}
public function current(): mixed { return 'C'; }
public function key(): mixed {
$this->parent->detachIterator($this);
echo "key: still alive\n";
return 'K';
}
public function valid(): bool { return true; }
}

echo "-- detach inside rewind --\n";
$mi = new MultipleIterator();
$mi->attachIterator(new DetachOnRewind($mi));
$mi->rewind();
var_dump($mi->countIterators());

echo "-- detach inside next --\n";
$mi = new MultipleIterator();
$mi->attachIterator(new DetachOnNext($mi));
$mi->rewind();
$mi->next();
var_dump($mi->countIterators());

echo "-- detach inside valid --\n";
$mi = new MultipleIterator();
$mi->attachIterator(new DetachOnValid($mi));
var_dump($mi->valid());
var_dump($mi->countIterators());

echo "-- detach inside current (numeric keys) --\n";
$mi = new MultipleIterator();
$mi->attachIterator(new DetachOnCurrent($mi));
var_dump($mi->current());
var_dump($mi->countIterators());

echo "-- detach inside key (numeric keys) --\n";
$mi = new MultipleIterator();
$mi->attachIterator(new DetachOnKey($mi));
var_dump($mi->key());
var_dump($mi->countIterators());

echo "-- detach inside current (assoc keys, refcounted inf) --\n";
$mi = new MultipleIterator(MultipleIterator::MIT_NEED_ALL | MultipleIterator::MIT_KEYS_ASSOC);
$mi->attachIterator(new DetachOnCurrent($mi), 'cur_info_string');
var_dump($mi->current());
var_dump($mi->countIterators());

echo "-- detach inside key (assoc keys, refcounted inf) --\n";
$mi = new MultipleIterator(MultipleIterator::MIT_NEED_ALL | MultipleIterator::MIT_KEYS_ASSOC);
$mi->attachIterator(new DetachOnKey($mi), 'key_info_string');
var_dump($mi->key());
var_dump($mi->countIterators());

?>
--EXPECT--
-- detach inside rewind --
rewind: still alive
int(0)
-- detach inside next --
next: still alive
int(0)
-- detach inside valid --
valid: still alive
bool(true)
int(0)
-- detach inside current (numeric keys) --
current: still alive
array(1) {
[0]=>
string(1) "C"
}
int(0)
-- detach inside key (numeric keys) --
key: still alive
array(1) {
[0]=>
string(1) "K"
}
int(0)
-- detach inside current (assoc keys, refcounted inf) --
current: still alive
array(1) {
["cur_info_string"]=>
string(1) "C"
}
int(0)
-- detach inside key (assoc keys, refcounted inf) --
key: still alive
array(1) {
["key_info_string"]=>
string(1) "K"
}
int(0)
Loading