From 430f1be63f4aa262f1ca4abd5e7f16c16b2e768a Mon Sep 17 00:00:00 2001 From: Yoan Arnaudov <1738437+nacholibre@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:40:04 +0300 Subject: [PATCH 1/2] Fix GH-8739: OPcache restart corrupts SHM in threaded SAPIs (ZTS) Before executing a scheduled restart (OOM, hash overflow or opcache_reset()), accel_is_inactive() checks that no request is still reading shared memory. On POSIX this check is implemented with fcntl() record locks: readers take an F_RDLCK in accel_activate_add() and the restarting thread probes for it with F_GETLK. POSIX record locks are per-process: a process never conflicts with its own locks. In threaded SAPIs (FrankenPHP, mod_php with a threaded MPM), all in-flight requests are threads of one process, so F_GETLK never reports a conflict and accel_is_inactive() always returns true. The restart then runs zend_accel_hash_clean() and accel_interned_strings_restore_state() while other threads execute op_arrays and hold interned string pointers in the wiped memory, corrupting the heap ("zend_mm_heap corrupted", SIGSEGV/SIGABRT). Fix it the way ZEND_WIN32 already solved the same problem for threaded SAPIs on Windows: track in-flight requests with an atomic counter in SHM. Requests register in RINIT (only while the accelerator is enabled, so registrations stop as soon as a restart is pending and the counter drains within the duration of the longest in-flight request) and deregister in accel_post_deactivate(). accel_is_inactive() refuses to restart while the counter is non-zero. The counter is only compiled in for ZTS non-Windows builds; NTS process-based SAPIs (php-fpm, mod_php prefork) keep using fcntl() locks, whose kernel-side cleanup on process death they rely on. Verified on a production Symfony workload under FrankenPHP (256 threads, ~120 concurrent requests): repeated opcache_reset() calls and wasted-memory-exhaustion restarts, both previously crashing within seconds, now complete safely after draining readers. --- NEWS | 3 ++ ext/opcache/ZendAccelerator.c | 58 +++++++++++++++++++++++++++++++++++ ext/opcache/ZendAccelerator.h | 7 +++++ 3 files changed, 68 insertions(+) diff --git a/NEWS b/NEWS index 2698420c7a5a..16737a3849d7 100644 --- a/NEWS +++ b/NEWS @@ -62,6 +62,9 @@ PHP NEWS - Opcache: . Fixed tracing JIT crash when a VM interrupt is handled during an observed user function call. (Levi Morrison) + . Fixed bug GH-8739 (OPcache restart corrupts shared memory under threaded + SAPIs: fcntl()-based reader tracking cannot see threads of the same + process). (Yoan Arnaudov) . Fixed bug GH-22004 (Assertion failure at ext/opcache/jit/zend_jit_trace.c). (Arnaud) diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index dca5f607ad39..453ced9f6d5a 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -402,6 +402,38 @@ static inline void accel_unlock_all(void) #endif } +#if defined(ZTS) && !defined(ZEND_WIN32) +/* In threaded SAPIs (FrankenPHP, mod_php with a threaded MPM, ...) requests + * run as threads of one process. The fcntl() record locks used by + * accel_activate_add()/accel_is_inactive() are per-process: a process never + * conflicts with its own locks, so in-flight readers running as threads are + * invisible to accel_is_inactive(). It then wrongly reports the cache as + * inactive and a scheduled restart wipes SHM (hash table + interned strings) + * under running requests, corrupting the heap (GH-8739, GH-14471, GH-18517). + * + * Mirror the approach ZEND_WIN32 already uses for the same reason (threaded + * SAPIs): track in-flight requests with an atomic counter in SHM and have + * accel_is_inactive() refuse to restart while it is non-zero. Registration + * is gated on ZCG(accelerator_enabled), so once a restart is pending (the + * accelerator is disabled) new requests no longer register and the counter + * drains within the duration of the longest in-flight request. */ +static zend_always_inline void accel_ts_request_enter(void) +{ + if (!ZCG(ts_reader_registered)) { + __atomic_add_fetch(&ZCSG(ts_active_requests), 1, __ATOMIC_SEQ_CST); + ZCG(ts_reader_registered) = true; + } +} + +static zend_always_inline void accel_ts_request_leave(void) +{ + if (ZCG(ts_reader_registered)) { + __atomic_sub_fetch(&ZCSG(ts_active_requests), 1, __ATOMIC_SEQ_CST); + ZCG(ts_reader_registered) = false; + } +} +#endif + /* Interned strings support */ /* O+ disables creation of interned strings by regular PHP compiler, instead, @@ -894,6 +926,13 @@ static inline void kill_all_lockers(struct flock *mem_usage_check) static inline bool accel_is_inactive(void) { +#if defined(ZTS) && !defined(ZEND_WIN32) + /* Threads of this process may still be inside requests registered as + * SHM readers; the fcntl() probe below cannot detect them. */ + if (__atomic_load_n(&ZCSG(ts_active_requests), __ATOMIC_SEQ_CST) != 0) { + return false; + } +#endif #ifdef ZEND_WIN32 /* on Windows, we don't need kill_all_lockers() because SAPIs that work on Windows don't manage child processes (and we @@ -2742,6 +2781,17 @@ ZEND_RINIT_FUNCTION(zend_accelerator) ZCG(accelerator_enabled) = ZCSG(accelerator_enabled); +#if defined(ZTS) && !defined(ZEND_WIN32) + if (ZCG(accelerator_enabled)) { + accel_ts_request_enter(); + if (UNEXPECTED(ZCSG(restart_in_progress))) { + /* lost the race against a restart that passed accel_is_inactive() + * before our registration became visible; run uncached */ + ZCG(accelerator_enabled) = false; + } + } +#endif + SHM_PROTECT(); HANDLE_UNBLOCK_INTERRUPTIONS(); @@ -2793,6 +2843,14 @@ zend_result accel_post_deactivate(void) accel_unlock_all(); ZCG(counted) = false; +#if defined(ZTS) && !defined(ZEND_WIN32) + if (!file_cache_only && accel_shared_globals) { + SHM_UNPROTECT(); + accel_ts_request_leave(); + SHM_PROTECT(); + } +#endif + return SUCCESS; } diff --git a/ext/opcache/ZendAccelerator.h b/ext/opcache/ZendAccelerator.h index 486074ef0012..4031e44d14f4 100644 --- a/ext/opcache/ZendAccelerator.h +++ b/ext/opcache/ZendAccelerator.h @@ -197,6 +197,7 @@ typedef struct _zend_accel_directives { typedef struct _zend_accel_globals { bool counted; /* the process uses shared memory */ + bool ts_reader_registered; /* this request is counted in ZCSG(ts_active_requests) (ZTS only) */ bool enabled; bool locked; /* thread obtained exclusive lock */ bool accelerator_enabled; /* accelerator enabled for current request */ @@ -267,6 +268,12 @@ typedef struct _zend_accel_shared_globals { #ifdef ZEND_WIN32 LONGLONG mem_usage; LONGLONG restart_in; +#elif defined(ZTS) + /* In-flight requests registered as SHM readers. POSIX fcntl() record + * locks cannot track readers that are threads of one process (a process + * never conflicts with its own locks), so threaded SAPIs need an + * explicit counter — mirroring what ZEND_WIN32 does with mem_usage. */ + uint32_t ts_active_requests; #endif bool restart_in_progress; bool jit_counters_stopped; From 40f0087aaf293b1157cc1ffd4b867a3a54d14e59 Mon Sep 17 00:00:00 2001 From: Yoan Arnaudov <1738437+nacholibre@users.noreply.github.com> Date: Wed, 17 Jun 2026 10:42:25 +0300 Subject: [PATCH 2/2] opcache: reuse the Windows atomic reader model on POSIX ZTS Review feedback on GH-8739: rather than a separate POSIX-ZTS counter, use the same atomic model Windows already relies on, via zend_atomic. - Add zend_atomic_int_fetch_add()/fetch_sub(); the API had load, store, exchange and compare-exchange but no atomic add. - Switch ZCSG(mem_usage) from LONGLONG to zend_atomic_int so Windows and POSIX ZTS share the same primitive. - On POSIX ZTS track in-flight readers in a process-local accel_active_readers counter, consulted by accel_is_inactive() before the existing fcntl() probe. It is process-local on purpose: cross-process coordination stays on the fcntl() locks, which the kernel releases if a process dies, keeping that self-healing behaviour. --- Zend/zend_atomic.c | 8 ++++ Zend/zend_atomic.h | 50 ++++++++++++++++++++ ext/opcache/ZendAccelerator.c | 86 +++++++++++++++++------------------ ext/opcache/ZendAccelerator.h | 17 +++---- 4 files changed, 110 insertions(+), 51 deletions(-) diff --git a/Zend/zend_atomic.c b/Zend/zend_atomic.c index 5f6a7793beea..42ce69d02bd7 100644 --- a/Zend/zend_atomic.c +++ b/Zend/zend_atomic.c @@ -57,6 +57,14 @@ ZEND_API void zend_atomic_int_store(zend_atomic_int *obj, int desired) { zend_atomic_int_store_ex(obj, desired); } +ZEND_API int zend_atomic_int_fetch_add(zend_atomic_int *obj, int value) { + return zend_atomic_int_fetch_add_ex(obj, value); +} + +ZEND_API int zend_atomic_int_fetch_sub(zend_atomic_int *obj, int value) { + return zend_atomic_int_fetch_sub_ex(obj, value); +} + #if defined(ZEND_WIN32) || defined(HAVE_SYNC_ATOMICS) /* On these platforms it is non-const due to underlying APIs. */ ZEND_API bool zend_atomic_bool_load(zend_atomic_bool *obj) { diff --git a/Zend/zend_atomic.h b/Zend/zend_atomic.h index 56422a721fdb..38ef01087e28 100644 --- a/Zend/zend_atomic.h +++ b/Zend/zend_atomic.h @@ -88,6 +88,9 @@ BEGIN_EXTERN_C() #ifndef InterlockedCompareExchange #define InterlockedCompareExchange _InterlockedCompareExchange #endif +#ifndef InterlockedExchangeAdd +#define InterlockedExchangeAdd _InterlockedExchangeAdd +#endif #define ZEND_ATOMIC_BOOL_INIT(obj, desired) ((obj)->value = (desired)) #define ZEND_ATOMIC_INT_INIT(obj, desired) ((obj)->value = (desired)) @@ -103,6 +106,14 @@ static zend_always_inline int zend_atomic_int_exchange_ex(zend_atomic_int *obj, return (int) InterlockedExchange(&obj->value, desired); } +static zend_always_inline int zend_atomic_int_fetch_add_ex(zend_atomic_int *obj, int value) { + return (int) InterlockedExchangeAdd(&obj->value, value); +} + +static zend_always_inline int zend_atomic_int_fetch_sub_ex(zend_atomic_int *obj, int value) { + return (int) InterlockedExchangeAdd(&obj->value, -value); +} + static zend_always_inline bool zend_atomic_bool_compare_exchange_ex(zend_atomic_bool *obj, bool *expected, bool desired) { bool prev = (bool) InterlockedCompareExchange8(&obj->value, *expected, desired); if (prev == *expected) { @@ -158,6 +169,14 @@ static zend_always_inline int zend_atomic_int_exchange_ex(zend_atomic_int *obj, return __c11_atomic_exchange(&obj->value, desired, __ATOMIC_SEQ_CST); } +static zend_always_inline int zend_atomic_int_fetch_add_ex(zend_atomic_int *obj, int value) { + return __c11_atomic_fetch_add(&obj->value, value, __ATOMIC_SEQ_CST); +} + +static zend_always_inline int zend_atomic_int_fetch_sub_ex(zend_atomic_int *obj, int value) { + return __c11_atomic_fetch_sub(&obj->value, value, __ATOMIC_SEQ_CST); +} + static zend_always_inline bool zend_atomic_bool_compare_exchange_ex(zend_atomic_bool *obj, bool *expected, bool desired) { return __c11_atomic_compare_exchange_strong(&obj->value, expected, desired, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); } @@ -204,6 +223,14 @@ static zend_always_inline int zend_atomic_int_exchange_ex(zend_atomic_int *obj, return prev; } +static zend_always_inline int zend_atomic_int_fetch_add_ex(zend_atomic_int *obj, int value) { + return __atomic_fetch_add(&obj->value, value, __ATOMIC_SEQ_CST); +} + +static zend_always_inline int zend_atomic_int_fetch_sub_ex(zend_atomic_int *obj, int value) { + return __atomic_fetch_sub(&obj->value, value, __ATOMIC_SEQ_CST); +} + static zend_always_inline bool zend_atomic_bool_compare_exchange_ex(zend_atomic_bool *obj, bool *expected, bool desired) { return __atomic_compare_exchange(&obj->value, expected, &desired, /* weak */ false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); } @@ -260,6 +287,14 @@ static zend_always_inline int zend_atomic_int_exchange_ex(zend_atomic_int *obj, return prev; } +static zend_always_inline int zend_atomic_int_fetch_add_ex(zend_atomic_int *obj, int value) { + return __sync_fetch_and_add(&obj->value, value); +} + +static zend_always_inline int zend_atomic_int_fetch_sub_ex(zend_atomic_int *obj, int value) { + return __sync_fetch_and_sub(&obj->value, value); +} + static zend_always_inline bool zend_atomic_bool_compare_exchange_ex(zend_atomic_bool *obj, bool *expected, bool desired) { bool prev = __sync_val_compare_and_swap(&obj->value, *expected, desired); if (prev == *expected) { @@ -362,6 +397,18 @@ static zend_always_inline int zend_atomic_int_exchange_ex(zend_atomic_int *obj, return prev; } +static zend_always_inline int zend_atomic_int_fetch_add_ex(zend_atomic_int *obj, int value) { + int prev = obj->value; + obj->value = prev + value; + return prev; +} + +static zend_always_inline int zend_atomic_int_fetch_sub_ex(zend_atomic_int *obj, int value) { + int prev = obj->value; + obj->value = prev - value; + return prev; +} + #endif ZEND_API void zend_atomic_bool_init(zend_atomic_bool *obj, bool desired); @@ -376,6 +423,9 @@ ZEND_API bool zend_atomic_int_compare_exchange(zend_atomic_int *obj, int *expect ZEND_API void zend_atomic_bool_store(zend_atomic_bool *obj, bool desired); ZEND_API void zend_atomic_int_store(zend_atomic_int *obj, int desired); +ZEND_API int zend_atomic_int_fetch_add(zend_atomic_int *obj, int value); +ZEND_API int zend_atomic_int_fetch_sub(zend_atomic_int *obj, int value); + #if defined(ZEND_WIN32) || defined(HAVE_SYNC_ATOMICS) /* On these platforms it is non-const due to underlying APIs. */ ZEND_API bool zend_atomic_bool_load(zend_atomic_bool *obj); diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index 453ced9f6d5a..b1239a4b3fc2 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -338,7 +338,7 @@ static inline zend_result accel_activate_add(void) { #ifdef ZEND_WIN32 SHM_UNPROTECT(); - INCREMENT(mem_usage); + zend_atomic_int_fetch_add_ex(&ZCSG(mem_usage), 1); SHM_PROTECT(); #else struct flock mem_usage_lock; @@ -362,7 +362,7 @@ static inline void accel_deactivate_sub(void) #ifdef ZEND_WIN32 if (ZCG(counted)) { SHM_UNPROTECT(); - DECREMENT(mem_usage); + zend_atomic_int_fetch_sub_ex(&ZCSG(mem_usage), 1); ZCG(counted) = false; SHM_PROTECT(); } @@ -403,33 +403,26 @@ static inline void accel_unlock_all(void) } #if defined(ZTS) && !defined(ZEND_WIN32) -/* In threaded SAPIs (FrankenPHP, mod_php with a threaded MPM, ...) requests - * run as threads of one process. The fcntl() record locks used by - * accel_activate_add()/accel_is_inactive() are per-process: a process never - * conflicts with its own locks, so in-flight readers running as threads are - * invisible to accel_is_inactive(). It then wrongly reports the cache as - * inactive and a scheduled restart wipes SHM (hash table + interned strings) - * under running requests, corrupting the heap (GH-8739, GH-14471, GH-18517). - * - * Mirror the approach ZEND_WIN32 already uses for the same reason (threaded - * SAPIs): track in-flight requests with an atomic counter in SHM and have - * accel_is_inactive() refuse to restart while it is non-zero. Registration - * is gated on ZCG(accelerator_enabled), so once a restart is pending (the - * accelerator is disabled) new requests no longer register and the counter - * drains within the duration of the longest in-flight request. */ -static zend_always_inline void accel_ts_request_enter(void) +/* Threaded POSIX SAPIs (e.g. FrankenPHP) run requests as threads of one + * process. fcntl() locks are per-process, so accel_is_inactive() can't see + * sibling threads still reading SHM (GH-8739). Mirror what Windows does with + * the atomic mem_usage counter, but keep it process-local: cross-process + * coordination stays on fcntl(), which self-heals if a process dies. */ +static zend_atomic_int accel_active_readers; + +static zend_always_inline void accel_reader_enter(void) { - if (!ZCG(ts_reader_registered)) { - __atomic_add_fetch(&ZCSG(ts_active_requests), 1, __ATOMIC_SEQ_CST); - ZCG(ts_reader_registered) = true; + if (!ZCG(reader_counted)) { + zend_atomic_int_fetch_add_ex(&accel_active_readers, 1); + ZCG(reader_counted) = true; } } -static zend_always_inline void accel_ts_request_leave(void) +static zend_always_inline void accel_reader_leave(void) { - if (ZCG(ts_reader_registered)) { - __atomic_sub_fetch(&ZCSG(ts_active_requests), 1, __ATOMIC_SEQ_CST); - ZCG(ts_reader_registered) = false; + if (ZCG(reader_counted)) { + zend_atomic_int_fetch_sub_ex(&accel_active_readers, 1); + ZCG(reader_counted) = false; } } #endif @@ -926,13 +919,6 @@ static inline void kill_all_lockers(struct flock *mem_usage_check) static inline bool accel_is_inactive(void) { -#if defined(ZTS) && !defined(ZEND_WIN32) - /* Threads of this process may still be inside requests registered as - * SHM readers; the fcntl() probe below cannot detect them. */ - if (__atomic_load_n(&ZCSG(ts_active_requests), __ATOMIC_SEQ_CST) != 0) { - return false; - } -#endif #ifdef ZEND_WIN32 /* on Windows, we don't need kill_all_lockers() because SAPIs that work on Windows don't manage child processes (and we @@ -941,10 +927,19 @@ static inline bool accel_is_inactive(void) instead of flocks (atomics are much faster but they don't provide us with the PID of locker processes) */ - if (LOCKVAL(mem_usage) == 0) { + if (zend_atomic_int_load_ex(&ZCSG(mem_usage)) == 0) { return true; } #else +#if defined(ZTS) + /* Sibling threads of this process are invisible to the fcntl() probe below + * (locks are per-process), so consult the process-local reader counter + * first. Acts as the whole check for single-process SAPIs like FrankenPHP; + * for multi-process ones the fcntl() probe still covers other processes. */ + if (zend_atomic_int_load_ex(&accel_active_readers) != 0) { + return false; + } +#endif struct flock mem_usage_check; mem_usage_check.l_type = F_WRLCK; @@ -2782,11 +2777,18 @@ ZEND_RINIT_FUNCTION(zend_accelerator) ZCG(accelerator_enabled) = ZCSG(accelerator_enabled); #if defined(ZTS) && !defined(ZEND_WIN32) + /* Register as an active SHM reader for the duration of this request so a + * restart scheduled while we run waits for us (see accel_is_inactive()). + * Done after the restart block above, so the restarting thread never counts + * itself. Gated on accelerator_enabled: once a restart is pending the + * accelerator is disabled, new requests stop registering and the counter + * drains to zero. */ if (ZCG(accelerator_enabled)) { - accel_ts_request_enter(); + accel_reader_enter(); if (UNEXPECTED(ZCSG(restart_in_progress))) { - /* lost the race against a restart that passed accel_is_inactive() - * before our registration became visible; run uncached */ + /* A restart passed accel_is_inactive() before our increment became + * visible and is wiping SHM now; back out and run uncached. */ + accel_reader_leave(); ZCG(accelerator_enabled) = false; } } @@ -2830,6 +2832,12 @@ void accel_deactivate(void) zend_result accel_post_deactivate(void) { +#if defined(ZTS) && !defined(ZEND_WIN32) + /* Stop counting as an active SHM reader (balances accel_reader_enter() in + * RINIT). A no-op if this request never registered. */ + accel_reader_leave(); +#endif + if (ZCG(cwd)) { zend_string_release_ex(ZCG(cwd), 0); ZCG(cwd) = NULL; @@ -2843,14 +2851,6 @@ zend_result accel_post_deactivate(void) accel_unlock_all(); ZCG(counted) = false; -#if defined(ZTS) && !defined(ZEND_WIN32) - if (!file_cache_only && accel_shared_globals) { - SHM_UNPROTECT(); - accel_ts_request_leave(); - SHM_PROTECT(); - } -#endif - return SUCCESS; } diff --git a/ext/opcache/ZendAccelerator.h b/ext/opcache/ZendAccelerator.h index 4031e44d14f4..17448d41d7e9 100644 --- a/ext/opcache/ZendAccelerator.h +++ b/ext/opcache/ZendAccelerator.h @@ -51,6 +51,7 @@ #include "zend_extensions.h" #include "zend_compile.h" #include "zend_API.h" +#include "zend_atomic.h" #include "Optimizer/zend_optimizer.h" #include "zend_accelerator_hash.h" @@ -197,7 +198,9 @@ typedef struct _zend_accel_directives { typedef struct _zend_accel_globals { bool counted; /* the process uses shared memory */ - bool ts_reader_registered; /* this request is counted in ZCSG(ts_active_requests) (ZTS only) */ +#if defined(ZTS) && !defined(ZEND_WIN32) + bool reader_counted; /* this request is counted in accel_active_readers */ +#endif bool enabled; bool locked; /* thread obtained exclusive lock */ bool accelerator_enabled; /* accelerator enabled for current request */ @@ -266,14 +269,12 @@ typedef struct _zend_accel_shared_globals { zend_accel_restart_reason restart_reason; bool cache_status_before_restart; #ifdef ZEND_WIN32 - LONGLONG mem_usage; + /* Count of requests currently reading SHM. Windows threaded SAPIs run in a + * single process, so this atomic counter alone tells us when it is safe to + * restart (see accel_is_inactive()). POSIX uses fcntl() locks instead, with + * an additional process-local counter under ZTS (see accel_active_readers). */ + zend_atomic_int mem_usage; LONGLONG restart_in; -#elif defined(ZTS) - /* In-flight requests registered as SHM readers. POSIX fcntl() record - * locks cannot track readers that are threads of one process (a process - * never conflicts with its own locks), so threaded SAPIs need an - * explicit counter — mirroring what ZEND_WIN32 does with mem_usage. */ - uint32_t ts_active_requests; #endif bool restart_in_progress; bool jit_counters_stopped;