Stale array iterator pointer

[iluuu1994]

FixesGH-19613

[bwoebi]

Looks good to me, thanks for fixing!

[dstogov]

The fix looks right, but I don't completely understand why zend_array_dup() duplicates the iterator (I forgot).
@iluuu1994, @bwoebi could you please revisit cd53ce8

[iluuu1994]

@dstogov Bob can correct me if I'm wrong.

zend_hash_iterator_pos_ex() detects when the iterated hashtable in the reference has been swapped, and reset the iterator position to 0. This makes sense when actually replacing the iterated array entirely. When the array is just modified, e.g. by appending an element, the iterator is not changed, and iteration continues from the same place.

Before cd53ce8, a CoW separation would also reset the iterator for the same reason: When the iterated array is stored in a temporary variable, and then the iterated array is appended to, the array will separate and thus the iter->ht != ht check in zend_hash_iterator_pos_ex() will also act as if the array was completely replaced. The only difference here is CoW, which is not supposed to be trivially observable.

The "obvious" solution would be to update the iter->ht on separation. However, we don't know which of the iterators need to be updated. E.g. the array might be referenced by multiple iterators, stored in separate references. For example:

$a = range(1, 3);
$b = $a;
foreach ($aas &$x) {
foreach ($bas &$y) {
echo"$x.$y\n";
if ($x === 3 && $y === 1) {
$a[] = 4; // <-- here
}
}
}

On modification of $a, we want to update only the outer iterator. Otherwise, we'll reset the position for the inner iterator, which keeps the handle to the old hashtable. For this approach to work we would need to store the zend_reference* in HashTableIterator and pass it to SEPARATE_ARRAY() to know which "end" of the array is modified. But this would require way too many changes in the codebase. Or we could put the iterator reset logic into zend_assign_to_variable_reference(), where we would loop through all iterators storing the updated reference, and reset its position. But I'm not fully confident this is the only way reference values can change.

Unfortunately, I can't think of a simpler solution either.

[iluuu1994]

I'll merge this for now, this fix is needed anyway.