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

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

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

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
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);

Make var_export first check for infinite recursion on the object

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

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

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

Fixed GH-8044 (var_export/debug_zval_dump HT_ASSERT_RC1 debug failure…

Merge branch 'PHP-8.0' into PHP-8.1

Merge branch 'PHP-8.1'

Navigation Menu

Repository navigation

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

Description

PHP Version

Operating System

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Description

PHP Version

Operating System

Activity

TysonAndre commented on Feb 5, 2022

TysonAndre commented on Feb 5, 2022

cmb69 commented on Feb 7, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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