Add SplFixedArray::__set_state

morrisonlevi · morrisonlevi · commit d11e94e382ec · 2020-10-04T17:42:20.000-06:00
Fixes bug 75268. The design is that all string keys must come before all integer keys. The string keys become properties and the integer keys become values in the array. Other serialization/unserialization methods should adopt this same strategy as there are bugs: https://3v4l.org/IMnI5 Migrate SplFixedArray to get_properties_for. Extract spl_fixedarray_import, as I expect to reuse this code for other cases as well, such as the serialization bug mentioned above.
diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c
@@ -195,23 +195,18 @@ static HashTable* spl_fixedarray_object_get_gc(zend_object *obj, zval **table, i
 	return ht;
 }
 
-static HashTable* spl_fixedarray_object_get_properties(zend_object *obj)
+static HashTable* spl_fixedarray_get_properties_for(zend_object *obj, zend_prop_purpose purpose)
 {
 	spl_fixedarray_object *intern = spl_fixed_array_from_obj(obj);
-	HashTable *ht = zend_std_get_properties(obj);
 
-	if (!spl_fixedarray_empty(&intern->array)) {
-		zend_long j = zend_hash_num_elements(ht);
+	/* Keep the values and properties separate*/
+	HashTable *ht = zend_array_dup(zend_std_get_properties(obj));
 
+	if (!spl_fixedarray_empty(&intern->array)) {
 		for (zend_long i = 0; i < intern->array.size; i++) {
-			zend_hash_index_update(ht, i, &intern->array.elements[i]);
+			zend_hash_index_add_new(ht, i, &intern->array.elements[i]);
 			Z_TRY_ADDREF(intern->array.elements[i]);
 		}
-		if (j > intern->array.size) {
-			for (zend_long i = intern->array.size; i < j; ++i) {
-				zend_hash_index_del(ht, i);
-			}
-		}
 	}
 
 	return ht;
@@ -628,7 +623,6 @@ PHP_METHOD(SplFixedArray, fromArray)
 	} else if (num > 0 && !save_indexes) {
 		zval *element;
 		zend_long i = 0;
-
 		spl_fixedarray_init(&array, num);
 
 		ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(data), element) {
@@ -754,6 +748,182 @@ PHP_METHOD(SplFixedArray, getIterator)
 	zend_create_internal_iterator_zval(return_value, ZEND_THIS);
 }
 
+static zend_result spl_fixedarray_import_packed(zend_object *object, HashTable *ht)
+{
+	ZEND_ASSERT(HT_FLAGS(ht) & HASH_FLAG_PACKED);
+
+	spl_fixedarray_object *intern = spl_fixed_array_from_obj(object);
+	spl_fixedarray *fixed = &intern->array;
+
+	uint32_t n = zend_hash_num_elements(ht);
+	zend_ulong num_idx = 0, i = 0;
+
+	/* This will allocate memory even if the value checks fail, but allows
+	 * a single iteration over the input array.
+	 * It's also manually initialized to do a single pass over the output.
+	 * Be sure to dtor and free the correct range if there's an error during
+	 * the construction process.
+	 */
+	fixed->size = n;
+	fixed->elements = safe_emalloc(n, sizeof(zval), 0);
+	zval *begin = fixed->elements, *end = fixed->elements + n;
+	zval *val = NULL;
+
+	ZEND_HASH_FOREACH_NUM_KEY_VAL(ht, num_idx, val) {
+		if (UNEXPECTED(num_idx != i)) {
+			spl_fixedarray_dtor_range(fixed, 0, i);
+			efree(fixed->elements);
+
+			/* Zero-initialize so the object release is valid. */
+			spl_fixedarray_init(fixed, 0);
+
+			zend_argument_value_error(1,
+				"did not have integer keys that start at 0 and increment sequentially");
+			return FAILURE;
+		}
+
+		i += 1;
+
+		ZEND_ASSERT(begin != end);
+		ZVAL_COPY(begin++, val);
+	} ZEND_HASH_FOREACH_END();
+
+	ZEND_ASSERT(begin == end);
+
+	return SUCCESS;
+}
+
+static zend_result spl_fixedarray_import_not_packed(zend_object *object, HashTable *ht)
+{
+	ZEND_ASSERT(!(HT_FLAGS(ht) & HASH_FLAG_PACKED));
+
+	spl_fixedarray_object *intern = spl_fixed_array_from_obj(object);
+	spl_fixedarray *fixed = &intern->array;
+
+	/* The array can have string keys, but they must come prior to integers:
+	 *
+	 * 	SplFixedArray::__set_state(array(
+	 * 	   'property' => 'value',
+	 * 	   0 => 1,
+	 * 	   1 => 2,
+	 * 	   2 => 3,
+	 * 	))
+	 */
+	spl_fixedarray_init(fixed, 0);
+
+	/* For performance we do a single pass over the input array and the
+	 * spl_fixedarray elems. This complicates the implementation, but part
+	 * of the reason for choosing SplFixedArray is for peformance, and
+	 * although that is primarily for memory we should still care about CPU.
+	 *
+	 * We need to track the number of string keys, which must come before
+	 * all the integer keys. This way the total size of the input minus the
+	 * number of string keys equals the number of integer keys, so we can
+	 * allocate the spl_fixedarray.elements and do this in a single pass.
+	 */
+
+	// The total size of the input array
+	const zend_long nht = zend_hash_num_elements(ht);
+	zend_long nstrings = 0; // The number of string keys
+	zend_long nints; // will be nht - nstrings
+
+	zend_string *str_idx = NULL; // current string key of input array
+	zend_ulong num_idx = 0; // current int key (valid iff str_idx == NULL)
+	zval *val = NULL; // current value of input array
+	zend_long max_idx = -1; // the largest index found so far
+	zval *begin = NULL; // pointer to beginning of fixedarray's storage
+	zval *end = NULL; // points one element passed the last element
+	const char *ex_msg = NULL; // message for the value exception
+
+	ZEND_HASH_FOREACH_KEY_VAL(ht, num_idx, str_idx, val) {
+		if (str_idx != NULL) {
+			if (UNEXPECTED(max_idx >= 0)) {
+				ex_msg = "must have all its string keys come before all integer keys";
+				goto value_error;
+			}
+
+			++nstrings;
+			object->handlers->write_property(object, str_idx, val, NULL);
+
+		} else if (UNEXPECTED((zend_long)num_idx != ++max_idx)) {
+			ex_msg = "did not have integer keys that start at 0 and increment sequentially";
+			goto value_error;
+
+		} else {
+			if (UNEXPECTED(max_idx == 0)) {
+				nints = nht - nstrings;
+				fixed->size = nints;
+				fixed->elements =
+					safe_emalloc(nints, sizeof(zval), 0);
+				begin = fixed->elements;
+				end = fixed->elements + nints;
+			}
+
+			ZEND_ASSERT(num_idx == max_idx);
+			ZEND_ASSERT(begin != end);
+
+			ZVAL_COPY(begin++, val);
+		}
+	} ZEND_HASH_FOREACH_END();
+
+	ZEND_ASSERT(begin == end);
+	return SUCCESS;
+
+value_error:
+	spl_fixedarray_dtor_range(fixed, 0, max_idx);
+	if (fixed->elements) {
+		efree(fixed->elements);
+	}
+
+	/* Zero-initialize so the object release is valid. */
+	spl_fixedarray_init(fixed, 0);
+
+	zend_argument_value_error(1, ex_msg);
+	return FAILURE;
+}
+
+/* Assumes the object has been created and the spl_fixedarray_object's
+ * array member is uninitialized or zero-initialized.
+ */
+static zend_result spl_fixedarray_import(zend_object *object, HashTable *ht)
+{
+
+	/* if result != SUCCESS then these need to dtor their contents. */
+	zend_result result = (EXPECTED((HT_FLAGS(ht) & HASH_FLAG_PACKED)))
+		? spl_fixedarray_import_packed(object, ht)
+		: spl_fixedarray_import_not_packed(object, ht);
+
+	if (UNEXPECTED(result != SUCCESS)) {
+		zend_object_release(object);
+	}
+
+	return result;
+}
+
+PHP_METHOD(SplFixedArray, __set_state)
+{
+	zval *state = NULL;
+
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_ARRAY(state);
+	ZEND_PARSE_PARAMETERS_END();
+
+	HashTable *ht = Z_ARRVAL_P(state);
+	zend_class_entry *called_scope = zend_get_called_scope(execute_data);
+
+	zend_result result = object_init_ex(return_value, called_scope);
+	if (UNEXPECTED(result != SUCCESS)) {
+		ZVAL_NULL(return_value);
+		RETURN_THROWS();
+	}
+
+	zend_object *object = Z_OBJ_P(return_value);
+	if (UNEXPECTED(spl_fixedarray_import(object, ht) != SUCCESS)) {
+		ZVAL_NULL(return_value);
+		RETURN_THROWS();
+	}
+}
+
 static void spl_fixedarray_it_dtor(zend_object_iterator *iter)
 {
 	zval_ptr_dtor(&iter->data);
@@ -844,7 +1014,8 @@ PHP_MINIT_FUNCTION(spl_fixedarray)
 	spl_handler_SplFixedArray.unset_dimension = spl_fixedarray_object_unset_dimension;
 	spl_handler_SplFixedArray.has_dimension   = spl_fixedarray_object_has_dimension;
 	spl_handler_SplFixedArray.count_elements  = spl_fixedarray_object_count_elements;
-	spl_handler_SplFixedArray.get_properties  = spl_fixedarray_object_get_properties;
+	spl_handler_SplFixedArray.get_properties_for
+		= spl_fixedarray_get_properties_for;
 	spl_handler_SplFixedArray.get_gc          = spl_fixedarray_object_get_gc;
 	spl_handler_SplFixedArray.dtor_obj        = zend_objects_destroy_object;
 	spl_handler_SplFixedArray.free_obj        = spl_fixedarray_object_free_storage;
diff --git a/ext/spl/spl_fixedarray.stub.php b/ext/spl/spl_fixedarray.stub.php
@@ -49,4 +49,7 @@ public function offsetSet($index, mixed $value) {}
     public function offsetUnset($index) {}
 
     public function getIterator(): Iterator {}
+
+    /** @return SplFixedArray */
+    public static function __set_state(array $state) {}
 }
diff --git a/ext/spl/spl_fixedarray_arginfo.h b/ext/spl/spl_fixedarray_arginfo.h
@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: c820bad6bcfcc7c60a221464008a882515b5c05e */
+ * Stub hash: 7578bd2365447aba8ce9fcf8c11a768d2aece034 */
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_class_SplFixedArray___construct, 0, 0, 0)
 	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, size, IS_LONG, 0, "0")
@@ -39,6 +39,10 @@ ZEND_END_ARG_INFO()
 ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_class_SplFixedArray_getIterator, 0, 0, Iterator, 0)
 ZEND_END_ARG_INFO()
 
+ZEND_BEGIN_ARG_INFO_EX(arginfo_class_SplFixedArray___set_state, 0, 0, 1)
+	ZEND_ARG_TYPE_INFO(0, state, IS_ARRAY, 0)
+ZEND_END_ARG_INFO()
+
 
 ZEND_METHOD(SplFixedArray, __construct);
 ZEND_METHOD(SplFixedArray, __wakeup);
@@ -52,6 +56,7 @@ ZEND_METHOD(SplFixedArray, offsetGet);
 ZEND_METHOD(SplFixedArray, offsetSet);
 ZEND_METHOD(SplFixedArray, offsetUnset);
 ZEND_METHOD(SplFixedArray, getIterator);
+ZEND_METHOD(SplFixedArray, __set_state);
 
 
 static const zend_function_entry class_SplFixedArray_methods[] = {
@@ -67,5 +72,6 @@ static const zend_function_entry class_SplFixedArray_methods[] = {
 	ZEND_ME(SplFixedArray, offsetSet, arginfo_class_SplFixedArray_offsetSet, ZEND_ACC_PUBLIC)
 	ZEND_ME(SplFixedArray, offsetUnset, arginfo_class_SplFixedArray_offsetUnset, ZEND_ACC_PUBLIC)
 	ZEND_ME(SplFixedArray, getIterator, arginfo_class_SplFixedArray_getIterator, ZEND_ACC_PUBLIC)
+	ZEND_ME(SplFixedArray, __set_state, arginfo_class_SplFixedArray___set_state, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
 	ZEND_FE_END
 };
diff --git a/ext/spl/tests/ArrayObject_overloaded_object_incompatible.phpt b/ext/spl/tests/ArrayObject_overloaded_object_incompatible.phpt
@@ -1,5 +1,7 @@
 --TEST--
 Objects with overloaded get_properties are incompatible with ArrayObject
+--XFAIL--
+SplFixedArray has migrated to get_properties_for; find new test case
 --FILE--
 <?php
 
diff --git a/ext/spl/tests/spl_fixedarray___set_state.phpt b/ext/spl/tests/spl_fixedarray___set_state.phpt
@@ -0,0 +1,60 @@
+--TEST--
+SplFixedArray: __set_state
+--FILE--
+<?php
+
+$fixed = SplFixedArray::__set_state([1, 2, 3]);
+var_export($fixed);
+echo "\n---\n";
+
+// dynamic properties
+$fixed->undefined = 'undef';
+var_export($fixed);
+echo "\n---\n";
+
+// inheritance
+class Vec
+    extends SplFixedArray
+{
+    public $type;
+}
+$vec = new Vec(3);
+$vec[0] = 1;
+$vec[1] = 2;
+$vec[2] = 3;
+$vec->type = 'int';
+var_export($vec);
+echo "\n---\n";
+
+$vec->undefined = 'undef';
+var_export($vec);
+
+?>
+--EXPECT--
+SplFixedArray::__set_state(array(
+   0 => 1,
+   1 => 2,
+   2 => 3,
+))
+---
+SplFixedArray::__set_state(array(
+   'undefined' => 'undef',
+   0 => 1,
+   1 => 2,
+   2 => 3,
+))
+---
+Vec::__set_state(array(
+   'type' => 'int',
+   0 => 1,
+   1 => 2,
+   2 => 3,
+))
+---
+Vec::__set_state(array(
+   'type' => 'int',
+   'undefined' => 'undef',
+   0 => 1,
+   1 => 2,
+   2 => 3,
+))
diff --git a/ext/spl/tests/spl_fixedarray___set_state_errors.phpt b/ext/spl/tests/spl_fixedarray___set_state_errors.phpt
@@ -0,0 +1,36 @@
+--TEST--
+SplFixedArray: __set_state
+--FILE--
+<?php
+
+try {
+    $fixed = SplFixedArray::__set_state([1 => 2]);
+} catch (ValueError $error) {
+    var_dump($error->getMessage());
+}
+
+try {
+    $fixed = SplFixedArray::__set_state([0 => 1, 1 => 2, 3 => 4]);
+} catch (ValueError $error) {
+    var_dump($error->getMessage());
+}
+
+try {
+    $fixed = SplFixedArray::__set_state([-1 => 2]);
+} catch (ValueError $error) {
+    var_dump($error->getMessage());
+}
+
+echo "-----\n";
+
+try {
+    $fixed = SplFixedArray::__set_state([0 => 1, 'type' => 'undefined']);
+} catch (ValueError $error) {
+    var_dump($error->getMessage());
+}
+--EXPECTF--
+string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially"
+string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially"
+string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially"
+-----
+string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) must have all its string keys come before all integer keys"
diff --git a/ext/spl/tests/spl_fixedarray_export_import.phpt b/ext/spl/tests/spl_fixedarray_export_import.phpt

Original file line number	Diff line number	Diff line change
`@@ -49,4 +49,7 @@ public function offsetSet($index, mixed $value) {}`
`49`	`49`	`public function offsetUnset($index) {}`
`50`	`50`
`51`	`51`	`public function getIterator(): Iterator {}`
	`52`	`+`
	`53`	`+ /** @return SplFixedArray */`
	`54`	`+ public static function __set_state(array $state) {}`
`52`	`55`	`}`
-Original file line number
+Diff line change
@@ @@ -1,5 +1,7 @@ @@
 --TEST--
 Objects with overloaded get_properties are incompatible with ArrayObject
 +--XFAIL--
 +SplFixedArray has migrated to get_properties_for; find new test case
 --FILE--
 <?php