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

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:

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
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
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

@dstogov

@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

@TysonAndre

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

Oh, you actually suggested the same :)

Fix GH-9697: array_walk($ffiInstance, function () {}) crashes due to …

Merge branch 'PHP-8.0' into PHP-8.1

Merge branch 'PHP-8.1' into PHP-8.2

Merge branch 'PHP-8.2'

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);

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.

You're right, it looks like only pos got modified, so I think the uses of Z_OBJPROP* I found were safe.

I found these issues while trying to understand the requirements for an implementation of get_properties/get_properties_for to work correctly. Does the following sound right?

get_properties

For handlers->get_properties, it seems like there's the following undocumented requirements:

This returns a non-null pointer, some callers such as PHP_FUNCTION(reset) expect the value to always be non-null - this is why ext/ffi returns &zend_empty_array and not null
This returns either
1. a reference counted property array which the object manages, generally obj->properties (because the callers obviously can't decrement reference counts)
2. The immutable empty array &zend_empty_array (in php 8.0+, not safe in php <= 7.4 if you care about array_walk) - if it's immutable and non-empty, then array_walk will crash. Bug fixes don't get backported farther than php 8.0 as per the release process. All of the code in php-src I looked at is safe for that
  - To get php-src to accept Z_OBJPROP* returning an immutable array, unserialization handlers must be overridden (e.g. implement __unserialize, or deny unserialization), and I forget if get_iterator handlers need to be overriden. Actually, I just found an issue with $x = FFI::new('int'); reset($x);

get_properties_for

For handlers->get_properties_for, the undocumented requirements seem to be

It's safe to return NULL and callers of get_properties_for handlers accepting arbitrary classes must check for null
For ZEND_PROP_PURPOSE_ARRAY_CAST and ZEND_PROP_PURPOSE_SERIALIZE, it's safe to return a brand new array
Prior to php 8.3 when Make var_export/debug_zval_dump check for infinite recursion on the *object* #8046 was merged, if infinite recursion in var_export/debug_zval_dump was a concern, ZEND_PROP_PURPOSE_VAR_EXPORT and ZEND_PROP_PURPOSE_DEBUG would need to be an array that had the same pointer every time
1. (usually easiest to make this obj->properties and use the get_properties handler implementation) - this is what I did in https://github.com/TysonAndre/pecl-teds/
2. An alternative would be to always return null or zend_empty_array and not support var_export/debug_zval_dump. This approach was used in https://github.com/php-ds/ext-ds when I checked years ago

PHP_FUNCTION(reset) may benefit from the same workaround of checking if the properties table of an array is empty

» php -a
Interactive shell

php > $x = FFI::new('int');
php > reset($x);

Deprecated: reset(): Calling reset() on an object is deprecated in php shell code on line 1
php: .../php-src/Zend/zend_hash.c:2609: zend_hash_internal_pointer_reset_ex: Assertion `(&ht->nInternalPointer != pos || zend_gc_refcount(&(ht)->gc) == 1) || ((ht)->u.flags & (1<<6))' failed.
[1]    639023 abort (core dumped)  php -a

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff6684859 in __GI_abort () at abort.c:79
#2  0x00007ffff6684729 in __assert_fail_base (fmt=0x7ffff681a588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=0x555556b3d178 "(&ht->nInternalPointer != pos || zend_gc_refcount(&(ht)->gc) == 1) || ((ht)->u.flags & (1<<6))", file=0x555556b3ce58 "/path/to/php-src/Zend/zend_hash.c", line=2609, 
    function=<optimized out>) at assert.c:92
#3  0x00007ffff6695fd6 in __GI___assert_fail (assertion=0x555556b3d178 "(&ht->nInternalPointer != pos || zend_gc_refcount(&(ht)->gc) == 1) || ((ht)->u.flags & (1<<6))", 
    file=0x555556b3ce58 "/path/to/php-src/Zend/zend_hash.c", line=2609, function=0x555556b3d8a0 <__PRETTY_FUNCTION__.15053> "zend_hash_internal_pointer_reset_ex") at assert.c:101
#4  0x0000555555edd81e in zend_hash_internal_pointer_reset_ex (ht=0x55555714f520 <zend_empty_array>, pos=0x55555714f544 <zend_empty_array+36>) at /path/to/php-src/Zend/zend_hash.c:2609
#5  0x0000555555c83e30 in zend_hash_internal_pointer_reset (ht=0x55555714f520 <zend_empty_array>) at /path/to/php-src/Zend/zend_hash.h:282
#6  0x0000555555c8b8fc in zif_reset (execute_data=0x7ffff38170b0, return_value=0x7fffffffafd0) at /path/to/php-src/ext/standard/array.c:1104
#7  0x0000555555f0091a in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER () at /path/to/php-src/Zend/zend_vm_execute.h:1250
#8  0x0000555555f77884 in execute_ex (ex=0x7ffff3817020) at /path/to/php-src/Zend/zend_vm_execute.h:55975
#9  0x0000555555f7d0d6 in zend_execute (op_array=0x7ffff388e000, return_value=0x0) at /path/to/php-src/Zend/zend_vm_execute.h:60343
#10 0x0000555555ebfde3 in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /path/to/php-src/Zend/zend.c:1780
#11 0x0000555555e19492 in php_execute_script (primary_file=0x7fffffffc7c0) at /path/to/php-src/main/main.c:2480
#12 0x0000555556039d27 in do_cli (argc=2, argv=0x555557189770) at /path/to/php-src/sapi/cli/php_cli.c:964
#13 0x000055555603a9e6 in main (argc=2, argv=0x555557189770) at /path/to/php-src/sapi/cli/php_cli.c:1333

Once the reset() crash is fixed, returning zend_empty_array for get_properties seems safe (and ext/ffi is implementing handlers properly) (as long as unserialize is denied or __unserialize is overridden), e.g. get_ht_for_iap will not leak memory because (1) rebuild_object_properties is prevented by the handler overrides (get_property_ptr_ptr, compare_objects, get_iterator), (2) even if rebuild_object_properties does somehow get called or those handler overrides (e.g. in a PECL), obj->properties will become a temporary array and stay a temporary array and be freed when the object is released

I'll think a bit more tomorrow.

Actually reset() does not crash but asserts

php-master/Zend/zend_hash.c:2609: zend_hash_internal_pointer_reset_ex: Assertion `(&ht->nInternalPointer != pos || zend_gc_refcount(&(ht)->gc) == 1) || ((ht)->u.flags & (1<<6))' failed.

It's possible to fix reset() or zend_hash_internal_pointer_reset(), but I expect there are few other places so may be it's better to return "mutable" array from FFI.

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.

I agree, I also think that's the case (e.g. to tolerate modifying the array by removing/adding elements).

The behavior of array_walk wasn't documented until a few years ago https://bugs.php.net/bug.php?id=80482 - I'm considering proposing deprecating using it with an object parameter in php 8.3, since

It doesn't behave like foreach, it iterates over Z_OBJPROP rather than using Traversable
The details of the handling of objects still isn't documented in https://www.php.net/array_walk (probably similar to array_walk($tmp = get_mangled_object_props($obj), ...))

Actually reset() does not crash but asserts

There is an assertion error in debug builds and a segmentation fault for writing to const data in non-debug builds

See my comment on the segfault in non-debug builds and valgrind stack trace

Avoid crash for reset/end/next/prev() on ffi classes

Navigation Menu

Repository navigation

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

Description

PHP Version

Operating System

get_properties

get_properties_for

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Description

PHP Version

Operating System

Activity

TysonAndre commented on Oct 9, 2022

dstogov commented on Oct 10, 2022

dstogov commented on Oct 10, 2022

TysonAndre commented on Oct 10, 2022

dstogov commented on Oct 10, 2022

TysonAndre commented on Oct 10, 2022

get_properties

get_properties_for

6 remaining items

dstogov commented on Oct 10, 2022

TysonAndre commented on Oct 10, 2022

TysonAndre commented on Oct 10, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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