Accessing the object after WeakRef unreferencing results in use-after-free

[bwoebi]

Description

The following code:

<?php

$w = new WeakMap;

$o = new stdClass;
$w[$o] = new class($r) {
	function __construct(public &$r) {}
	function __destruct() {
		$o = $this->r->get();
		register_shutdown_function(function() use ($o) {
			var_dump($o);
		});
	}
};
$r = WeakReference::create($o);

Resulted in this output:

==36789== Invalid read of size 4
==36789==    at 0x54C4404: zend_gc_delref (zend_types.h:1342)
==36789==    by 0x54BFAC7: i_zval_ptr_dtor (zend_variables.h:43)
==36789==    by 0x54BF86B: zend_array_destroy (/usr/local/src/php/Zend/zend_hash.c:1840)
==36789==    by 0x54862CF: zend_destroy_static_vars (/usr/local/src/php/Zend/zend_opcode.c:532)
==36789==    by 0x55C4DE7: zend_closure_free_storage (/usr/local/src/php/Zend/zend_closures.c:522)
==36789==    by 0xBA0918F: dd_uhook_closure_free_wrapper (uhook.c:1158)
==36789==    by 0x55EA76F: zend_objects_store_del (/usr/local/src/php/Zend/zend_objects_API.c:200)
==36789==    by 0x5499757: rc_dtor_func (/usr/local/src/php/Zend/zend_variables.c:57)
==36789==    by 0x54997F3: i_zval_ptr_dtor (zend_variables.h:44)
==36789==    by 0x5499793: zval_ptr_dtor (/usr/local/src/php/Zend/zend_variables.c:84)
==36789==    by 0x52333E3: fci_release (/usr/local/src/php/ext/standard/basic_functions.c:1611)
==36789==    by 0x522B973: user_shutdown_function_dtor (/usr/local/src/php/ext/standard/basic_functions.c:1622)
==36789==  Address 0xe813150 is 0 bytes inside a block of size 40 free'd
==36789==    at 0x604B614: free (m_replacemalloc/vg_replace_malloc.c:989)
==36789==    by 0x544D673: _efree_custom (/usr/local/src/php/Zend/zend_alloc.c:2500)
==36789==    by 0x544D5C7: _efree (/usr/local/src/php/Zend/zend_alloc.c:2620)
==36789==    by 0x55EA7DF: zend_objects_store_del (/usr/local/src/php/Zend/zend_objects_API.c:204)
==36789==    by 0x5499757: rc_dtor_func (/usr/local/src/php/Zend/zend_variables.c:57)
==36789==    by 0x54997F3: i_zval_ptr_dtor (zend_variables.h:44)
==36789==    by 0x5499793: zval_ptr_dtor (/usr/local/src/php/Zend/zend_variables.c:84)
==36789==    by 0x54BE46F: _zend_hash_del_el_ex (/usr/local/src/php/Zend/zend_hash.c:1488)
==36789==    by 0x54BDF6B: _zend_hash_del_el (/usr/local/src/php/Zend/zend_hash.c:1515)
==36789==    by 0x54C15C3: zend_hash_reverse_apply (/usr/local/src/php/Zend/zend_hash.c:2231)
==36789==    by 0x547A843: shutdown_destructors (/usr/local/src/php/Zend/zend_execute_API.c:260)
==36789==    by 0x549C227: zend_call_destructors (/usr/local/src/php/Zend/zend.c:1281)
==36789==  Block was alloc'd at
==36789==    at 0x60485D4: malloc (m_replacemalloc/vg_replace_malloc.c:446)
==36789==    by 0x544DA1B: __zend_malloc (/usr/local/src/php/Zend/zend_alloc.c:3128)
==36789==    by 0x544D54F: _malloc_custom (/usr/local/src/php/Zend/zend_alloc.c:2491)
==36789==    by 0x544D493: _emalloc (/usr/local/src/php/Zend/zend_alloc.c:2610)
==36789==    by 0x55E0907: zend_objects_new (/usr/local/src/php/Zend/zend_objects.c:189)
==36789==    by 0x54A6143: _object_and_properties_init (/usr/local/src/php/Zend/zend_API.c:1773)
==36789==    by 0x54A621B: object_init_ex (/usr/local/src/php/Zend/zend_API.c:1796)
==36789==    by 0x552F62F: ZEND_NEW_SPEC_CONST_UNUSED_HANDLER (zend_vm_execute.h:10557)
==36789==    by 0x54E2073: execute_ex (zend_vm_execute.h:57037)
==36789==    by 0x54E2483: zend_execute (zend_vm_execute.h:61634)
==36789==    by 0x549E34B: zend_execute_scripts (/usr/local/src/php/Zend/zend.c:1895)
==36789==    by 0x53CB8E3: php_execute_script (/usr/local/src/php/main/main.c:2529)

Not exactly sure what to expect - either we just decide to make WeakReference->get() return NULL when the targeted object has IS_OBJ_FREE_CALLED. Which would fix #20404 trivially as well.
But it would be a BC break and render the object inaccessible in "externally associated destructors" (I mean dummy classes with destructors in weakmaps to observe an object to be freed). Because simply accessing as long as it lives is currently fine (i.e. within the destructor). Just persisting it outside of the destructor is problematic.

Or we strip the IS_OBJ_FREE_CALLED flag in the case the refcount is increased again in zend_object_std_dtor and skip freeing the object in case the flag is no longer present after the free_obj() call.

The latter solution is more flexible and does not have a BC break.

PHP Version

PHP 8.2+

Operating System

No response

[arnaud-lb]

#20001 would also fix the issue, with the same BC break.

But it would be a BC break and render the object inaccessible in "externally associated destructors" (I mean dummy classes with destructors in weakmaps to observe an object to be freed). Because simply accessing as long as it lives is currently fine (i.e. within the destructor). Just persisting it outside of the destructor is problematic.

Is this a real use-case? This depends on undocumented behaviors, and this seems fragile as it works only if the WeakMap entry is cleared before the WeakReference. It will not work if the WeakReference was registered before the WeakMap entry, or if a WeakReference to that object existed before:

$w = new WeakMap;

$o = new stdClass;
$r0 = WeakReference::create($o);
$w[$o] = new class($r) {
	function __construct(public &$r) {}
	function __destruct() {
		var_dump($this->r->get()); // NULL
	}
};
$r = WeakReference::create($o);

[bwoebi]

Yes, I know that it's ordering sensitive. Whether it's undocumented or not I didn't know, but it worked :-D

But yes, it's actually coming from real code (I just did not do the dangerous thing of persisting it outside of the destructor - I discovered this while reproducing the other issue).

I'm currently contemplating whether we possibly should actually break this, and instead provide first-class support for observing destruction. I.e. WeakReference::observe($o, function($object) { ... }); which then has well-defined semantics and does not rely on undocumented and fragile behaviour.

Does that sound reasonable maybe?

[arnaud-lb]

This sounds reasonable, but IIRC currently we make some assumptions about classes without a destructor, that will not be valid anymore if we introduce this.

Other than that, this has the same drawbacks as destructors / should not add more engine edge cases.

If we don't pass the object itself to the callback it would be similar to https://docs.oracle.com/javase/9/docs/api/java/lang/ref/Cleaner.html. The cleaner/callback may reference any state it needs, but not the object itself, so it can not resurrect it. Ordering is improved as anything referenced by the callback is guaranteed to not have been destroyed yet (except during shutdown).

[bwoebi]

assumptions about classes without a destructor

These assumptions are only for dtor, but not free (which is where weakrefs get freed), IIRC?

[arnaud-lb]

I was thinking about the Optimizer or JIT. I remembered seeing destructor-related optimizations, but I think I was wrong.