`array_walk($ffiInstance, function () {})` and `reset($ffiInstance)` crashes due to expecting mutable array

[TysonAndre]

Description

The following code:

<?php
$x = FFI::new('int');
array_walk($x, function($x) { echo "test\n"; });

Resulted in this output:

segmentation fault (core dumped)

valgrind output:

==73944== Process terminating with default action of signal 11 (SIGSEGV)
==73944==  Bad permissions for mapped region at address 0x1D0352A
==73944==    at 0xA8A598: zend_hash_iterator_add (zend_hash.c:503)
==73944==    by 0x840B07: php_array_walk (array.c:1286)
==73944==    by 0x84183B: zif_array_walk (array.c:1407)
==73944==    by 0xAB4919: ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1250)
==73944==    by 0xB2B883: execute_ex (zend_vm_execute.h:55975)
==73944==    by 0xB310D5: zend_execute (zend_vm_execute.h:60343)
==73944==    by 0xA73DE2: zend_execute_scripts (zend.c:1780)
==73944==    by 0x9CD491: php_execute_script (main.c:2480)
==73944==    by 0xBEDD26: do_cli (php_cli.c:964)
==73944==    by 0xBEE9E5: main (php_cli.c:1333)

But I expected this output instead:

// blank output

To fix this:
array_walk expects get_properties to return a non-empty refcounted array. Options include:

1. Start doing that, e.g. with rebuild_object_properties. Also start to override the get_properties_for handler alongside the get_properties handler to return null
2. Change behavior of that and other callers of Z_OBJPROP and other uses of get_properties to allow internal classes to return null in handler overrides
3. Special case zend_hash_num_elements(target_hash) == 0 and return early while continuing to free userdata - this should avoid this crash?

// ext/standard/array.c
// static int php_array_walk(
// 	php_array_walk_context *context, zval *array, zval *userdata, int recursive)
	zend_hash_internal_pointer_reset_ex(target_hash, &pos);
	ht_iter = zend_hash_iterator_add(target_hash, pos); // this tries to modify the immutable array

// ext/ffi/ffi.c
static HashTable *zend_fake_get_properties(zend_object *obj) /* {{{ */
{
	return (HashTable*)&zend_empty_array;  // This is a const immutable array
}
/* }}} */

PHP Version

7.4-8.3-dev

Operating System

Any

[TysonAndre]

@dstogov - what are your thoughts on the best way to fix this?

In older php versions, I think removing the get_properties handler (just using the default zend_std_get_properties to build the empty property tables) may work with the least impact

• If get_properties_for is added, return NULL there

I noticed this while looking into how Z_OBJ_PROP and the get_properties macros are used in practice for implementing a pecl trying to avoid populating properties tables .
Some code in php-src checks if Z_OBJ_PROP returns null and others don't

[dstogov]

@TysonAndre I'll take care about this. The easy way is an additional check for empty array before the iterating attempt.

[dstogov]

Oh, you actually suggested the same :)

[TysonAndre]

A similar fix may help for the com_dotnet bindings, though I don't have Windows to test and don't use this binding. It might also be safe to use the new ZEND_*FOREACH* macros if this is just using the older style of code and hasn't been touched, but I'm not completely sure.

(checking for 0 properties or switching to foreach macros that don't mutate the property array, as well as putting the HashTable into a local variable rather than repeatedly calling handler->get_properties())

zend_hash_internal_pointer_reset_ex would attempt to modify memory of the const zend_empty_array?

// ext/com_dotnet/com_wrapper.c
	/* properties */
	if (Z_OBJPROP(disp->object)) {
		zend_hash_internal_pointer_reset_ex(Z_OBJPROP(disp->object), &pos);
		while (HASH_KEY_NON_EXISTENT != (keytype =
				zend_hash_get_current_key_ex(Z_OBJPROP(disp->object), &name,
				&pid, &pos))) {
			char namebuf[32];
			if (keytype == HASH_KEY_IS_LONG) {
				snprintf(namebuf, sizeof(namebuf), ZEND_ULONG_FMT, pid);
				name = zend_string_init(namebuf, strlen(namebuf), 0);
			} else {
				zend_string_addref(name);
			}

			zend_hash_move_forward_ex(Z_OBJPROP(disp->object), &pos);

[dstogov]

I also don't have Windows now, but I think this dom loop is OK. zend_hash_internal_pointer_reset_ex() is going to modify pos but not the HashTable.

I think, array_walk() uses old iteration API on purpose, because of possible side effects in callback function. I hope, we converted most "safe" places where we used the old iteration API to ZEND_HASH_FOREACH... during 7.0 development. array_walk() is not safe, but this dom loop looks safe.