php 8.2.0-dev crashes with assertion for cloning/get_object_vars on non-empty SplFixedArray (8.1 unaffected)

[TysonAndre]

Description

The following code:

<?php
php > $x = new SplFixedArray(/*count*/ 1);
php > var_export((array)$x);
array (
  0 => NULL,
)
php > $y = clone $x;

Resulted in this output (in a debug build - php 8.1 is unaffected), crashes in nts build for larger arrays:

php: /path/to/php-src/Zend/zend_objects.c:241: zend_objects_clone_members: Assertion `!(((__ht)->u.flags & (1<<2)) != 0)' failed.
[1]    126358 abort (core dumped)  php -a

But I expected this output instead:

Not crashing with a ZEND_ASSERT failure or invalid memory access

---

Similar bugs affect Zend/zend_builtin_functions.c get_object_vars()

php > $x = new SplFixedArray(1);
php > $x[0] = $x;
php > var_export(get_object_vars($x));
php: /path/to/php-src/Zend/zend_hash.c:1012: _zend_hash_index_add_or_update_i: Assertion `(zend_gc_refcount(&(ht)->gc) == 1) || ((ht)->u.flags & (1<<6))' failed.
[1]    127583 abort (core dumped)  php -a

---

Cause: ZEND_HASH_MAP_FOREACH_KEY_VAL(old_object->properties, num_key, key, prop) { expects the properties HashTable* to always be associative, but it sometimes be packed, e.g. for SplFixedArray.

Possible solutions:

1. Use the slower ZEND_HASH_FOREACH_KEY_VAL macro that works with both packed and associative HashTables. This seems to me like the best solution.
2. Split up the code into two separate cases for LIST/MAP internal representations where performance sensitive, in combination with 1.
3. Somehow force properties tables to always be associative when creating objects, including for PECL code (would waste memory)

@nikic @dstogov thoughts?

The following parts of the code are likely to be affected, I've only confirmed for zend_objects.c and zend_builtin_functions.c

» ag 'ZEND_HASH.*properties\b' ../php-src
../php-src/Zend/zend_objects.c
241:            ZEND_HASH_MAP_FOREACH_KEY_VAL(old_object->properties, num_key, key, prop) {

../php-src/Zend/zend_API.c
1291:   ZEND_HASH_MAP_FOREACH_STR_KEY_VAL(properties, key, value) {
1549:           ZEND_HASH_MAP_FOREACH_STR_KEY_VAL(properties, key, prop) {
1581:   ZEND_HASH_FOREACH_KEY_VAL(properties, h, key, prop) {

../php-src/Zend/zend_builtin_functions.c
764:            ZEND_HASH_MAP_FOREACH_KEY_VAL(properties, num_key, key, value) {

../php-src/ext/snmp/snmp.c
1824:   ZEND_HASH_MAP_FOREACH_STR_KEY_PTR(&php_snmp_properties, key, hnd) {

../php-src/ext/reflection/php_reflection.c
480:                    ZEND_HASH_MAP_FOREACH_STR_KEY(properties, prop_name) {
4590:           ZEND_HASH_MAP_FOREACH_STR_KEY_VAL(properties, key, prop) {

PHP Version

PHP 8.2.0-dev (2022-Feb-05)

Operating System

No response

[dstogov]

The real problem is SplFixedArray that reuses properties for something else. I'll try to take a deeper look before "fixing" the engine for one weird class.

In any case, for zend_objects_clone_members() I would prefer to split the loops for MAP and unexpected PACKED arrays.
Your second example actually asserts not in get_object_vars() but in spl_fixedarray_object_get_properties().
I prefer not to touch other places before we confirmed failures.

[TysonAndre]

The real problem is SplFixedArray that reuses properties for something else. I'll try to take a deeper look before "fixing" the engine for one weird class.

Thanks for the help!

@dstogov you may want to look at #6261 - that PR overrides get_properties_for instead, and I had suggestions on how to finish the implementation of that (that would overlap with possible fixes that could be implemented for get_properties, as well as be useful as a model for other data structures/PECLs to properly implement get_properties* in 8.2+)

• I think the approach in that PR would reduce memory usage, by not leaving around the property table
• The __set_state part is optional but seems uncontroversial

Some context on the problem I'm trying to solve

The way other data structures in the SPL is slightly unintuitive for end users the first time they run into it - e.g. SplDoublyLinkedList and it's subclasses don't implement var_export or (array) casts - var_export property fetching is implemented for SplDoublyLinkedList but not others.

• The first time I saw this I thought the list was empty, years ago
• Requiring that the property table be built (or for get_properties to return an empty array and hide properties, like in ext/ffi/ffi.c) also increases memory usage of data structures that do support inspection with var_export.
• This would also affect PECL extensions implementing custom behavior for get_properties, or future datastructures added to core php-src.
• php-ds also doesn't implement var_export, possibly due to initial limitations of get_properties predating get_properties_for being introduced in php 7.4, but it'd run into this issue because of that.
• I ran into issues like this when testing vectors in DEBUG mode in https://github.com/TysonAndre/pecl-teds/

php > $x = new SplFixedArray(1); $x[0] = 123; var_export($x);
SplFixedArray::__set_state(array(
   0 => 123,
))

php > $x = new SplDoublyLinkedList();
php > $x->push(123);
php > var_export($x);
SplDoublyLinkedList::__set_state(array(
))
php > var_dump($x);
object(SplDoublyLinkedList)#1 (2) {
  ["flags":"SplDoublyLinkedList":private]=>
  int(0)
  ["dllist":"SplDoublyLinkedList":private]=>
  array(1) {
    [0]=>
    int(123)
  }
}
php > var_dump((array)$x);
array(0) {
}
php > debug_zval_dump($x);
object(SplDoublyLinkedList)#1 (2) refcount(2){
  ["flags":"SplDoublyLinkedList":private]=>
  int(0)
  ["dllist":"SplDoublyLinkedList":private]=>
  array(1) refcount(1){
    [0]=>
    int(123)
  }
}
php > echo json_encode($x);
{}

[dstogov]

This should be fixed by c77bbcd

[TysonAndre]

Thanks, that approach seems good - it may need to be communicated to PECL authors, though. (e.g. PECL authors should conditionally call zend_hash_packed_to_hash in 8.2 or implement get_properties_for)

EDIT: get_properties_for won't work for var_export infinite recursion detection where cyclic data structures are possible, but it's probably fine if everything's a non-object/non-array, e.g. https://github.com/TysonAndre/pecl-teds/blob/main/teds_intvector.stub.php

EDIT: A note in UPGRADING.INTERNALS, probably