Fix GH-17715: Handle preloaded internal function runtime cache by bwoebi · Pull Request #17835 · php/php-src · GitHub

This solely affects the builtin enum functions currently.

Given that these are stored in SHM, we cannot simply hardwire a pointer into the internal function runtime cache on NTS too, but have to use a MAP_PTR (like on ZTS). Now, by design, the runtime cache of internal functions no longer is reset between requests, hence we need to store them explicitly as static runtime cache.

On NTS builds we cannot trivially move the pointers into CG(internal_run_time_cache) as they're directly stored on the individual functions (on ZTS we could simply iterate the static map_ptrs). Hence, we have the choice between having opcache managing the internal run_time_cache for its preloaded functions itself or realloc CG(internal_run_time_cache) and iterate through all functions to assign the new address. We choose the latter for simplicity and initial speed.

This changes the accel globals structs, but that should be fine given that they're not exported.
map_ptr_static_last has been added as last element to zend_accel_shared_globals, so that accesses to it are compatible. We do not have to care about the ABI of creating new zend_accel_shared_globals structs. That's opcaches prerogative.

I didn't understand the problem from the PR name and comments.
Only the issue clarifies that the problem is related to observer.

Given that these are stored in SHM, we cannot simply hardwire a pointer into the internal function runtime cache on NTS too, but have to use a MAP_PTR (like on ZTS).

I can't understand this.

Now, by design, the runtime cache of internal functions no longer is reset between requests, hence we need to store them explicitly as static runtime cache.

I don't know about this design.
Run time cache for internal functions was introduced by you exactly for observer support.
Initially, I thought it's a good idea, but now observer related code infected into core parts of the engine and opcache...
This made observer one of the most things that I dislike.
Sorry, I don't understand the patch and if all of its details are safe.

@iluuu1994 this is related to PHP Enums. Please also take a look.

Noted. :)

I can't understand this.

op_arrays generated by preloaded scripts are stored in shm. Thus we need to use MAP_PTRs for NTS too. That's it.

This is hard to understand (especially after a day of work at my job).
It's more complexity over the original PR that introduced the issue. Is this worth the cost to just make observers faster? I suppose the original PR can't be reverted at this point due to ABI constraints?

This was quite worth the cost, yes :-(
I'm not happy about the extra complexity and may have to think about streamlining this better. But that's not for now.

I did have that assumption that all persistent internal functions were loaded before post_startup. Evidently preloaded enums violate this assumption...

I understand the concept better now, but I still find it too complex to understand how this will interact with all of the details and possible implicit assumptions made in other parts of PHP.

I ran your test under Valgrind and I got the following Valgrind errors:

==226054== Invalid write of size 8
==226054==    at 0x9EF417: zend_observer_fcall_install (zend_observer.c:126)
==226054==    by 0x9EFAF4: zend_observer_fcall_begin_prechecked (zend_observer.c:262)
==226054==    by 0x8E3E8B: zend_observer_fcall_begin_specialized (zend_observer.h:116)
==226054==    by 0x8F7BC0: ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER (zend_vm_execute.h:2141)
==226054==    by 0x976552: execute_ex (zend_vm_execute.h:58859)
==226054==    by 0x97AED8: zend_execute (zend_vm_execute.h:64236)
==226054==    by 0xA1B97E: zend_execute_script (zend.c:1934)
==226054==    by 0x7CA094: php_execute_script_ex (main.c:2575)
==226054==    by 0x7CA212: php_execute_script (main.c:2615)
==226054==    by 0xA1DE38: do_cli (php_cli.c:935)
==226054==    by 0xA1EB86: main (php_cli.c:1310)
==226054==  Address 0x7b0f5d8 is 0 bytes after a block of size 8 alloc'd
==226054==    at 0x48457A8: malloc (vg_replace_malloc.c:446)
==226054==    by 0x8779E9: __zend_malloc (zend_alloc.c:3280)
==226054==    by 0x787C911: preload_load (ZendAccelerator.c:4388)
==226054==    by 0x787D3E3: accel_preload (ZendAccelerator.c:4596)
==226054==    by 0x787D94F: accel_finish_startup_preload (ZendAccelerator.c:4717)
==226054==    by 0x787DDFC: accel_finish_startup (ZendAccelerator.c:4862)
==226054==    by 0x7878771: accel_post_startup (ZendAccelerator.c:3373)
==226054==    by 0xA19647: zend_post_startup (zend.c:1105)
==226054==    by 0x7C99DE: php_module_startup (main.c:2324)
==226054==    by 0xA1CD4E: php_cli_startup (php_cli.c:397)
==226054==    by 0xA1EAEA: main (php_cli.c:1277)
==226054== 
==226054== Invalid write of size 8
==226054==    at 0x9EF48C: zend_observer_fcall_install (zend_observer.c:138)
==226054==    by 0x9EFAF4: zend_observer_fcall_begin_prechecked (zend_observer.c:262)
==226054==    by 0x8E3E8B: zend_observer_fcall_begin_specialized (zend_observer.h:116)
==226054==    by 0x8F7BC0: ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER (zend_vm_execute.h:2141)
==226054==    by 0x976552: execute_ex (zend_vm_execute.h:58859)
==226054==    by 0x97AED8: zend_execute (zend_vm_execute.h:64236)
==226054==    by 0xA1B97E: zend_execute_script (zend.c:1934)
==226054==    by 0x7CA094: php_execute_script_ex (main.c:2575)
==226054==    by 0x7CA212: php_execute_script (main.c:2615)
==226054==    by 0xA1DE38: do_cli (php_cli.c:935)
==226054==    by 0xA1EB86: main (php_cli.c:1310)
==226054==  Address 0x7b0f5d8 is 0 bytes after a block of size 8 alloc'd
==226054==    at 0x48457A8: malloc (vg_replace_malloc.c:446)
==226054==    by 0x8779E9: __zend_malloc (zend_alloc.c:3280)
==226054==    by 0x787C911: preload_load (ZendAccelerator.c:4388)
==226054==    by 0x787D3E3: accel_preload (ZendAccelerator.c:4596)
==226054==    by 0x787D94F: accel_finish_startup_preload (ZendAccelerator.c:4717)
==226054==    by 0x787DDFC: accel_finish_startup (ZendAccelerator.c:4862)
==226054==    by 0x7878771: accel_post_startup (ZendAccelerator.c:3373)
==226054==    by 0xA19647: zend_post_startup (zend.c:1105)
==226054==    by 0x7C99DE: php_module_startup (main.c:2324)
==226054==    by 0xA1CD4E: php_cli_startup (php_cli.c:397)
==226054==    by 0xA1EAEA: main (php_cli.c:1277)
==226054== 
==226054== Invalid read of size 8
==226054==    at 0x9EFB11: zend_observer_fcall_begin_prechecked (zend_observer.c:269)
==226054==    by 0x8E3E8B: zend_observer_fcall_begin_specialized (zend_observer.h:116)
==226054==    by 0x8F7BC0: ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER (zend_vm_execute.h:2141)
==226054==    by 0x976552: execute_ex (zend_vm_execute.h:58859)
==226054==    by 0x97AED8: zend_execute (zend_vm_execute.h:64236)
==226054==    by 0xA1B97E: zend_execute_script (zend.c:1934)
==226054==    by 0x7CA094: php_execute_script_ex (main.c:2575)
==226054==    by 0x7CA212: php_execute_script (main.c:2615)
==226054==    by 0xA1DE38: do_cli (php_cli.c:935)
==226054==    by 0xA1EB86: main (php_cli.c:1310)
==226054==  Address 0x7b0f5d8 is 0 bytes after a block of size 8 alloc'd
==226054==    at 0x48457A8: malloc (vg_replace_malloc.c:446)
==226054==    by 0x8779E9: __zend_malloc (zend_alloc.c:3280)
==226054==    by 0x787C911: preload_load (ZendAccelerator.c:4388)
==226054==    by 0x787D3E3: accel_preload (ZendAccelerator.c:4596)
==226054==    by 0x787D94F: accel_finish_startup_preload (ZendAccelerator.c:4717)
==226054==    by 0x787DDFC: accel_finish_startup (ZendAccelerator.c:4862)
==226054==    by 0x7878771: accel_post_startup (ZendAccelerator.c:3373)
==226054==    by 0xA19647: zend_post_startup (zend.c:1105)
==226054==    by 0x7C99DE: php_module_startup (main.c:2324)
==226054==    by 0xA1CD4E: php_cli_startup (php_cli.c:397)
==226054==    by 0xA1EAEA: main (php_cli.c:1277)
==226054== 
==226054== Invalid read of size 8
==226054==    at 0x9EFC94: call_end_observers (zend_observer.c:303)
==226054==    by 0x9EFD1C: zend_observer_fcall_end_prechecked (zend_observer.c:315)
==226054==    by 0x8E3ED2: zend_observer_fcall_end (zend_observer.h:126)
==226054==    by 0x8F7D44: ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER (zend_vm_execute.h:2161)
==226054==    by 0x976552: execute_ex (zend_vm_execute.h:58859)
==226054==    by 0x97AED8: zend_execute (zend_vm_execute.h:64236)
==226054==    by 0xA1B97E: zend_execute_script (zend.c:1934)
==226054==    by 0x7CA094: php_execute_script_ex (main.c:2575)
==226054==    by 0x7CA212: php_execute_script (main.c:2615)
==226054==    by 0xA1DE38: do_cli (php_cli.c:935)
==226054==    by 0xA1EB86: main (php_cli.c:1310)
==226054==  Address 0x7b0f5d8 is 0 bytes after a block of size 8 alloc'd
==226054==    at 0x48457A8: malloc (vg_replace_malloc.c:446)
==226054==    by 0x8779E9: __zend_malloc (zend_alloc.c:3280)
==226054==    by 0x787C911: preload_load (ZendAccelerator.c:4388)
==226054==    by 0x787D3E3: accel_preload (ZendAccelerator.c:4596)
==226054==    by 0x787D94F: accel_finish_startup_preload (ZendAccelerator.c:4717)
==226054==    by 0x787DDFC: accel_finish_startup (ZendAccelerator.c:4862)
==226054==    by 0x7878771: accel_post_startup (ZendAccelerator.c:3373)
==226054==    by 0xA19647: zend_post_startup (zend.c:1105)
==226054==    by 0x7C99DE: php_module_startup (main.c:2324)
==226054==    by 0xA1CD4E: php_cli_startup (php_cli.c:397)
==226054==    by 0xA1EAEA: main (php_cli.c:1277)
==226054== 
==226054== Invalid read of size 8
==226054==    at 0x9EFCA0: call_end_observers (zend_observer.c:303)
==226054==    by 0x9EFD1C: zend_observer_fcall_end_prechecked (zend_observer.c:315)
==226054==    by 0x8E3ED2: zend_observer_fcall_end (zend_observer.h:126)
==226054==    by 0x8F7D44: ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER (zend_vm_execute.h:2161)
==226054==    by 0x976552: execute_ex (zend_vm_execute.h:58859)
==226054==    by 0x97AED8: zend_execute (zend_vm_execute.h:64236)
==226054==    by 0xA1B97E: zend_execute_script (zend.c:1934)
==226054==    by 0x7CA094: php_execute_script_ex (main.c:2575)
==226054==    by 0x7CA212: php_execute_script (main.c:2615)
==226054==    by 0xA1DE38: do_cli (php_cli.c:935)
==226054==    by 0xA1EB86: main (php_cli.c:1310)
==226054==  Address 0x7b0f5d8 is 0 bytes after a block of size 8 alloc'd
==226054==    at 0x48457A8: malloc (vg_replace_malloc.c:446)
==226054==    by 0x8779E9: __zend_malloc (zend_alloc.c:3280)
==226054==    by 0x787C911: preload_load (ZendAccelerator.c:4388)
==226054==    by 0x787D3E3: accel_preload (ZendAccelerator.c:4596)
==226054==    by 0x787D94F: accel_finish_startup_preload (ZendAccelerator.c:4717)
==226054==    by 0x787DDFC: accel_finish_startup (ZendAccelerator.c:4862)
==226054==    by 0x7878771: accel_post_startup (ZendAccelerator.c:3373)
==226054==    by 0xA19647: zend_post_startup (zend.c:1105)
==226054==    by 0x7C99DE: php_module_startup (main.c:2324)
==226054==    by 0xA1CD4E: php_cli_startup (php_cli.c:397)
==226054==    by 0xA1EAEA: main (php_cli.c:1277)
==226054== 
==226054== Invalid read of size 8
==226054==    at 0x9EFCC7: call_end_observers (zend_observer.c:309)
==226054==    by 0x9EFD1C: zend_observer_fcall_end_prechecked (zend_observer.c:315)
==226054==    by 0x8E3ED2: zend_observer_fcall_end (zend_observer.h:126)
==226054==    by 0x8F7D44: ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER (zend_vm_execute.h:2161)
==226054==    by 0x976552: execute_ex (zend_vm_execute.h:58859)
==226054==    by 0x97AED8: zend_execute (zend_vm_execute.h:64236)
==226054==    by 0xA1B97E: zend_execute_script (zend.c:1934)
==226054==    by 0x7CA094: php_execute_script_ex (main.c:2575)
==226054==    by 0x7CA212: php_execute_script (main.c:2615)
==226054==    by 0xA1DE38: do_cli (php_cli.c:935)
==226054==    by 0xA1EB86: main (php_cli.c:1310)
==226054==  Address 0x7b0f5d8 is 0 bytes after a block of size 8 alloc'd
==226054==    at 0x48457A8: malloc (vg_replace_malloc.c:446)
==226054==    by 0x8779E9: __zend_malloc (zend_alloc.c:3280)
==226054==    by 0x787C911: preload_load (ZendAccelerator.c:4388)
==226054==    by 0x787D3E3: accel_preload (ZendAccelerator.c:4596)
==226054==    by 0x787D94F: accel_finish_startup_preload (ZendAccelerator.c:4717)
==226054==    by 0x787DDFC: accel_finish_startup (ZendAccelerator.c:4862)
==226054==    by 0x7878771: accel_post_startup (ZendAccelerator.c:3373)
==226054==    by 0xA19647: zend_post_startup (zend.c:1105)
==226054==    by 0x7C99DE: php_module_startup (main.c:2324)
==226054==    by 0xA1CD4E: php_cli_startup (php_cli.c:397)
==226054==    by 0xA1EAEA: main (php_cli.c:1277)

This was quite worth the cost, yes :-(
I'm not happy about the extra complexity and may have to think about streamlining this better. But that's not for now.

Is it still worth the cost? I'm not convinced tbh. I rather revert the original PR. The public ZEND_API fields introduced by the original PR can be hardcoded to zeros.

@nielsdos You're absolutely right. It passed under asan, because asan is 16 byte aligned. Accidentally multiplied by sizeof(void *) instead of cache_size.

If I recall correctly this change had a minor (0.1~0.2%) benefit to performance without observers and 1.5% benefit with observers enabled with symfony demo. I'd be honestly disappointed if rolling it back were the way to go.
What I'd rather do is an improvement of the current situation in master (i.e. have this patch in 8.4, then improve for 8.5) and think about how to streamline this in master.
I was also anticipating that this work could be beneficial for mutable class data and (ironically) for preloaded functions to give yet another small percentage of benefit.
The estimated performance gain was to be assumed to be bigger for preloading (as most calls would ultimately be going to already allocated functions).

I thought some time about this and went over it again and I think I fully understand it. I just have some small nits/questions. I hope there won't be any sneaky problems anymore.
I also tested the preloading in the test suite and Symfony-demo in Valgrind and there were no issue reports.
I hope this can be further cleaned up in master. I particularly don't like how some map_ptr details are spreading inside the accelerator itself. In particular the reallocation logic is very similar/repeated.

Why is this using CG(map_ptr_size) for the number of elements instead of new_static_size + CG(map_ptr_size) ?

Nit: Normally we use C-style comments

I hope no one is doing anything funky ABI-wise, they shouldn't of course...
Anyway, since ABI shouldn't be a concern, can we please put this next to the map_ptr_last field? A bit more cache locality.

This solely affects the builtin enum functions currently. Given that these are stored in SHM, we cannot simply hardwire a pointer into the internal function runtime cache on NTS too, but have to use a MAP_PTR (like on ZTS). Now, by design, the runtime cache of internal functions no longer is reset between requests, hence we need to store them explicitly as static runtime cache. On NTS builds we cannot trivially move the pointers into CG(internal_run_time_cache) as they're directly stored on the individual functions (on ZTS we could simply iterate the static map_ptrs). Hence, we have the choice between having opcache managing the internal run_time_cache for its preloaded functions itself or realloc CG(internal_run_time_cache) and iterate through all functions to assign the new address. We choose the latter for simplicity and initial speed. Note: map_ptr_static_last has been added as last element to zend_accel_shared_globals, so that accesses to it are compatible. We do not have to care about the ABI of creating new zend_accel_shared_globals structs. That's opcaches prerogative.

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>

@nielsdos yeah, master should get a proper extend-API, to avoid doing this stuff in opcache, I fully agree.
Thanks for the thorough reviewing!

I don't see problems anymore. I hope all details are safe and interact properly.

…ache (php#17835)" This reverts commit 53fa98e.

opcache persists CE-cache map_ptr offsets into SHM-interned class-name strings (zend_accel_get_class_name_map_ptr()), allocating them from the persisting process's CG(map_ptr_last). That numbering is process-local, and php-fpm de-synchronizes it across pools of one master: pool-level php_admin_value[extension] lines are loaded post-fork, and each internal function of such an extension claims a dynamic map_ptr slot at MINIT. A CE-cache offset baked by a pool without the extension then lands, in pools with the extension, inside the band of slots owned by internal-function run-time caches. When an fcall observer is registered, zend_init_internal_run_time_cache() fills every such slot with a zero-initialized arena slice each request, and zend_lookup_class_ex()'s CE-cache fast path - whose only guard is slot < CG(map_ptr_last) - returns that slice as a zend_class_entry *. The next static method call does zend_hash_find() on a zeroed function_table and segfaults at 4 * zend_string_hash(method_name). Backport the static map_ptr region from PHP 8.4, where this class of bug is fixed by design: - 25d7616 (phpGH-15040): internal-function run-time caches are allocated from a separate static region via ZEND_MAP_PTR_NEW_STATIC() instead of the per-process dynamic counter, so pool-local extension MINIT no longer advances CG(map_ptr_last) and SHM-persisted CE-cache offsets stay consistent across pools. - The map_ptr_static preload handling from 53fa98e (phpGH-17835): opcache's preload_load() re-allocates the map_ptr base itself and must account for the static region, otherwise the heap is corrupted under opcache.preload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

bwoebi requested a review from dstogov as a code owner February 16, 2025 20:04

github-actions Bot added Category: Engine Extension: opcache labels Feb 16, 2025

bwoebi requested review from ndossche and removed request for dstogov February 16, 2025 20:05

bwoebi mentioned this pull request Feb 16, 2025

Null pointer deref in observer API when calling cases() method on preloaded enum #17715

Closed

bwoebi force-pushed the fix-internal-enum-rt-cache branch from c37d9f8 to ec35e3a Compare February 16, 2025 20:28

cmb69 reviewed Feb 17, 2025

View reviewed changes

bwoebi requested a review from iluuu1994 February 17, 2025 14:26

ndossche requested changes Feb 17, 2025

View reviewed changes

Comment thread ext/opcache/ZendAccelerator.h Outdated

Comment thread ext/opcache/ZendAccelerator.h Outdated

Comment thread ext/opcache/ZendAccelerator.c Outdated

Comment thread ext/opcache/ZendAccelerator.c Outdated

bwoebi force-pushed the fix-internal-enum-rt-cache branch from 4f8cf6e to 7ce04d3 Compare February 17, 2025 21:38

ndossche requested changes Feb 18, 2025

View reviewed changes

github-actions Bot added the ABI break label Feb 20, 2025

bwoebi removed the ABI break label Feb 20, 2025

ndossche requested changes Feb 21, 2025

View reviewed changes

bwoebi added 5 commits February 23, 2025 14:16

Fix bad alignment

e736aeb

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>

Fix cache size

595471c

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>

Move static map ptr number to pointer computation into macro

a39cdef

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>

Address review concerns

2bc5e14

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>

bwoebi force-pushed the fix-internal-enum-rt-cache branch from 2a30bcc to 2bc5e14 Compare February 23, 2025 13:16

github-actions Bot added the ABI break label Feb 23, 2025

ndossche approved these changes Feb 23, 2025

View reviewed changes

bwoebi merged commit 53fa98e into php:PHP-8.4 Feb 24, 2025

kocsismate added a commit to kocsismate/php-src that referenced this pull request Aug 22, 2025

Revert "Fix phpGH-17715 : Handle preloaded internal function runtime c…

49fc38a

…ache (php#17835)" This reverts commit 53fa98e.

Nibbler999 mentioned this pull request Aug 7, 2026

opcache: set ZEND_ACC_PRELOADED for compile-only preloaded enums #23103

Open

nyrzhun mentioned this pull request Aug 18, 2026

PHP-FPM SIGSEGV with OPcache, an observer, and an extension loaded per pool #23355

Open

Fix GH-17715: Handle preloaded internal function runtime cache by bwoebi · Pull Request #17835 · php/php-src · GitHub

Uh oh!

Fix GH-17715: Handle preloaded internal function runtime cache - #17835

Fix GH-17715: Handle preloaded internal function runtime cache#17835bwoebi merged 5 commits intophp:PHP-8.4php/php-src:PHP-8.4from bwoebi:fix-internal-enum-rt-cachebwoebi/php-src:fix-internal-enum-rt-cacheCopy head branch name to clipboard

Conversation

bwoebi commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstogov commented Feb 17, 2025

Uh oh!

cmb69 Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Feb 17, 2025

Uh oh!

ndossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwoebi commented Feb 18, 2025

Uh oh!

ndossche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Feb 18, 2025

Uh oh!

ndossche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndossche Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

ndossche Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

ndossche Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Feb 23, 2025

Uh oh!

ndossche left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix GH-17715: Handle preloaded internal function runtime cache#17835
bwoebi merged 5 commits into
php:PHP-8.4from
bwoebi:fix-internal-enum-rt-cache

bwoebi commented Feb 16, 2025 •
edited
Loading

ndossche left a comment •
edited
Loading

ndossche left a comment •
edited
Loading