Assertion failure in ext/spl/spl_fixedarray.c

[YuanchengJiang]

Description

The following code:

<?php
$foo = new SplFixedArray(5);
$fusion = $foo;
$arrayObject = new ArrayObject($fusion);
$arrayObject[] = 'five';
var_dump($arrayObject);

Resulted in this output:

/php-src/ext/spl/spl_fixedarray.c:250: spl_fixedarray_object_get_properties_for: Assertion `!(((__ht)->u.flags & (1<<2)) != 0)' failed.
Aborted (core dumped)

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

[cmb69]

See #15775 (comment).

[nielsdos]

Bruh, and if we change ZEND_HASH_MAP_FOREACH_KEY_VAL_IND to ZEND_HASH_FOREACH_KEY_VAL_IND then we can do this:

<?php
$foo = new SplFixedArray(1);
$arrayObject = new ArrayObject($foo);
$arrayObject[] = 'x';
$arrayObject[] = 'x';
$arrayObject[] = 'x';
var_dump($foo->getSize()); // Outputs int(1)
var_dump($foo); // Outputs 3 elements

🤦

But hey at least the assertion is then gone 🤔

[nielsdos]

ArrayObject doesn't even know it's operating on an SplFixedArray, it's just going through the properties case and manipulating the properties table...

[iluuu1994]

Yeah, this class needs to be burned with fire 😄

[iluuu1994]

I would close this as a wontfix. We can't account for all the craziness that is possible with ArrayObject. In reality, we should disallow it from being used on internal classes, as doing so can break assumptions. Or just drop it entirely. Any objections?

[nielsdos]

@iluuu1994 Fixing the assertion would improve stability/fix crash, but won't fix the other issues of course. I think it would be good to at least change that foreach macro but I agree otherwise on burning the class with fire and ignoring semantic issues.

[cmb69]

In my opinion, ArrayObject should be deprecated as soon as possible, and for now we should document that using it on internal classes is undefined behavior (or maybe something less drastic).

[iluuu1994]

I don't have a big opinion. ArrayObject seems fundamentally broken, but I don't object to fixing this particular case, as the fix seems harmless.

[nielsdos]

I noticed that PHP 8.2 correctly throws an exception that you're not allowed to use SplFixedArray as input for an ArrayObject. In PHP 8.3 somehow this no longer throws. So I bisected this to c4ecd82. So turns out this is a regression and we should fix it such that using SplFixedArray is not allowed, instead of allowing it and fixing the assert.

Looking at the commit, it seems it replaced SplFixedArray's properties handler with a get_properties_for handler, and ArrayObject only checks whether get_properties is overloaded, it doesn't check get_properties_for.

I propose fixing it like this:

diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
index 79802454e3f..3fcce1a7682 100644
--- a/ext/spl/spl_array.c
+++ b/ext/spl/spl_array.c
@@ -1084,7 +1084,7 @@ static void spl_array_set_array(zval *object, spl_array_object *intern, zval *ar
 			}
 		} else {
 			zend_object_get_properties_t handler = Z_OBJ_HANDLER_P(array, get_properties);
-			if (handler != zend_std_get_properties) {
+			if (handler != zend_std_get_properties || Z_OBJ_HANDLER_P(array, get_properties_for)) {
 				zend_throw_exception_ex(spl_ce_InvalidArgumentException, 0,
 					"Overloaded object of type %s is not compatible with %s",
 					ZSTR_VAL(Z_OBJCE_P(array)->name), ZSTR_VAL(intern->std.ce->name));

This will make one test fail: ext/spl/tests/ArrayObject_overloaded_SplFixedArray.phpt. But imo this should've never worked in the first place, question is if this counts as BC break since this is broken anyway.

[Girgias]

I'm in favour of the proposed fix from @nielsdos

[cmb69]

Good find, @nielsdos! I think your suggested change makes sense, I don't mind the minor BC break, since it fixes problems, and only affects PHP 8.3.

For the record, relevant comment from the PR discussion: #9757 (comment)