Fix GH-16414: zend_test.observer.observe_function_names may segfault #16438
Conversation
|
@cmb69 No, the proper fix would be simply checking for |
|
The new test segfaults on aarch64. :/ |
|
@cmb69 |
|
@cmb69 the aarch runner runs with the observer already enabled. I suppose it's missing handling for that case when the function wasn't called yet... |
It seems we're back at square one. :) |
Unless `zend_test.observer.enabled` is on, we must not add observer handlers, so we let the INI modify handler fail early. Proper fix suggested by @bwoebi.
We also need to ensure that the functions to observe have already been called, so that their begin and end handlers are properly initialized. Otherwise we will not observe the function execution, but a segfault.
|
Happily hacking along. I think this is okay, but would not work for PHP-8.2 and 8.3 due to |
|
@cmb69 Yes, tracking via hashtable is the right way to do. But I agree, for the purposes of zend_test, this is fine. |
| if (stage != PHP_INI_STAGE_STARTUP && stage != PHP_INI_STAGE_ACTIVATE && stage != PHP_INI_STAGE_DEACTIVATE && stage != PHP_INI_STAGE_SHUTDOWN) { | ||
| ZEND_HASH_FOREACH_STR_KEY(*p, funcname) { | ||
| if ((func = zend_hash_find_ptr(EG(function_table), funcname))) { | ||
| if ((func = zend_hash_find_ptr(EG(function_table), funcname)) && ZEND_OBSERVER_DATA(func) != NULL) { |
There was a problem hiding this comment.
I don't understand this. ZEND_OBSERVER_DATA() expands to &RUN_TIME_CACHE(op_array)[handle], which cannot be NULL (except for RUN_TIME_CACHE(op_array) == NULL && handle == 0, which is undefined behavior). Removing these two conditions still makes the test pass. Should this have dereferenced the pointer?
There was a problem hiding this comment.
Actually, it probably must handle ZEND_OBSERVER_NOT_OBSERVED and ZEND_OBSERVER_NONE_OBSERVED. But I forgot about the details of this api.
There was a problem hiding this comment.
@iluuu1994 The value is NULL if the observer has not been initialized yet. It's non-NULL once the init handler ran, assigning a specific handler or one of these two constants. Here we only care about initialized.
But it seems to be missing a dereference, yes. (In PHP 8.3 and before it evaluated to the first value. Hence I missed this.)
There was a problem hiding this comment.
I'll have a look as soon as possible.
There was a problem hiding this comment.
@iluuu1994, this PR targets PHP-8.4; the definition of ZEND_OBSERVER_DATA() has changed there:
Lines 39 to 40 in 5892991
That part is needed if zend_test.observer.enabled=1 upfront (default for our Circle CI job).
@bwoebi, when running the test with zend_test.observer.enabled=1, ZEND_OBSERVER_DATA(func) is NULL for me.
There was a problem hiding this comment.
It's because RUN_TIME_CACHE(&func->common) is NULL, so doing pointer arithmetics on it is UB. In that case, I think we should check whether RUN_TIME_CACHE(&func->common) is set. Note that you're lucky here, if observer had a higher handle, this check would break.
There was a problem hiding this comment.
Ah, thank you! I'll have a closer look again, and might actually go with the hash table.
php-src/Zend/zend_observer.h
Lines 62 to 63 in 5955ce8
How would poor ext/zend_test know whether
zend_observer_fcall_register()had been called? Can that function call be observed? ;)Kidding aside, I don't think this patch is the proper fix (since the issue is related to ext/zend_test, and not really to observers). Instead, it should be possible to check whether an fcall has been registered; unfortunately, there appears to be no API for that, and
zend_observers_fcall_listis static.