var_export emits debug assertion errors for HT_ASSERT_RC1(ht); in builds for SplFixedArray circular references

[TysonAndre]

Description

The following code in a php build configured with --enable-debug (reproduced in 7.4, 8.0, 8.1, 8.2.0-dev):

<?php
$x = new SplFixedArray(1);
$x[0] = $x;
var_export($x);

Resulted in this output in _zend_hash_index_add_or_update_i for HT_ASSERT_RC1. This is due to ext/spl/spl_array.c needing to update the implementation of its array in place for static HashTable* spl_fixedarray_object_get_properties(zend_object *obj) so that the output of var_export, var_dump, json_encode, etc. all reflect the latest array contents of the backing fixed array:

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]    312613 abort (core dumped)  php -a

(Most of the time it will be setting the offset of the array to itself, and not actually matter, but set_error_handler can have user code modify the original properties. This is a constructed example and not really likely to matter)

But I expected this output instead:

Not crashing, successfully setting $x[0]

(I haven't tested php 7.3, though that branch is closed regardless. 7.2 did not have this debug failure)

---

Also, I'm not familiar with the history of this, but it seems like there are (at least) 3 different mechanisms in use for infinite recursion detection for json_encode/var_export/var_dump. Would it make sense to firstcheck Z_PROTECT_RECURSION in var_export in addition to/instead of protecting the array, before calling get_properties_for?

• json_encode needs distinct results of *get_properties (Z_OBJPROP) to detect infinite recursion.

• var_export needs *get_properties_for to return the same array every time to detect infinite recursion with GC_TRY_PROTECT_RECURSION. (E.g. this 1. makes it impractical to save memory (and save time in garbage collection) by returning a distinct array rather than populating get_properties., 2. Seems like it would delay garbage collection of objects unintuitively if you call var_export then replace the entry of SplFixedArray (they wouldn't be garbage collected until the next var_export/json_encode/var_dump call, or (array) cast?))

  Adding similar calls to Z_PROTECT_RECURSION(zval *struct) (and UNPROTECT), either instead of or in addition to existing may help avoid this failure mode.

  By design, var_export does not use https://www.php.net/manual/en/language.oop5.magic.php#object.debuginfo - I wonder if that's somehow related to the different design used by var_dump, but the code isn't documented

• var_dump is doing yet another different thing, and calling Z_PROTECT_RECURSION(zval *struct) to set the GC_FLAGS
  on the object?

• serialize thankfully calls php_var_serialize_intern and checks with var_already = php_add_var_hash with a var_hash specific to that serializer, which is correct

---

I'd be worried about replacing the existing checks in <=8.1/8.0, in case extensions or end users of those extensions were somehow relying on specifics of whatever calls to get_properties/get_properties_for were made.

#8041 is a different issue I also found when looking into best practices for implementing *get_properties and *get_properties_for

PHP Version

PHP 7.4-8.2

Operating System

No response

[TysonAndre]

The below snippet is not even specific to SplFixedArray, but illustrates why the HT_ASSERT_RC1 would matter during iteration for anyone wondering. Increasing the size from 8 to 9 reallocates the memory backing the array, and the old memory being iterated over by var_export becomes invalid.

Possibly related to https://bugs.php.net/bug.php?id=79214

1. Maybe this should avoid emitting E_WARNING during var_export and postpone emitting them until after var_export is finished? This would get the same result as the below snippet by increasing the number of properties to 9 during var_export
2. Maybe use SEPARATE_ARRAY after the recursion check if the array was non-empty to make side effects less likely?

$ USE_ZEND_ALLOC=0 php spl_fixedarray_var_export.php
var_export does not handle circular references
In handler

Warning: var_export does not handle circular references in /usr/local/tyson/php-src/spl_fixedarray_var_export.php on line 9
array (
  0 => NULL,
  1 => 
  ArrayObject::__set_state(array(
  )),
  2 => NULL,
  3 => NULL,
  4 => NULL,
  5 => NULL,
  6 => NULL,
  7 => NULL,
  8 => NULL,
)
After clear

Warning: var_export does not handle circular references in /usr/local/tyson/php-src/spl_fixedarray_var_export.php on line 12
array (
  0 => NULL,
)malloc_consolidate(): invalid chunk size
[1]    335364 abort (core dumped)  USE_ZEND_ALLOC=0 php spl_fixedarray_var_export.php

<?php
set_error_handler(function ($errno, $errmsg) {
    echo "$errmsg\n";
    if (isset($GLOBALS['array'])) {
        $fixedArray = $GLOBALS['array'][0];
        $fixedArray->setSize(9);
        $fixedArray[1] = new ArrayObject();
        echo "In handler\n";
        var_export((array)$fixedArray);
        echo "\nAfter clear\n";
        $fixedArray->setSize(1);
        var_export((array)$fixedArray);
    }
});

$array = [];
$fixedArray = new SplFixedArray(2);
$fixedArray[0] = $fixedArray;
$i = 0;
$fixedArray[1] = (object)["a$i" => 123];
$array = [$fixedArray];
var_export($array);

[TysonAndre]

It seems like the most commonly used extension (out of extensions with functionality similar to SplFixedArray) works around this/avoids this by implementing neither get_properties or get_properties_for for any classes, returning empty arrays.

• Possibly not done to avoid this infinite recursion, possibly issues on finding documentation on the proper way to have get_properties/get_properties_for implemented (no code comments in php-src for this), possibly to save memory, or some combination of those issues (I didn't see anything in a quick search of var_export skimming the issue tracker)

php > $v = new Ds\Vector();
php > $v[] = 123;
php > var_export($v);
Ds\Vector::__set_state(array(
))
php > var_dump($v);                                                                                                                                                                                                                                           
object(Ds\Vector)#2 (1) {
  [0]=>
  int(123)
}
php > var_export((array)$v);
array (
)

I was working on similar functionality. Except for the way infinite recursion is currently detected in var_export (fixed by the linked PR), it looks like it would be possible for PECLs/php-src modules to get var_export/array casts to work and keep the low memory usage (and also avoid delaying garbage collections by having duplicates in stale copies of the underlying collection in zend_object->properties)) by

1. Returning brand new arrays in get_properties_for for the caller to free
2. Returning the immutable empty array (zend_empty_array) in get_properties, similar to ext/ffi/ffi.c

[cmb69]

Note that PHP-7.4 is also closed for non-security related fixes.