From a970eefb6c6a3771d0b046b0f75870376e13d00c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 3 Jan 2025 16:40:21 +0100 Subject: [PATCH] Fix GH-11874: intl causing segfault in docker images The segfault happens because zoi->wrapping_obj points to an object that has been freed. This wrapping_obj is set in IntlIterator_from_StringEnumeration(). Notice how the refcount is not increased in this function. By switching to ZVAL_OBJ_COPY, the segfault disappears. We also need to move the responsibility of destroying the iterator to the iterator itself and keep the object data destruction in the object destruction. The existing code used a weird recursive destruction between the iterator and object that was too hard to understand to be honest. This patch simplifies everything and in the process gets rid of the leak. Iterators that are embedded are now responsible for their own memory cleanup. Closes GH-17343. --- NEWS | 3 ++ .../breakiterator/breakiterator_iterators.cpp | 16 +++++++++-- ext/intl/common/common_enum.cpp | 27 +++--------------- ext/intl/tests/gh11874.phpt | 28 +++++++++++++++++++ 4 files changed, 49 insertions(+), 25 deletions(-) create mode 100644 ext/intl/tests/gh11874.phpt diff --git a/NEWS b/NEWS index 3bba53848bddf..2ad9dcdca3fd9 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,9 @@ PHP NEWS - FTP: . Fixed bug GH-16800 (ftp functions can abort with EINTR). (nielsdos) +- Intl: + . Fixed bug GH-11874 (intl causing segfault in docker images). (nielsdos) + 02 Jan 2025, PHP 8.3.16RC1 - Core: diff --git a/ext/intl/breakiterator/breakiterator_iterators.cpp b/ext/intl/breakiterator/breakiterator_iterators.cpp index b95238a24b912..ac0664bb21239 100644 --- a/ext/intl/breakiterator/breakiterator_iterators.cpp +++ b/ext/intl/breakiterator/breakiterator_iterators.cpp @@ -51,6 +51,7 @@ inline BreakIterator *_breakiter_prolog(zend_object_iterator *iter) static void _breakiterator_destroy_it(zend_object_iterator *iter) { zval_ptr_dtor(&iter->data); + /* Don't free iter here because it is allocated as an object on its own, not embedded. */ } static void _breakiterator_move_forward(zend_object_iterator *iter) @@ -79,8 +80,18 @@ static void _breakiterator_rewind(zend_object_iterator *iter) ZVAL_LONG(&zoi_iter->current, (zend_long)pos); } +static void zoi_with_current_dtor_self(zend_object_iterator *iter) +{ + // Note: wrapping_obj is unused, call to zoi_with_current_dtor() not necessary + zoi_with_current *zoi_iter = (zoi_with_current*)iter; + ZEND_ASSERT(Z_ISUNDEF(zoi_iter->wrapping_obj)); + + // Unlike the other iterators, this iterator is a new, standalone instance + zoi_iter->destroy_it(iter); +} + static const zend_object_iterator_funcs breakiterator_iterator_funcs = { - zoi_with_current_dtor, + zoi_with_current_dtor_self, zoi_with_current_valid, zoi_with_current_get_current_data, NULL, @@ -133,6 +144,7 @@ typedef struct zoi_break_iter_parts { static void _breakiterator_parts_destroy_it(zend_object_iterator *iter) { zval_ptr_dtor(&iter->data); + efree(iter); } static void _breakiterator_parts_get_current_key(zend_object_iterator *iter, zval *key) @@ -231,7 +243,7 @@ void IntlIterator_from_BreakIterator_parts(zval *break_iter_zv, ii->iterator->index = 0; ((zoi_with_current*)ii->iterator)->destroy_it = _breakiterator_parts_destroy_it; - ZVAL_OBJ(&((zoi_with_current*)ii->iterator)->wrapping_obj, Z_OBJ_P(object)); + ZVAL_OBJ_COPY(&((zoi_with_current*)ii->iterator)->wrapping_obj, Z_OBJ_P(object)); ZVAL_UNDEF(&((zoi_with_current*)ii->iterator)->current); ((zoi_break_iter_parts*)ii->iterator)->bio = Z_INTL_BREAKITERATOR_P(break_iter_zv); diff --git a/ext/intl/common/common_enum.cpp b/ext/intl/common/common_enum.cpp index 874c748d58b70..a527279558ed6 100644 --- a/ext/intl/common/common_enum.cpp +++ b/ext/intl/common/common_enum.cpp @@ -35,25 +35,7 @@ zend_object_handlers IntlIterator_handlers; void zoi_with_current_dtor(zend_object_iterator *iter) { zoi_with_current *zoiwc = (zoi_with_current*)iter; - - if (!Z_ISUNDEF(zoiwc->wrapping_obj)) { - /* we have to copy the pointer because zoiwc->wrapping_obj may be - * changed midway the execution of zval_ptr_dtor() */ - zval *zwo = &zoiwc->wrapping_obj; - - /* object is still here, we can rely on it to call this again and - * destroy this object */ - zval_ptr_dtor(zwo); - } else { - /* Object not here anymore (we've been called by the object free handler) - * Note that the iterator wrapper objects (that also depend on this - * structure) call this function earlier, in the destruction phase, which - * precedes the object free phase. Therefore there's no risk on this - * function being called by the iterator wrapper destructor function and - * not finding the memory of this iterator allocated anymore. */ - iter->funcs->invalidate_current(iter); - zoiwc->destroy_it(iter); - } + zval_ptr_dtor(&zoiwc->wrapping_obj); } U_CFUNC int zoi_with_current_valid(zend_object_iterator *iter) @@ -124,6 +106,7 @@ static void string_enum_rewind(zend_object_iterator *iter) static void string_enum_destroy_it(zend_object_iterator *iter) { delete (StringEnumeration*)Z_PTR(iter->data); + efree(iter); } static const zend_object_iterator_funcs string_enum_object_iterator_funcs = { @@ -148,7 +131,7 @@ U_CFUNC void IntlIterator_from_StringEnumeration(StringEnumeration *se, zval *ob ii->iterator->funcs = &string_enum_object_iterator_funcs; ii->iterator->index = 0; ((zoi_with_current*)ii->iterator)->destroy_it = string_enum_destroy_it; - ZVAL_OBJ(&((zoi_with_current*)ii->iterator)->wrapping_obj, Z_OBJ_P(object)); + ZVAL_OBJ_COPY(&((zoi_with_current*)ii->iterator)->wrapping_obj, Z_OBJ_P(object)); ZVAL_UNDEF(&((zoi_with_current*)ii->iterator)->current); } @@ -157,9 +140,7 @@ static void IntlIterator_objects_free(zend_object *object) IntlIterator_object *ii = php_intl_iterator_fetch_object(object); if (ii->iterator) { - zval *wrapping_objp = &((zoi_with_current*)ii->iterator)->wrapping_obj; - ZVAL_UNDEF(wrapping_objp); - zend_iterator_dtor(ii->iterator); + ((zoi_with_current*)ii->iterator)->destroy_it(ii->iterator); } intl_error_reset(INTLITERATOR_ERROR_P(ii)); diff --git a/ext/intl/tests/gh11874.phpt b/ext/intl/tests/gh11874.phpt new file mode 100644 index 0000000000000..4b44cfb157be1 --- /dev/null +++ b/ext/intl/tests/gh11874.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-11874 (intl causing segfault in docker images) +--EXTENSIONS-- +intl +--FILE-- + 0); +echo "No crash\n"; + +?> +--EXPECT-- +string(7) "persian" +string(9) "gregorian" +string(7) "islamic" +string(13) "islamic-civil" +string(12) "islamic-tbla" +bool(true) +No crash