diff --git a/NEWS b/NEWS index 982945e5ebfc..8f6d2f3d7dda 100644 --- a/NEWS +++ b/NEWS @@ -52,6 +52,12 @@ PHP NEWS . Fixed socket_select() silently truncating sets larger than FD_SETSIZE on Windows. (David Carlier) +- SPL: + . Fixed SplFixedArray::setSize() doing nothing on subclasses whose + constructor does not call parent::__construct(). (Marc Bennewitz) + . Fixed memory leak when __construct(), __wakeup() or __unserialize() is + called from an element destructor during setSize(0). (Marc Bennewitz) + - SQLite: . Fixed a crash when SQLite3::close() is called from a userland callback. (Ilia Alshanetsky) diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 8f8108e2f90b..18025e0cbeb8 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -77,6 +77,14 @@ static bool spl_fixedarray_empty(spl_fixedarray *array) return true; } +/* True while spl_fixedarray_resize() runs. A clear empties the array before + * destroying its elements, so emptiness alone cannot tell "never constructed" + * from "clear in progress"; re-initialising in that window leaks. */ +static bool spl_fixedarray_resize_in_progress(const spl_fixedarray *array) +{ + return array->cached_resize >= 0; +} + static void spl_fixedarray_default_ctor(spl_fixedarray *array) { array->size = 0; @@ -188,9 +196,9 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) /* clearing the array */ if (size == 0) { + /* Clears elements and size; resetting them afterwards would leak + * anything a destructor re-installed. */ spl_fixedarray_dtor(array); - array->elements = NULL; - array->size = 0; } else if (size > array->size) { array->elements = safe_erealloc(array->elements, size, sizeof(zval), 0); spl_fixedarray_init_elems(array, array->size, size); @@ -201,8 +209,12 @@ static void spl_fixedarray_resize(spl_fixedarray *array, zend_long size) array->elements = erealloc(array->elements, sizeof(zval) * size); } - /* If resized within the destructor, take the last resize command and perform it */ + /* If resized within the destructor, take the last resize command and + * perform it. The sentinel is still set: re-initialising during a + * resize is refused. */ zend_long cached_resize = array->cached_resize; + ZEND_ASSERT(cached_resize >= 0); + array->cached_resize = -1; if (cached_resize != size) { spl_fixedarray_resize(array, cached_resize); @@ -285,6 +297,9 @@ static zend_object *spl_fixedarray_object_new_ex(zend_class_entry *class_type, z if (orig && clone_orig) { spl_fixedarray_object *other = spl_fixed_array_from_obj(orig); spl_fixedarray_copy_ctor(&intern->array, &other->array); + } else { + /* The zeroed struct would mean "resizing"; set the sentinel. */ + spl_fixedarray_default_ctor(&intern->array); } while (parent) { @@ -554,7 +569,7 @@ PHP_METHOD(SplFixedArray, __construct) intern = Z_SPLFIXEDARRAY_P(object); - if (!spl_fixedarray_empty(&intern->array)) { + if (UNEXPECTED(!spl_fixedarray_empty(&intern->array) || spl_fixedarray_resize_in_progress(&intern->array))) { /* called __construct() twice, bail out */ return; } @@ -572,7 +587,7 @@ PHP_METHOD(SplFixedArray, __wakeup) RETURN_THROWS(); } - if (intern->array.size == 0) { + if (EXPECTED(intern->array.size == 0 && !spl_fixedarray_resize_in_progress(&intern->array))) { int index = 0; int size = zend_hash_num_elements(intern_ht); @@ -634,7 +649,7 @@ PHP_METHOD(SplFixedArray, __unserialize) RETURN_THROWS(); } - if (intern->array.size == 0) { + if (EXPECTED(intern->array.size == 0 && !spl_fixedarray_resize_in_progress(&intern->array))) { size = zend_hash_num_elements(data); spl_fixedarray_init_non_empty_struct(&intern->array, size); if (!size) { diff --git a/ext/spl/tests/SplFixedArray_setSize_destruct_reinit_during_clear.phpt b/ext/spl/tests/SplFixedArray_setSize_destruct_reinit_during_clear.phpt new file mode 100644 index 000000000000..838bf30c8282 --- /dev/null +++ b/ext/spl/tests/SplFixedArray_setSize_destruct_reinit_during_clear.phpt @@ -0,0 +1,68 @@ +--TEST-- +SplFixedArray::setSize: re-initialising from a destructor during clear (GH-23811) +--DESCRIPTION-- +setSize(0) clears elements and size before running the element destructors, so +the array momentarily looks like it was never constructed. __construct(), +__wakeup() and __unserialize() must not re-initialise it in that window: the +in-progress clear would discard whatever they installed, leaking it. +--FILE-- +setSize(0); + echo "size: ", $arr->getSize(), "\n"; + + /* The array must still be usable. */ + $arr->setSize(1); + $arr[0] = "ok"; + var_dump($arr[0]); + + Reentrant::$arr = null; + Reentrant::$action = null; +} + +echo "-- __construct() --\n"; +clear_with(function ($arr) { $arr->__construct(5); }); + +/* __construct() is ignored, but the following setSize() is still recorded as + * the pending resize and applied once the clear finishes. */ +echo "-- __construct() then setSize() --\n"; +clear_with(function ($arr) { $arr->__construct(7); $arr->setSize(3); }); + +echo "-- __unserialize() --\n"; +clear_with(function ($arr) { $arr->__unserialize(["a", "b", "c"]); }); + +echo "-- __wakeup() --\n"; +clear_with(function ($arr) { @$arr->__wakeup(); }); +?> +--EXPECT-- +-- __construct() -- +size: 0 +string(2) "ok" +-- __construct() then setSize() -- +size: 3 +string(2) "ok" +-- __unserialize() -- +size: 0 +string(2) "ok" +-- __wakeup() -- +size: 0 +string(2) "ok" diff --git a/ext/spl/tests/SplFixedArray_subclass_without_parent_construct.phpt b/ext/spl/tests/SplFixedArray_subclass_without_parent_construct.phpt new file mode 100644 index 000000000000..969661b3dca4 --- /dev/null +++ b/ext/spl/tests/SplFixedArray_subclass_without_parent_construct.phpt @@ -0,0 +1,63 @@ +--TEST-- +SplFixedArray: subclass not calling parent::__construct() (GH-23811) +--DESCRIPTION-- +The internal struct is zeroed on object creation, which used to leave the +"resize in progress" sentinel at 0 instead of -1. setSize() then took the +re-entrancy early return and silently did nothing, leaving the array stuck +at size 0 for the lifetime of the object. +--FILE-- +getSize(), "\n"; + +$a->setSize(3); +echo "after setSize(3): ", $a->getSize(), "\n"; + +$a[0] = "x"; +$a[2] = "z"; +var_dump($a->toArray()); + +$a->setSize(1); +echo "after setSize(1): ", $a->getSize(), "\n"; + +$a->setSize(0); +echo "after setSize(0): ", $a->getSize(), "\n"; + +/* Deferred initialisation: calling the parent constructor later still works. */ +class LateInit extends SplFixedArray { + public function __construct() { + } + public function init(int $size): void { + parent::__construct($size); + } +} +$b = new LateInit(); +$b->init(2); +echo "deferred parent::__construct(2): ", $b->getSize(), "\n"; + +/* Cloning one of these must also yield a resizable array. */ +$c = clone new Unconstructed(); +$c->setSize(2); +echo "clone then setSize(2): ", $c->getSize(), "\n"; +?> +--EXPECT-- +initial: 0 +after setSize(3): 3 +array(3) { + [0]=> + string(1) "x" + [1]=> + NULL + [2]=> + string(1) "z" +} +after setSize(1): 1 +after setSize(0): 0 +deferred parent::__construct(2): 2 +clone then setSize(2): 2