PHPC-2456: Correctly dereference arrays in type maps

[alcaeus]

PHPC-2456

This PR adds dereferencing logic to the PHP array API polyfill. This logic has already been added for longs, strings, and doubles, but not for arrays and objects. This can cause issues when passing a reference to an array in a type map, as we extract an array but receive NULL. To the user, this makes no sense as from their point of view, a reference to an array is an array like any other.

Note that I'm targeting v1.19 here as that's our current stable branch, but we may opt to not release another 1.19 patch release and only fix this in 1.20.0.

[jmikola]

This looks related to a previous issue we had with dereferencing strings. See c68cdcd from PHPC-1839.

Also, you should contribute all of these ZVAL_DEREF() changes upstream to sgolemon/php-array-api when you get a chance. It's your ticket to the very exclusive contributors club.

Maybe @sgolemon can make us t-shirts.

[alcaeus]

Will contribute this upstream 👍

[jmikola]

I realize this is the same logic from c68cdcd, but I'm curious this could be replaced with a fall-through.

Dereferencing and Unwrapping includes the following code example:

zval *val;
ZEND_HASH_FOREACH_VAL(ht, val) {
    ZVAL_DEREF(val);

    /* Do something with val, now a guaranteed non-reference. */
} ZEND_HASH_FOREACH_END();

That implies that dereferencing would never yield another IS_REFERENCE type. That is also supported by this earlier text in Initializing references:

zval *zv = /* ... */;
ZVAL_MAKE_REF(zv);

If zv was already a reference, this does nothing. It if wasn’t a reference yet, this will change zv into a reference and set its initial value to the old value of zv.

It's a bit too early for me to dive through the macros in zend_types.h to confirm this.

[jmikola]

Looking through php-src, I don't see any examples of ZVAL_DEREF() being called iteratively. It tends to precede zval access, and code relies on it being a no-op for non-references. In that case, perhaps you could unconditionally call ZVAL_DEREF(zarr) after the initial NULL check and eliminate the goto.

Likewise for the original patch in c68cdcd.

If you'd rather not make this change now, I'd consider it for the upstream contribution to sgolemon/php-array-api.

[alcaeus]

Looking at this, I don't think that will work. After calling ZVAL_DEREF we still have to check the type of the dereferenced zval before accessing the ce, which wouldn't work with a fallthrough. As much as I hate the goto way of doing things, the only other option I see is a while (Z_TYPE_P(z) == IS_REFERENCE), which is equally bad.

[alcaeus]

Removed the goto after all by calling ZVAL_DEREF unconditionally after a NULL check.

[jmikola]

I have a suggestion to eliminate the goto, but I'm sure this works as-is given the regression test (and previous, related issue).