Fix GH-22000: ext/reflection: ensure __isset is not returning a reference before calling it

[zaaarf]

#22000 outlines a problem that happens in the new-ish 8.6 methods introduced by this RFC (hence why I'm submitting this against master).

They call __isset, but if the implementer made __isset return a reference, the call breaks and fails an assertion. I am pretty sure this qualifies as an unrecoverable state, so I just added a check before calling it that ensures that __isset does not return a reference, throwing an exception if that's the case so at least the user gets a graceful exit.

If this is recoverable, please do leave me a comment on how you envision it, and I'll make it happen.

Not sure if this bug warrants a dedicated test, but if it does, leave me a comment and I'll add it. added test

I should add that this is kind of a dirty fix that addresses the situation in a single case; we should probably look towards forbidding magic methods from returning references altogether, but maybe there's some valid niche use case that I can't envision.

[LamentXU123]

Hi :) First of all, this is technically a bug fix. So at first you may need to write a new entry in NEWS and UPGRADING file.

Frankly I don't that much understand your intention, but anyways, this makes isReadable() diverge from normal property semantics. You see, a regular isset($obj->prop) call does not fail just because __isset() is by-ref. So this PR do avoids the assertion/crash, however it also do so by introducing a new behavioral change that is not good IMO.

[zaaarf]

You see, a regular isset($obj->prop) call does not fail just because __isset() is by-ref

I know. But I've thought about it, and I'm not sure that we can recover from that state in this specific case. I could've done something wrong, but from what I've seen, in this case the assertion will fail if we do call __isset.

An alternative, slightly less invasive solution could be to output a warning and just make it return false, but that still breaks standard behavior.

So at first you may need to write a new entry in NEWS and UPGRADING file.

This is kind of why I haven't. Wanted to hear feedback considering there might be a better solution out there.

[LamentXU123]

An alternative, slightly less invasive solution could be to output a warning and just make it return false, but that still breaks standard behavior.

That sounds much better to me :) However, since this is not debated before we might need a new RFC on this.

[zaaarf]

we might need a new RFC on this

Thought that might be the case, but wasn't sure, since it's technically a bug "fix". I'll wait for a third opinion and then if I have to I'll figure out how you even make one.

[DanielEScherzer]

so there are a few issues with this

• the issue is that __isset is going to return a reference, not that it already has, so the error message is wrong
• if this is just a warning, then the behavior should not be changed, and __isset() should still presumably be called
• but also, why can't the reference just be dereferenced and examined?

also CC @iluuu1994 @Crell as the authors of the RFC

[LamentXU123]

if this is just a warning, then the behavior should not be changed, and __isset() should still presumably be called

I would most prefer this practice. I don't actually think this worth a backward complicity break but to keep the bug at least known for users a warning would be good.

[iluuu1994]

This is the wrong solution. This should suffice.

diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c
index 94d8bb7d149..3244a39450f 100644
--- a/ext/reflection/php_reflection.c
+++ b/ext/reflection/php_reflection.c
@@ -6731,6 +6731,7 @@ ZEND_METHOD(ReflectionProperty, isReadable)
 					zval member;
 					ZVAL_STR(&member, ref->unmangled_name);
 					zend_call_known_instance_method_with_1_params(ce->__isset, obj, return_value, &member);
+					zend_unwrap_reference(return_value);
 					*guard &= ~ZEND_GUARD_PROPERTY_ISSET;
 					OBJ_RELEASE(obj);
 					return;

[zaaarf]

In my defense, that's the very first thing I tried to do, conceptually. I guess I used the wrong function though because yours works, unlike the one I tried. Thank you.

[zaaarf]

Should I squash all the various commits into one?