Fix GH-16811: Crash in zend_test observer on runtime observe_function_names change#21635
Fix GH-16811: Crash in zend_test observer on runtime observe_function_names change#21635iliaal wants to merge 2 commits intophp:masterfrom
Conversation
…ion_names change OnUpdateCommaList called zend_observer_remove/add_begin_handler without checking whether observer data was initialized. This null-dereferenced when the function had never been called (no runtime cache), and hit ZEND_UNREACHABLE() when observe_all had already installed the same handler. Guard both the remove and add blocks with runtime cache checks. Remove existing handlers before re-adding to prevent slot overflow from duplicates. Closes phpGH-16811
| void *old_handler; | ||
| zend_observer_remove_begin_handler(func, observer_begin, (zend_observer_fcall_begin_handler *)&old_handler); | ||
| zend_observer_remove_end_handler(func, observer_end, (zend_observer_fcall_end_handler *)&old_handler); |
There was a problem hiding this comment.
I don't understand why this is needed. Any previous handlers should have been removed above, no?
There was a problem hiding this comment.
The first loop only removes handlers for functions in the previous comma list. If observe_all=1 installed the handler at first-call time (via observer_fcall_init), that function was never in the old comma list, so the first loop won't touch it. Without the remove here, add_begin_handler installs a duplicate and hits ZEND_UNREACHABLE().
The *ZEND_OBSERVER_DATA(func) check gates this: we only attempt removal when observer data already exists.
|
@iliaal See the test failure, it looks related. |
CircleCI ARM passes -d zend_test.observer.show_output=0 globally. The test relied on the default (1) instead of setting it explicitly, so the <!-- init --> lines were suppressed.
|
The test didn't set |
2 participants
zend_test_observer_OnUpdateCommaListcrashes whenini_setchangesobserve_function_namesat runtime. Two paths:Function exists in the function table but was never called.
RUN_TIME_CACHEis NULL, soZEND_OBSERVER_DATAnull-derefs inzend_observer_remove_handler.observe_all=1already installed the handler at first call.OnUpdateCommaListadds the same handler again, finds no free slot, hitsZEND_UNREACHABLE().Fix: guard remove/add with runtime cache checks, and remove existing handlers before re-adding to prevent duplicates.
Fixes #16811