Skip to content

Commit 7bf6cf1

Browse files
morrisonlevimorrisonlevi
committed
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.
1 parent a9f6572 commit 7bf6cf1

7 files changed

+278
-13
lines changed

‎ext/spl/spl_fixedarray.c‎

Lines changed: 121 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -195,23 +195,18 @@ static HashTable* spl_fixedarray_object_get_gc(zend_object *obj, zval **table, i
195195
return ht;
196196
}
197197

198-
static HashTable* spl_fixedarray_object_get_properties(zend_object *obj)
198+
static HashTable* spl_fixedarray_get_properties_for(zend_object *obj, zend_prop_purpose purpose)
199199
{
200200
spl_fixedarray_object *intern = spl_fixed_array_from_obj(obj);
201-
HashTable *ht = zend_std_get_properties(obj);
202201

203-
if (!spl_fixedarray_empty(&intern->array)) {
204-
zend_long j = zend_hash_num_elements(ht);
202+
/* Keep the values and properties separate*/
203+
HashTable *ht = zend_array_dup(zend_std_get_properties(obj));
205204

205+
if (!spl_fixedarray_empty(&intern->array)) {
206206
for (zend_long i = 0; i < intern->array.size; i++) {
207-
zend_hash_index_update(ht, i, &intern->array.elements[i]);
207+
zend_hash_index_add_new(ht, i, &intern->array.elements[i]);
208208
Z_TRY_ADDREF(intern->array.elements[i]);
209209
}
210-
if (j > intern->array.size) {
211-
for (zend_long i = intern->array.size; i < j; ++i) {
212-
zend_hash_index_del(ht, i);
213-
}
214-
}
215210
}
216211

217212
return ht;
@@ -628,7 +623,6 @@ PHP_METHOD(SplFixedArray, fromArray)
628623
} else if (num > 0 && !save_indexes) {
629624
zval *element;
630625
zend_long i = 0;
631-
632626
spl_fixedarray_init(&array, num);
633627

634628
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(data), element) {
@@ -754,6 +748,120 @@ PHP_METHOD(SplFixedArray, getIterator)
754748
zend_create_internal_iterator_zval(return_value, ZEND_THIS);
755749
}
756750

751+
/* Assumes the object has been created and the spl_fixedarray_object's
752+
* array member is uninitialized or zero-initialized.
753+
* The state can have string keys, but they must come prior to integers:
754+
*
755+
* SplFixedArray::__set_state(array(
756+
* 'property' => 'value',
757+
* 0 => 1,
758+
* 1 => 2,
759+
* 2 => 3,
760+
* ))
761+
*/
762+
static zend_result spl_fixedarray_import(zend_object *object, HashTable *ht)
763+
{
764+
spl_fixedarray_object *intern = spl_fixed_array_from_obj(object);
765+
spl_fixedarray *fixed = &intern->array;
766+
767+
spl_fixedarray_init(fixed, 0);
768+
769+
/* For performance we do a single pass over the input array and the
770+
* spl_fixedarray elems. This complicates the implementation, but part
771+
* of the reason for choosing SplFixedArray is for peformance, and
772+
* although that is primarily for memory we should still care about CPU.
773+
*
774+
* We need to track the number of string keys, which must come before
775+
* all the integer keys. This way the total size of the input minus the
776+
* number of string keys equals the number of integer keys, so we can
777+
* allocate the spl_fixedarray.elements and do this in a single pass.
778+
*/
779+
780+
// The total size of the input array
781+
const zend_long nht = zend_hash_num_elements(ht);
782+
zend_long nstrings = 0; // The number of string keys
783+
zend_long nints; // will be nht - nstrings
784+
785+
zend_string *str_idx = NULL; // current string key of input array
786+
zend_ulong num_idx = 0; // current int key (valid iff str_idx == NULL)
787+
zval *val = NULL; // current value of input array
788+
zend_long max_idx = -1; // the largest index found so far
789+
zval *begin = NULL; // pointer to beginning of fixedarray's storage
790+
zval *end = NULL; // points one element passed the last element
791+
const char *ex_msg = NULL; // message for the value exception
792+
793+
ZEND_HASH_FOREACH_KEY_VAL(ht, num_idx, str_idx, val) {
794+
if (str_idx != NULL) {
795+
if (UNEXPECTED(max_idx >= 0)) {
796+
ex_msg = "must have all its string keys come before all integer keys";
797+
goto value_error;
798+
}
799+
800+
++nstrings;
801+
object->handlers->write_property(object, str_idx, val, NULL);
802+
803+
} else if (UNEXPECTED((zend_long)num_idx != ++max_idx)) {
804+
ex_msg = "did not have integer keys that start at 0 and increment sequentially";
805+
goto value_error;
806+
807+
} else {
808+
if (UNEXPECTED(max_idx == 0)) {
809+
nints = nht - nstrings;
810+
fixed->size = nints;
811+
fixed->elements =
812+
safe_emalloc(nints, sizeof(zval), 0);
813+
begin = fixed->elements;
814+
end = fixed->elements + nints;
815+
}
816+
817+
ZEND_ASSERT(num_idx == max_idx);
818+
ZEND_ASSERT(begin != end);
819+
820+
ZVAL_COPY(begin++, val);
821+
}
822+
} ZEND_HASH_FOREACH_END();
823+
824+
ZEND_ASSERT(begin == end);
825+
return SUCCESS;
826+
827+
value_error:
828+
spl_fixedarray_dtor_range(fixed, 0, max_idx);
829+
if (fixed->elements) {
830+
efree(fixed->elements);
831+
}
832+
833+
/* Zero-initialize so the object release is valid. */
834+
spl_fixedarray_init(fixed, 0);
835+
zend_object_release(object);
836+
837+
zend_argument_value_error(1, ex_msg);
838+
return FAILURE;
839+
}
840+
841+
PHP_METHOD(SplFixedArray, __set_state)
842+
{
843+
zval *state = NULL;
844+
845+
ZEND_PARSE_PARAMETERS_START(1, 1)
846+
Z_PARAM_ARRAY(state);
847+
ZEND_PARSE_PARAMETERS_END();
848+
849+
HashTable *ht = Z_ARRVAL_P(state);
850+
zend_class_entry *called_scope = zend_get_called_scope(execute_data);
851+
852+
zend_result result = object_init_ex(return_value, called_scope);
853+
if (UNEXPECTED(result != SUCCESS)) {
854+
ZVAL_NULL(return_value);
855+
RETURN_THROWS();
856+
}
857+
858+
zend_object *object = Z_OBJ_P(return_value);
859+
if (UNEXPECTED(spl_fixedarray_import(object, ht) != SUCCESS)) {
860+
ZVAL_NULL(return_value);
861+
RETURN_THROWS();
862+
}
863+
}
864+
757865
static void spl_fixedarray_it_dtor(zend_object_iterator *iter)
758866
{
759867
zval_ptr_dtor(&iter->data);
@@ -844,7 +952,8 @@ PHP_MINIT_FUNCTION(spl_fixedarray)
844952
spl_handler_SplFixedArray.unset_dimension = spl_fixedarray_object_unset_dimension;
845953
spl_handler_SplFixedArray.has_dimension = spl_fixedarray_object_has_dimension;
846954
spl_handler_SplFixedArray.count_elements = spl_fixedarray_object_count_elements;
847-
spl_handler_SplFixedArray.get_properties = spl_fixedarray_object_get_properties;
955+
spl_handler_SplFixedArray.get_properties_for
956+
= spl_fixedarray_get_properties_for;
848957
spl_handler_SplFixedArray.get_gc = spl_fixedarray_object_get_gc;
849958
spl_handler_SplFixedArray.dtor_obj = zend_objects_destroy_object;
850959
spl_handler_SplFixedArray.free_obj = spl_fixedarray_object_free_storage;

‎ext/spl/spl_fixedarray.stub.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,7 @@ public function offsetSet($index, mixed $value) {}
4949
public function offsetUnset($index) {}
5050

5151
public function getIterator(): Iterator {}
52+
53+
/** @return SplFixedArray */
54+
public static function __set_state(array $state) {}
5255
}

‎ext/spl/spl_fixedarray_arginfo.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/* This is a generated file, edit the .stub.php file instead.
2-
* Stub hash: 831fe70055eb62135ae49321e5e5f3fe08c3d95f */
2+
* Stub hash: 54028c566f2d868da1f91e5f4f627fbe6e823bb5 */
33

44
ZEND_BEGIN_ARG_INFO_EX(arginfo_class_SplFixedArray___construct, 0, 0, 0)
55
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, size, IS_LONG, 0, "0")
@@ -39,6 +39,10 @@ ZEND_END_ARG_INFO()
3939
ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_class_SplFixedArray_getIterator, 0, 0, Iterator, 0)
4040
ZEND_END_ARG_INFO()
4141

42+
ZEND_BEGIN_ARG_INFO_EX(arginfo_class_SplFixedArray___set_state, 0, 0, 1)
43+
ZEND_ARG_TYPE_INFO(0, state, IS_ARRAY, 0)
44+
ZEND_END_ARG_INFO()
45+
4246

4347
ZEND_METHOD(SplFixedArray, __construct);
4448
ZEND_METHOD(SplFixedArray, __wakeup);
@@ -52,6 +56,7 @@ ZEND_METHOD(SplFixedArray, offsetGet);
5256
ZEND_METHOD(SplFixedArray, offsetSet);
5357
ZEND_METHOD(SplFixedArray, offsetUnset);
5458
ZEND_METHOD(SplFixedArray, getIterator);
59+
ZEND_METHOD(SplFixedArray, __set_state);
5560

5661

5762
static const zend_function_entry class_SplFixedArray_methods[] = {
@@ -67,5 +72,6 @@ static const zend_function_entry class_SplFixedArray_methods[] = {
6772
ZEND_ME(SplFixedArray, offsetSet, arginfo_class_SplFixedArray_offsetSet, ZEND_ACC_PUBLIC)
6873
ZEND_ME(SplFixedArray, offsetUnset, arginfo_class_SplFixedArray_offsetUnset, ZEND_ACC_PUBLIC)
6974
ZEND_ME(SplFixedArray, getIterator, arginfo_class_SplFixedArray_getIterator, ZEND_ACC_PUBLIC)
75+
ZEND_ME(SplFixedArray, __set_state, arginfo_class_SplFixedArray___set_state, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
7076
ZEND_FE_END
7177
};

‎ext/spl/tests/ArrayObject_overloaded_object_incompatible.phpt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
--TEST--
22
Objects with overloaded get_properties are incompatible with ArrayObject
3+
--XFAIL--
4+
SplFixedArray has migrated to get_properties_for; find new test case
35
--FILE--
46
<?php
57

‎ext/spl/tests/spl_fixedarray___set_state.phpt‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
--TEST--
2+
SplFixedArray: __set_state
3+
--FILE--
4+
<?php
5+
6+
$fixed = SplFixedArray::__set_state([1, 2, 3]);
7+
var_export($fixed);
8+
echo "\n---\n";
9+
10+
// dynamic properties
11+
$fixed->undefined = 'undef';
12+
var_export($fixed);
13+
echo "\n---\n";
14+
15+
// inheritance
16+
class Vec
17+
extends SplFixedArray
18+
{
19+
public $type;
20+
}
21+
$vec = new Vec(3);
22+
$vec[0] = 1;
23+
$vec[1] = 2;
24+
$vec[2] = 3;
25+
$vec->type = 'int';
26+
var_export($vec);
27+
echo "\n---\n";
28+
29+
$vec->undefined = 'undef';
30+
var_export($vec);
31+
32+
?>
33+
--EXPECT--
34+
SplFixedArray::__set_state(array(
35+
0 => 1,
36+
1 => 2,
37+
2 => 3,
38+
))
39+
---
40+
SplFixedArray::__set_state(array(
41+
'undefined' => 'undef',
42+
0 => 1,
43+
1 => 2,
44+
2 => 3,
45+
))
46+
---
47+
Vec::__set_state(array(
48+
'type' => 'int',
49+
0 => 1,
50+
1 => 2,
51+
2 => 3,
52+
))
53+
---
54+
Vec::__set_state(array(
55+
'type' => 'int',
56+
'undefined' => 'undef',
57+
0 => 1,
58+
1 => 2,
59+
2 => 3,
60+
))

‎ext/spl/tests/spl_fixedarray___set_state_errors.phpt‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
--TEST--
2+
SplFixedArray: __set_state
3+
--FILE--
4+
<?php
5+
6+
try {
7+
$fixed = SplFixedArray::__set_state([1 => 2]);
8+
} catch (ValueError $error) {
9+
var_dump($error->getMessage());
10+
}
11+
12+
try {
13+
$fixed = SplFixedArray::__set_state([0 => 1, 1 => 2, 3 => 4]);
14+
} catch (ValueError $error) {
15+
var_dump($error->getMessage());
16+
}
17+
18+
try {
19+
$fixed = SplFixedArray::__set_state([-1 => 2]);
20+
} catch (ValueError $error) {
21+
var_dump($error->getMessage());
22+
}
23+
24+
echo "-----\n";
25+
26+
try {
27+
$fixed = SplFixedArray::__set_state([0 => 1, 'type' => 'undefined']);
28+
} catch (ValueError $error) {
29+
var_dump($error->getMessage());
30+
}
31+
--EXPECTF--
32+
string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially"
33+
string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially"
34+
string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) did not have integer keys that start at 0 and increment sequentially"
35+
-----
36+
string(%d) "SplFixedArray::__set_state(): Argument #1 ($state) must have all its string keys come before all integer keys"

‎ext/spl/tests/spl_fixedarray_export_import.phpt‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
--TEST--
2+
SplFixedArray: __set_state export and import
3+
--FILE--
4+
<?php
5+
6+
function compare_export($var, string $case) {
7+
$exported = var_export($var, true);
8+
9+
$imported = eval("return {$exported};");
10+
$reexported = var_export($imported, true);
11+
12+
if ($exported == $reexported) {
13+
echo "Pass {$case}.\n";
14+
} else {
15+
echo "Fail {$case}.\nExpected:\n{$exported}\nActual:\n{$reexported}\n";
16+
}
17+
}
18+
19+
// simple
20+
$fixed = SplFixedArray::__set_state([1, 2, 3]);
21+
compare_export($fixed, 'simple');
22+
23+
// dynamic properties
24+
$fixed->undefined = 'undef';
25+
compare_export($fixed, 'dynamic properties');
26+
27+
// inheritance
28+
class Vec
29+
extends SplFixedArray
30+
{
31+
public $type;
32+
}
33+
$vec = new Vec(3);
34+
$vec[0] = 1;
35+
$vec[1] = 2;
36+
$vec[2] = 3;
37+
$vec->type = 'int';
38+
39+
compare_export($vec, 'inheritance');
40+
41+
// dynamic properties and inheritance
42+
$vec->undefined = 'undef';
43+
compare_export($vec, 'dynamic properties and inheritance');
44+
45+
--EXPECT--
46+
Pass simple.
47+
Pass dynamic properties.
48+
Pass inheritance.
49+
Pass dynamic properties and inheritance.

0 commit comments

Comments
 (0)