Segmentation fault in Zend/zend_execute_API.c

[YuanchengJiang]

Description

The following code:

<?php
$iter1 = new ArrayIterator(array(1,2,3));
$iter2 = new ArrayIterator(array(1,2));
$iter3 = new ArrayIterator(array(new stdClass(),"string",3));
$m = new MultipleIterator();
$m->setFlags(MultipleIterator::MIT_NEED_ALL | MultipleIterator::MIT_KEYS_ASSOC);
$m->attachIterator($iter1, "1");
$m->attachIterator($iter2, "2");
$m->attachIterator($m, 3);
foreach($m as $key => $value) {
 var_dump($key, $value);
}
?>

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

[ndossche]

This code fragment triggers infinite call recursion, and so the segfault comes from running out of stack space.
For VM re-entry, we do check the stack limit and exit the program gracefully in that case. However, here there is no VM re-entry, it all happens on the C side.
There's similar situations in where you can trick some extensions that wrap C libraries to do infinite recursion by constructing your code in a certain way, and that would also result in a segfault. I think this lies in the same category conceptually.
cc @arnaud-lb Do you think we should do anything here or is this a won't-fix?

[arnaud-lb]

I think we could add an explicit stack limit check in MultipleIterator, as it's cheap and doesn't really increase complexity (compared to e.g. adding guards). It would improve user experience slightly.

[ndossche]

@arnaud-lb While that is true, where does this stop? Just doing MultipleIterator seems ad-hoc to me. Do we really want to add a stack check to any internal C code that can recurse?

[arnaud-lb]

It's definitely ad-hoc, and I don't know how many of these checks would be necessary. If we only need a few here and there it would be ok in my opinion.

A more general solution would be to detect overflows with a guard page and signals. However we cannot nicely stop recursion and recover. We have to either abort or grow the stack (eventually leading to OOM).

[ndossche]

If we only need a few here and there it would be ok in my opinion.

Honestly, I'm not a fan of that idea, I think that will quickly become an inconsistent mess.