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/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 dca5f607ad39..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(); } @@ -402,6 +402,31 @@ static inline void accel_unlock_all(void) #endif } +#if defined(ZTS) && !defined(ZEND_WIN32) +/* 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(reader_counted)) { + zend_atomic_int_fetch_add_ex(&accel_active_readers, 1); + ZCG(reader_counted) = true; + } +} + +static zend_always_inline void accel_reader_leave(void) +{ + if (ZCG(reader_counted)) { + zend_atomic_int_fetch_sub_ex(&accel_active_readers, 1); + ZCG(reader_counted) = false; + } +} +#endif + /* Interned strings support */ /* O+ disables creation of interned strings by regular PHP compiler, instead, @@ -902,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; @@ -2742,6 +2776,24 @@ 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_reader_enter(); + if (UNEXPECTED(ZCSG(restart_in_progress))) { + /* 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; + } + } +#endif + SHM_PROTECT(); HANDLE_UNBLOCK_INTERRUPTIONS(); @@ -2780,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; diff --git a/ext/opcache/ZendAccelerator.h b/ext/opcache/ZendAccelerator.h index 486074ef0012..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,6 +198,9 @@ typedef struct _zend_accel_directives { typedef struct _zend_accel_globals { bool counted; /* the process uses shared memory */ +#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 */ @@ -265,7 +269,11 @@ 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; #endif bool restart_in_progress;