`object_properties_load` crashes in debug mode when unserializing references to typed properties in php 8.1+

[TysonAndre]

Description

The following code:

<?php
class ObjectPropertiesLoadBug extends GMP {
    public int $a;
    public int $b;
}
$o = new ObjectPropertiesLoadBug();
$o->a = 123;
$o->b = &$o->a;
var_dump($o);
$ser = serialize($o);
echo "$ser\n";
unserialize($ser);
echo "Done\n";

Resulted in this output (in a PHP debug build using --enable-debug):

object(ObjectPropertiesLoadBug)#1 (3) {
  ["a"]=>
  &int(123)
  ["b"]=>
  &int(123)
  ["num"]=>
  string(1) "0"
}
O:23:"ObjectPropertiesLoadBug":2:{i:0;s:1:"0";i:1;a:2:{s:1:"a";i:123;s:1:"b";R:4;}}
php: .../php-src/Zend/zend_execute.c:3653: zend_ref_del_type_source: Assertion `source_list->ptr == prop' failed.
[1]    632930 abort (core dumped)  php object_properties_load.php

(It crashes when freeing the unserialized object)

(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=0x555556b40f6d "source_list->ptr == prop", file=0x555556b3fc40 "/path/to/php-src/Zend/zend_execute.c", line=3653, 
    function=<optimized out>) at assert.c:92
#3  0x00007ffff6695fd6 in __GI___assert_fail (assertion=0x555556b40f6d "source_list->ptr == prop", file=0x555556b3fc40 "/path/to/php-src/Zend/zend_execute.c", line=3653, 
    function=0x555556b42e10 <__PRETTY_FUNCTION__.19279> "zend_ref_del_type_source") at assert.c:101
#4  0x0000555555efa971 in zend_ref_del_type_source (source_list=0x7ffff3801dd8, prop=0x7ffff3806218) at /path/to/php-src/Zend/zend_execute.c:3653
#5  0x0000555555fad4f2 in zend_object_std_dtor (object=0x7ffff386f190) at /path/to/php-src/Zend/zend_objects.c:68
#6  0x0000555555996342 in gmp_free_object_storage (obj=0x7ffff386f190) at /path/to/php-src/ext/gmp/gmp.c:246
#7  0x0000555555fb4efd in zend_objects_store_del (object=0x7ffff386f190) at /path/to/php-src/Zend/zend_objects_API.c:200
#8  0x0000555555ebb231 in rc_dtor_func (p=0x7ffff386f190) at /path/to/php-src/Zend/zend_variables.c:57
#9  0x0000555555eef1e2 in i_zval_ptr_dtor (zval_ptr=0x7fffffffaf40) at /path/to/php-src/Zend/zend_variables.h:44
#10 0x0000555555f00a8c in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER () at /path/to/php-src/Zend/zend_vm_execute.h:1279
#11 0x0000555555f77884 in execute_ex (ex=0x7ffff3817020) at /path/to/php-src/Zend/zend_vm_execute.h:55975
#12 0x0000555555f7d0d6 in zend_execute (op_array=0x7ffff3887140, return_value=0x0) at /path/to/php-src/Zend/zend_vm_execute.h:60343
#13 0x0000555555ebfde3 in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /path/to/php-src/Zend/zend.c:1780
#14 0x0000555555e19492 in php_execute_script (primary_file=0x7fffffffc730) at /path/to/php-src/main/main.c:2480
#15 0x0000555556039d27 in do_cli (argc=2, argv=0x555557189770) at /path/to/php-src/sapi/cli/php_cli.c:964
#16 0x000055555603a9e6 in main (argc=2, argv=0x555557189770) at /path/to/php-src/sapi/cli/php_cli.c:1333

But I expected this output instead:

object(ObjectPropertiesLoadBug)#1 (3) {
  ["a"]=>
  &int(123)
  ["b"]=>
  &int(123)
  ["num"]=>
  string(1) "0"
}
O:23:"ObjectPropertiesLoadBug":2:{i:0;s:1:"0";i:1;a:2:{s:1:"a";i:123;s:1:"b";R:4;}}
Done

Various classes use object_properties_load, including GMP, SplFixedArray, etc.

Converting references to non-references would work, but would not be ideal. Ideally, this would behave the same way as $this->a = &$data['a'] would behave in userland, and add the type to the reference groups somehow

PHP Version

PHP 8.1 - 8.3-dev

Operating System

No response

[TysonAndre]

Actually, php 8.1 is when GMP first implemented __unserialize and __serialize (GMP::__unserialize calls object_properties_load) - this bug may also affect php 7.4, where typed properties were first added.

I noticed this when running the test case of SplFixedArray::__unserialize I added for #9706 in php 8.2 and newer

[TysonAndre]

@dstogov thoughts on this and #9708 ? E.g. any edge cases I missed

It looks like this was just missed (in the implementation of object_properties_load) when typed properties were implemented in php 7.4 - the above function object_properties_init_ex in Zend/zend_API.c does have checks for ZEND_TYPE_IS_SET(property_info->type) and zend_verify_property_type

EDIT: Similar code to base a fix for typed properties handling in object_properties_load on

The code for object_properties_init_ex seems correct for values, but doesn't handle references (it doesn't need to. it's only used in a few internal modules, to convert array return values to object return values of a given class, through object_and_properties_init - looks correct). ZEND_REF_ADD_TYPE_SOURCE seems to be the only additional thing to add for the Z_ISREF_P case based on the unserialize() implementation

// ext/standard/var_unserializer.re
		if (!php_var_unserialize_internal(data, p, max, var_hash)) {
			if (info && Z_ISREF_P(data)) {
				/* Add type source even if we failed to unserialize.
				 * The data is still stored in the property. */
				ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(data), info);
			}
			goto failure;
		}

		if (UNEXPECTED(info)) {
			if (!zend_verify_prop_assignable_by_ref(info, data, /* strict */ 1)) {
				zval_ptr_dtor(data);
				ZVAL_UNDEF(data);
				goto failure;
			}

			if (Z_ISREF_P(data)) {
				ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(data), info);
			} else {
				// ...
			}
		}

[dstogov]

@ TysonAndre sorry. I have problems understanding you. I reread the report few times and hardly understand what do you like me to check. Please, write more concrete and short questions.

Probably, you are right and object_properties_init_ex misses support for typed references.

[TysonAndre]

What do you think of this plan to fix object_properties_load to fix the crashes - am I missing any checks, and are these the right functions to use ? I'd only posted object_properties_init_ex and the unserializer as an example of similar functionality that is working correctly to refer to later when it's implemented (those work correctly).

My plan is to add checks to object_properties_load based on how the unserializer handles typed properties and references:

1. object_properties_load should start to call zend_verify_prop_assignable_by_ref and check if it failed
2. If the assigned value (e.g. from the __unserialize data in the gmp crash) is a reference, then object_properties_load should also call ZEND_REF_ADD_TYPE_SOURCE

The part of object_properties_load I'm asking about was

php-src/Zend/zend_API.c

Lines 1619 to 1629 in adb45a6

	if (property_info != ZEND_WRONG_PROPERTY_INFO &&
	property_info &&
	(property_info->flags & ZEND_ACC_STATIC) == 0) {
	zval *slot = OBJ_PROP(object, property_info->offset);
	zval_ptr_dtor(slot);
	ZVAL_COPY_VALUE(slot, prop);
	zval_add_ref(slot);
	if (object->properties) {
	ZVAL_INDIRECT(&tmp, slot);
	zend_hash_update(object->properties, key, &tmp);
	}

Yeah, sorry, that was disorganized.

[dstogov]

Your plan looks correct.