Segmentation fault in Zend/zend_types.h

[YuanchengJiang]

Description

The following code:

<?php
$dom = new DOMDocument;
$dom->loadXML('<foo>foo1</foo>');
$nodes = $dom->documentElement->childNodes;
$iter = $nodes->getIterator();
$script1_dataflow = $iter;
clone $script1_dataflow;

Resulted in this output:

Segmentation fault (core dumped)

Valgrind:

==2405895== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==2405895==  Bad permissions for mapped region at address 0x16D68A8
==2405895==    at 0x973F0F: zend_gc_delref (zend_types.h:1344)
==2405895==    by 0x97407A: zend_iterator_dtor (zend_iterators.c:91)
==2405895==    by 0x97350D: zend_internal_iterator_free (zend_interfaces.c:514)
==2405895==    by 0x9A51D5: zend_objects_store_del (zend_objects_API.c:200)
==2405895==    by 0x9C552D: rc_dtor_func (zend_variables.c:57)
==2405895==    by 0x8A281F: zval_ptr_dtor_nogc (zend_variables.h:36)
==2405895==    by 0x8D7E63: ZEND_FREE_SPEC_TMPVAR_HANDLER (zend_vm_execute.h:14893)
==2405895==    by 0x937424: execute_ex (zend_vm_execute.h:59399)
==2405895==    by 0x93AD56: zend_execute (zend_vm_execute.h:62962)
==2405895==    by 0x9D180A: zend_execute_script (zend.c:1906)
==2405895==    by 0x78AF30: php_execute_script_ex (main.c:2529)
==2405895==    by 0x78B0B6: php_execute_script (main.c:2569)

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

[devnexen]

Reproducible since 8.3

[nielsdos]

Bisect points me to 1e1ea4fbb7926b636614803c9fd020893a26ebc0 which I already suspected. It's probably not copying/setting the handler correctly.

EDIT: actually the bug exists on 8.2 too but is not triggerable because the standard free_obj is called on 8.2.

[nielsdos]

Not related to dom. Minimal reproducer:

<?php
$subject = new \ZendTest\Iterators\TraversableTest();
$it = $subject->getIterator();
clone $it;

[nielsdos]

The bug is this: The create_obj handler of InternalIterator is overwritten, but not the clone_obj handler. This is not allowed. In PHP 8.2 this didn't cause a segfault because the standard object handler was used for the clone instead of the internal handler. So then it allocates and frees the object using the standard object handlers. In 8.3 however, the object is created using the standard object handler and freed using the custom handler, resulting in the buffer overflow. Even though bisect points to https://github.com/php/php-src/commit/1e1ea4fbb7926b636614803c9fd020893a26ebc0 this only reveals the bug.

For example, actually using the clone always used to crash anyway: https://3v4l.org/QR1NS#v8.2.20 (also reproducible with zend-test)

The following patch would fix this issue: https://gist.github.com/nielsdos/4860d3cf9761cd4fa57f2f154b670b9e However, this patch ties the iterators together: i.e. advancing one iterator also advances the clone which is a "wtf moment". I'm not sure InternalIterator is actually supposed to be cloneable, if not then I'd just make a patch that sets clone_obj to NULL.

cc @iluuu1994 Can you please check this out?

[iluuu1994]

@nielsdos Sorry for the late response. My work capacity is currently reduced and I was a little busy with property hooks. I'll look at this later tonight.

[iluuu1994]

@nielsdos Thank you for your analysis! I agree that tying the iterators together is not the way to go. I don't think we can create a new iterator either, because we cannot move it to the same position without potential side effects. I think what we'd need is a new, optional clone function on zend_object_iterator_funcs, which would allow to opt-into cloning, correctly replicating the cloned iterators position. But I would not go through the trouble unless somebody actually asks for it. So, I'd go with your suggestion to simply disallow cloning. WDYT?

[nielsdos]

@iluuu1994 Thanks. I also thought about the optional clone handler a few days ago. However, given that this issue has existed for years and only now this comes up (artificially), it doesn't seem like there's a real need for this. So I'll go ahead and disallow cloning.