Fix GH-23811: SplFixedArray leak on re-init and broken setSize() on subclasses

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)

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) {

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--
+<?php
+class Reentrant {
+    public static $arr = null;
+    public static $action = null;
+    public function __destruct() {
+        if (self::$action === null) {
+            return;
+        }
+        $fn = self::$action;
+        self::$action = null;
+        $fn(self::$arr);
+    }
+}
+
+function clear_with(callable $action): void {
+    $arr = new SplFixedArray(2);
+    $arr[0] = new Reentrant();
+    $arr[1] = "tail";
+    Reentrant::$arr = $arr;
+    Reentrant::$action = $action;
+
+    $arr->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"

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--
+<?php
+class Unconstructed extends SplFixedArray {
+    public function __construct() {
+        /* deliberately does not call parent::__construct() */
+    }
+}
+
+$a = new Unconstructed();
+echo "initial: ", $a->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