Fixed bug #81430 Check if runtime cache pointer is NULL before dereferencing

[rioderelfte]

Check if the runtime cache pointer is NULL before dereferencing it.

[beberlei]

We thought alternatively to make sure the dummy execute_data created in call_attribute_constructor (ext/reflection/php_refleciton.c) should do what zend_get_call_trampoline_func does, but we didn't manage to replicate this:

ZEND_MAP_PTR_INIT(func->run_time_cache, (void***)&dummy);

[bwoebi]

This looks like a partial fix to me. But we can merge this specific fix already I think.

As far as I had diagnosed back then, the dummy frame is stack-allocated - data on a stack which may have been overwritten by the time zend_observer_fcall_end is executed. (i.e. the attributes ctor frame is not within the active stack at that point in time, if it bailed out somewhere within attribute construction.)

[rioderelfte]

Thanks for your feedback!

I think those are two different problems. This PR fixes the problem the issue that the runtime cache of the dummy frame is null. I think this issue was introduced by 12b0f1b.

The other issue is that accessing stack allocated frames inside zend_observer_fcall_end_all is not allowed. I added a second test case for this which fails for now. I think accessing frames during shutdown is currently not allowed and for this reason the current_execute_data is cleared at the very beginning of php_request_shutdown:

php-src/main/main.c

Lines 1783 to 1786 in 4c171ed

	/* EG(current_execute_data) points into nirvana and therefore cannot be safely accessed
	* inside zend_executor callback functions.
	*/
	EG(current_execute_data) = NULL;

From my understanding the reflection dummy frame is not the only place where frames are stack allocated. I don't think accessing the frames in the zend_observer_fcall_end_all is safe right now. I see four possible fixes for this issue:

1. Heap allocate the reflection dummy frame. This only fixes this specific issue but there might be stack allocated frames at other places.
2. Always heap allocate the frames. This probably is a bigger refactoring and might have some performance implications.
3. Don't directly use the frames inside zend_observer_fcall_end_all but always copy them in zend_observer_fcall_begin. I think we need to make a deep copy since for example the zend_function is also stack allocated in this case.
4. Register a bailout handler in zend_observer_fcall_begin and deregister it in zend_observer_fcall_end to get rid of the zend_observer_fcall_end_all function.

I am happy to try to implement one of these fixes. My preferred fix would be number four. What do you think about this?

But since I think these are two separat issues and this one is a regression while the other is not, I would like to ask you to merge this PR while I try to fix the other issue as well.

[rioderelfte]

For clarification: This fixes a real live crash in our extension which was introduced by PHP 8.0.12. That's why I would like to get this merged before the 8.0.14 release.

[cmb69]

That's why I would like to get this merged before the 8.0.14 release.

What we missed. Sorry.

Anyhow, the fix looks good to me; if there are no objections, I'll merge it on Monday.

[beberlei]

@cmb69 thabk you!

[cmb69]

Oh, just noticed that the test is apparently failing, but the logs are no longer available, and for some reason triggering a re-run does not work. So I'm closing and re-opening to trigger the test runs again.

[cmb69]

The other issue is that accessing stack allocated frames inside zend_observer_fcall_end_all is not allowed. I added a second test case for this which fails for now.

Ah, I missed that. However, the test appears to have failed due to the hard-coded memory limit in the error message; replacing that with %d made the test pass. Or did something else change in the meantime?

I'm going ahead and commit the null check with the first test only. Any further issues can still be addressed later, if necessary.

[bwoebi]

@cmb69 IIRC the second test was largely "luck" dependent, depending on whether the memory it points to is containing a valid pointer or such.

[cmb69]

@bwoebi, ah, thanks for clarifying. So the test is somewhat useless anyway.

[cmb69]

I have reverted the commit, since it broke PHP-8.1 and master, and according to @dstogov, "the real problem is the improper call frame construction in ext/reflection".