Fix use-after-free with nested FFI::addr() calls

[iluuu1994]

Fixes GH-9598

@dstogov I did not see another way to solve this. Basically, cdata1.ptr -> cdata1.ptr_holder -> cdata2.ptr_holder -> Example. Right after \FFI::addr(\FFI::addr($struct)) cdata2 is released. Since this object is released by the VM when the TMPVAR is destroyed we need to increase the refcount of the CData object itself.

Please let me know if you can see a better solution. This is the first time I've looked at the FFI internals, very much possible I'm missing something simple.

Thanks!

[dstogov]

@iluuu1994 thanks for looking into this.

See https://www.php.net/manual/en/ffi.addr.php

1. FFI::addr() creates unmanaged pointer. PHP/FFI won't try to keep the pointed data alive.
2. I suspect FFI::addr(FFI::addr($var)) was used to get ptr_ptr to the var, but its equivalent in C is &(&var). This C code doesn't make sense at all, why should it work in PHP :)

I don't think this should be fixed, may be it's better to throw a error.
I may be wrong. You are welcome for discussion. I hope, I'll be able to look into this deeper on Monday.

[iluuu1994]

@dstogov Ah! Thanks for explaining. Should've read the documentation better 🙂

I suspect FFI::addr(FFI::addr($var)) was used to get ptr_ptr to the var, but its equivalent in C is &(&var). This C code doesn't make sense at all, why should it work in PHP :)

Yeah, I found this in a Symfony test which I'll fix instead then.

I don't think this should be fixed, may be it's better to throw a error.

This would be the case when GC_REFCOUNT(&cdata->std) == 1 && Z_REFCOUNT_P(arg) == 1, right? What confused me is that this is explicitly handled in FFI::addr():

php-src/ext/ffi/ffi.c

Lines 4231 to 4242 in b61c81c

	if (GC_REFCOUNT(&cdata->std) == 1 && Z_REFCOUNT_P(arg) == 1) {
	if (ZEND_FFI_TYPE_IS_OWNED(cdata->type)) {
	/* transfer type ownership */
	cdata->type = type;
	new_type->pointer.type = ZEND_FFI_TYPE_MAKE_OWNED(type);
	}
	if (cdata->flags & ZEND_FFI_FLAG_OWNED) {
	/* transfer ownership */
	cdata->flags &= ~ZEND_FFI_FLAG_OWNED;
	new_cdata->flags |= ZEND_FFI_FLAG_OWNED;
	}
	}

That branch then wouldn't make much sense as it is.

Edit: I later found out transferring ownership of other things like structs works just fine as for those the CData can safely be deallocated with the data still being fully valid. I added a test for this in #9601.