From ac7a8a4210f8b48dbcd1ec47caa2b9993b6c8e05 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sat, 2 May 2026 16:27:25 +0100 Subject: [PATCH] Fix GH-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. --- ext/spl/spl_observer.c | 23 +++++- ext/spl/tests/gh21927.phpt | 151 +++++++++++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 3 deletions(-) create mode 100644 ext/spl/tests/gh21927.phpt diff --git a/ext/spl/spl_observer.c b/ext/spl/spl_observer.c index e653d1173b24..56cdbdd4b5f3 100644 --- a/ext/spl/spl_observer.c +++ b/ext/spl/spl_observer.c @@ -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); } } @@ -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); } } @@ -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); @@ -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)) { @@ -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 { @@ -1351,15 +1364,17 @@ 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; } @@ -1367,6 +1382,8 @@ static void spl_multiple_iterator_get_all(spl_SplObjectStorage *intern, int get_ add_next_index_zval(return_value, &retval); } + OBJ_RELEASE(it); + zval_ptr_dtor(&inf); zend_hash_move_forward_ex(&intern->storage, &intern->pos); } } diff --git a/ext/spl/tests/gh21927.phpt b/ext/spl/tests/gh21927.phpt new file mode 100644 index 000000000000..a60ea5ca1025 --- /dev/null +++ b/ext/spl/tests/gh21927.phpt @@ -0,0 +1,151 @@ +--TEST-- +GH-21927: Use-after-free of self-freeing MultipleIterator children +--FILE-- +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)