magic property guards are not reset in timeout

[ivantsepp]

Description

The overloaded methods are protected via property guards flags from circular calls. An example is at

php-src/Zend/zend_object_handlers.c

Lines 724 to 726 in 0f398a4

	*guard |= IN_GET; /* prevent circular getting */
	zend_std_call_getter(zobj, name, rv);
	*guard &= ~IN_GET;

However, these guards are not reset when a timeout happens within the overloaded method. We will get undefined property warnings in the shutdown handlers as a result. This is because PHP still thinks we are inside the __get method

From https://www.php.net/manual/en/language.oop5.overloading.php#object.get:

PHP will not call an overloaded method from within the same overloaded method. That means, for example, writing return $this->foo inside of __get() will return null and raise an E_WARNING if there is no foo property defined, rather than calling __get() a second time. However, overload methods may invoke other overload methods implicitly (such as __set() triggering __get()).

However, I believe that we are no longer inside the __get call when we are in the shutdown handlers and we should allow further __get calls to work.

Here's a repro script (https://3v4l.org/DhXZH) :

<?php

ini_set('max_execution_time', 1);
set_error_handler(null, E_ALL);

class A {
    function __get($name) {
        return $name;
    }
}
global $a;
$a = new A;

function shutdown() {
    global $a;
    // Warning: Undefined property: A::$foo ...
    var_dump($a->foo);
}

register_shutdown_function('shutdown');

// Try to trigger "Fatal error: Maximum execution time of 1 second exceeded"
// within the A::__get() method.
for ($i = 0; $i < 100000000; $i++) {
    $a->foo;
}

And the possible outputs are:

1. The timeout is triggered outside of the __get call (A

Fatal error: Maximum execution time of 1 second exceeded in /home/itse/development/Etsyweb/ivantestcase.php on line 26
string(3) "foo"

or

2. The timeout is triggered within the __get call:

Fatal error: Maximum execution time of 1 second exceeded in /home/itse/development/Etsyweb/ivantestcase.php on line 9

Warning: Undefined property: A::$foo in /home/itse/development/Etsyweb/ivantestcase.php on line 18
NULL

I expect output 1 no matter when the timeout occurs. Output 2 shouldn't occur.

I attempted to dig into the source code for a fix. Perhaps in the zend_interrupt_helper that gets called from ZEND_VM_INTERRUPT, we can look at the call frames and reset the property guards? I'm not too familiar with PHP Virtual Machine so I am not sure if this is the correct approach.
https://github.com/php/php-src/compare/f58a3c392f4fc0ce2f82935399c5e6e64441e2e9...ivantsepp:php-src:61e0b6293829a508884de388f99ab093be9e4563?expand=1

Let me know if this is a bug that should be addressed and I would appreciate any pointers on what a potential fix might look like! I am interested in working on a fix if so.

PHP Version

PHP 8.4.0-dev

Operating System

macOS 13.6

[iluuu1994]

I attempted to dig into the source code for a fix. Perhaps in the zend_interrupt_helper that gets called from ZEND_VM_INTERRUPT, we can look at the call frames and reset the property guards?

It looks like the patch will only work if the __get call is on the top of the stack frame. You'd need to walk the entire stack frame to fix this for all objects. You'd also probably want to move that to zend_timeout() or similar itself.

We have gotten a similar bug report (possibly) regarding fibers, which pose the same issue when the fiber is suspended within __get, or any method called from it. #12694 If we do decide to solve this issue, we might want to think about approaches that will work for both.

[iluuu1994]

Also, thank you for the detailed report! Love to see that.

[ivantsepp]

Thanks for looking and yea I agree, my proposed fix would require walking the entire stack frame in order to reset property guards.

I hadn't thought about fibers and it would make sense to tackle these issues in one go. (If I'm following correctly, https://3v4l.org/ju5mp represents the fiber scenario right?) #12694 also mentions that other "longjumps" can trigger this behavior such as the memory exhaustion scenario (described in https://3v4l.org/Q7nSo#v8.2.0).

Perhaps we can scope the property guards to the "execution context" we're in? So we would have different sets of property guards for each possible "execution context" which could be a zend_fiber_context or a "i am in shutdown" context.

[iluuu1994]

That's a good idea! We may track this with a simple integer that we bump for fibers, and then for shutdown. We might be able to store it in Z_EXTRA of the guard property, but I can't check since I'm on mobile.

[iluuu1994]

From a quick look, we're already using the properties extra field for the guard itself. I forgot that the property value is used for the property name. Furthermore, when there are multiple guards that are stored in an array, we need to be able to store a guard for the same property in different environments. One approach might be to bias the string hash with the execution context counter, similar to what we're doing for literal compaction. The downside is that the string would need to be cloned, so that we're not breaking string comparison elsewhere. Fortunately, we can likely avoid this for the "main" execution context, i.e. not within a fiber and not in shutdown, by making it use a bias of 0.

[iluuu1994]

Here's a simple Poc: #14994 A test for fibers is needed still, but I ran out of time for right now.

[ivantsepp]

That's amazing! Thanks for the quick turnaround and let me know if I can help out with anything!

[iluuu1994]

It was a nice idea. 🙂 Let's see what the feedback is! Note that this most likely will only land for master, i.e. PHP 8.4.