From 7bf6cf11cb830275c03dfbfcff446fa9cd62b633 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 1 Oct 2020 20:52:04 -0600 Subject: [PATCH 1/2] Add SplFixedArray::__set_state 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. --- ext/spl/spl_fixedarray.c | 133 ++++++++++++++++-- ext/spl/spl_fixedarray.stub.php | 3 + ext/spl/spl_fixedarray_arginfo.h | 8 +- ...Object_overloaded_object_incompatible.phpt | 2 + ext/spl/tests/spl_fixedarray___set_state.phpt | 60 ++++++++ .../spl_fixedarray___set_state_errors.phpt | 36 +++++ .../tests/spl_fixedarray_export_import.phpt | 49 +++++++ 7 files changed, 278 insertions(+), 13 deletions(-) create mode 100644 ext/spl/tests/spl_fixedarray___set_state.phpt create mode 100644 ext/spl/tests/spl_fixedarray___set_state_errors.phpt create mode 100644 ext/spl/tests/spl_fixedarray_export_import.phpt diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 4b0d186a78d28..8156d360d5aba 100644 --- 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,120 @@ PHP_METHOD(SplFixedArray, getIterator) zend_create_internal_iterator_zval(return_value, ZEND_THIS); } +/* Assumes the object has been created and the spl_fixedarray_object's + * array member is uninitialized or zero-initialized. + * The state can have string keys, but they must come prior to integers: + * + * SplFixedArray::__set_state(array( + * 'property' => 'value', + * 0 => 1, + * 1 => 2, + * 2 => 3, + * )) + */ +static zend_result spl_fixedarray_import(zend_object *object, HashTable *ht) +{ + spl_fixedarray_object *intern = spl_fixed_array_from_obj(object); + spl_fixedarray *fixed = &intern->array; + + 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_object_release(object); + + zend_argument_value_error(1, ex_msg); + return FAILURE; +} + +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 +952,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 index f2bb378f7aa1d..da13d88c03187 100644 --- 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 index 6067a3ebdeb1f..22fa5e8b81076 100644 --- 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: 831fe70055eb62135ae49321e5e5f3fe08c3d95f */ + * Stub hash: 54028c566f2d868da1f91e5f4f627fbe6e823bb5 */ 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 index 8c1121b8d01ad..dc703c4b02d93 100644 --- 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-- 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 new file mode 100644 index 0000000000000..eebcdd8eb83fd --- /dev/null +++ b/ext/spl/tests/spl_fixedarray___set_state_errors.phpt @@ -0,0 +1,36 @@ +--TEST-- +SplFixedArray: __set_state +--FILE-- + 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 new file mode 100644 index 0000000000000..522438002e1a0 --- /dev/null +++ b/ext/spl/tests/spl_fixedarray_export_import.phpt @@ -0,0 +1,49 @@ +--TEST-- +SplFixedArray: __set_state export and import +--FILE-- +undefined = 'undef'; +compare_export($fixed, 'dynamic properties'); + +// inheritance +class Vec + extends SplFixedArray +{ + public $type; +} +$vec = new Vec(3); +$vec[0] = 1; +$vec[1] = 2; +$vec[2] = 3; +$vec->type = 'int'; + +compare_export($vec, 'inheritance'); + +// dynamic properties and inheritance +$vec->undefined = 'undef'; +compare_export($vec, 'dynamic properties and inheritance'); + +--EXPECT-- +Pass simple. +Pass dynamic properties. +Pass inheritance. +Pass dynamic properties and inheritance. From 1b93e4ace145b558a528eb2195a9086ac6c94946 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 9 Oct 2020 21:37:31 -0600 Subject: [PATCH 2/2] Add SplFixedArray::__set_state to NEWS --- NEWS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS b/NEWS index 5fc105a2f1ddd..feccb58c3d764 100644 --- a/NEWS +++ b/NEWS @@ -11,5 +11,8 @@ PHP NEWS - PSpell: . Convert resource to object \PSpell. (Sara) . Convert resource to object \PSPellConfig. (Sara) +- SPL: + . Add SplFixedArray::__set_state (FR #75268) (levim). Note this converts + SplFixedArray from get_properties to get_properties_for. <<< NOTE: Insert NEWS from last stable release here prior to actual release! >>>