From 02fdf5aa1a6651487baac3d4fbc6e47c2ef7c551 Mon Sep 17 00:00:00 2001 From: Pavel Gushcha Date: Tue, 6 Oct 2026 17:32:26 +0000 Subject: [PATCH 1/2] Fix OPcache releasing other threads' SHM locks under ZTS accel_unlock_all() runs at the end of every request and releases the whole lock file with F_UNLCK (l_start = 0, l_len = 0). fcntl() record locks are owned by the process, not by the thread, so under ZTS a thread finishing its request also releases the SHM write lock (byte 0) held by another thread of the same process inside zend_shared_alloc_lock(), and the restart lock (byte 2). With several multi-threaded processes sharing the cache (Apache mod_php with the event or worker MPM) another process can then take the write lock while the first one is still writing, and two processes modify SHM at the same time. Children crash during warm-up even without any OPcache restart (SIGSEGV in accel_find_interned_string() and zend_hash_find_bucket()). Bytes 0 and 2 are always released by the thread that took them (zend_shared_alloc_unlock(), zend_shared_alloc_safe_unlock(), accel_restart_leave()), so under ZTS only release the reader lock. NTS is unchanged. --- ext/opcache/ZendAccelerator.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index 79c7668ba5e9..63dcf702fb72 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -393,8 +393,16 @@ static inline void accel_unlock_all(void) mem_usage_unlock_all.l_type = F_UNLCK; mem_usage_unlock_all.l_whence = SEEK_SET; +#ifdef ZTS + /* fcntl() locks belong to the process: unlocking the whole file would also release + * the SHM write lock (byte 0) and the restart lock (byte 2) held by other threads. + * Those are always released by the thread that took them. */ + mem_usage_unlock_all.l_start = 1; + mem_usage_unlock_all.l_len = 1; +#else mem_usage_unlock_all.l_start = 0; mem_usage_unlock_all.l_len = 0; +#endif if (fcntl(lock_file, F_SETLK, &mem_usage_unlock_all) == -1) { zend_accel_error(ACCEL_LOG_DEBUG, "UnlockAll: %s (%d)", strerror(errno), errno); From 5492ce318d0b029309652b107ee6cec3cd34caee Mon Sep 17 00:00:00 2001 From: Pavel Gushcha Date: Tue, 6 Oct 2026 17:38:47 +0000 Subject: [PATCH 2/2] Fix GH-8739: OPcache restart corrupts SHM in threaded SAPIs Readers of the shared memory are tracked with an fcntl() read lock on byte 1 of the lock file, and a restart only runs when no process holds it. Record locks belong to the process, so under ZTS this does not see threads: F_GETLK ignores the locks of the calling process, and the restart (OOM, hash overflow, opcache_reset()) wipes SHM while other threads of the same process still execute code from it. With several multi-threaded processes (Apache event/worker MPM) any thread ending its request also released the reader lock of the whole process. Result: crashes ("zend_mm_heap corrupted", SIGSEGV) under FrankenPHP and Apache mod_php ZTS. Under ZTS on POSIX (NTS and Windows are unchanged): - A thread becomes a reader only in RINIT, before the restart generation is checked, and stays one until post_deactivate. The threads of a process are counted in a process-local atomic counter; the process holds the reader lock on byte 1 whenever the counter is not zero. Only the 0 <-> 1 transitions take a mutex and call fcntl(), other requests do a single compare-and-swap on enter and on leave. - Lazy reader locks (accel_activate_add()) fail under ZTS, so a request that was not admitted in RINIT never starts to use SHM later (e.g. through the file cache or interned strings). - A restart takes the mutex, requires a zero counter and a write lock on byte 1, and holds both until it is finished. Threads of the process cannot become readers meanwhile, and the kernel neither grants the write lock while another process holds the reader lock nor the reader lock while the write lock is held. This replaces the F_GETLK probe, so there is no window between checking for readers and starting the restart. - Threads cannot be killed after opcache.force_restart_timeout: a warning is logged once per timeout period instead. Other processes are still killed by kill_all_lockers(). - A thread retired without its request cleanup releases its reference in the globals destructor. - Without real atomics (HAVE_NO_ATOMICS) every enter and leave takes the mutex. FrankenPHP worker threads stay readers for their whole life, so a restart waits until the workers are restarted. Not covered: requests that start while a restart is pending still run preloaded functions, including their JIT code. Co-authored-by: Yoan Arnaudov <1738437+nacholibre@users.noreply.github.com> --- ext/opcache/ZendAccelerator.c | 180 ++++++++++++++++++++++++++++++++-- 1 file changed, 172 insertions(+), 8 deletions(-) diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index 63dcf702fb72..42f0e5e07523 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -333,6 +333,82 @@ static inline int accel_restart_is_active(void) return 0; } +#if defined(ZTS) && !defined(ZEND_WIN32) +/* fcntl() locks belong to the process, so under ZTS they cannot tell which threads + * use SHM (GH-8739). The threads of this process that use SHM are counted here, and + * the process holds the reader lock (byte 1) whenever the count is not zero, so other + * processes still see it. A restart needs a zero count and a write lock on byte 1. + * The 0 <-> 1 transitions and the restart are serialized by accel_zts_readers_mutex, + * other changes of the count are lock-free. */ +static MUTEX_T accel_zts_readers_mutex = NULL; +static zend_atomic_int accel_zts_readers; +static time_t accel_zts_next_warning; + +static bool accel_zts_lock_reader_byte(int type) +{ + struct flock lock; + + lock.l_type = type; + lock.l_whence = SEEK_SET; + lock.l_start = 1; + lock.l_len = 1; + + return fcntl(lock_file, F_SETLK, &lock) != -1; +} + +/* Must only be called from RINIT; on success the caller sets ZCG(counted) */ +static bool accel_zts_reader_enter(void) +{ + int count; + +#ifndef HAVE_NO_ATOMICS + /* the process already holds the reader lock: only a positive count may grow */ + count = zend_atomic_int_load_ex(&accel_zts_readers); + while (count > 0) { + if (zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count + 1)) { + return true; + } + } +#endif + + tsrm_mutex_lock(accel_zts_readers_mutex); + count = zend_atomic_int_load_ex(&accel_zts_readers); + /* fails while another process restarts the cache (it holds the write lock) */ + if (count == 0 && !accel_zts_lock_reader_byte(F_RDLCK)) { + zend_accel_error(ACCEL_LOG_DEBUG, "UpdateC(+1): %s (%d)", strerror(errno), errno); + tsrm_mutex_unlock(accel_zts_readers_mutex); + return false; + } + while (!zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count + 1)); + tsrm_mutex_unlock(accel_zts_readers_mutex); + return true; +} + +/* Releases one reference; does not use ZCG() as it is also called from accel_globals_dtor() */ +static void accel_zts_reader_leave(void) +{ + int count; + +#ifndef HAVE_NO_ATOMICS + count = zend_atomic_int_load_ex(&accel_zts_readers); + while (count > 1) { + if (zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count - 1)) { + return; + } + } +#endif + + tsrm_mutex_lock(accel_zts_readers_mutex); + count = zend_atomic_int_load_ex(&accel_zts_readers); + while (!zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count - 1)); + if (count == 1) { + /* the last reader of this process; unlock before another thread may relock */ + accel_zts_lock_reader_byte(F_UNLCK); + } + tsrm_mutex_unlock(accel_zts_readers_mutex); +} +#endif + /* Creates a read lock for SHM access */ static inline zend_result accel_activate_add(void) { @@ -340,6 +416,10 @@ static inline zend_result accel_activate_add(void) SHM_UNPROTECT(); INCREMENT(mem_usage); SHM_PROTECT(); +#elif defined(ZTS) + /* A thread becomes a reader only in RINIT, see accel_zts_reader_enter(). A request + * that was not admitted there must not start to use SHM later. */ + return FAILURE; #else struct flock mem_usage_lock; @@ -366,6 +446,8 @@ static inline void accel_deactivate_sub(void) ZCG(counted) = false; SHM_PROTECT(); } +#elif defined(ZTS) + /* nothing to release: accel_activate_add() never succeeds under ZTS */ #else struct flock mem_usage_unlock; @@ -384,6 +466,13 @@ static inline void accel_unlock_all(void) { #ifdef ZEND_WIN32 accel_deactivate_sub(); +#elif defined(ZTS) + /* Release this thread's reference only. Never unlock the whole file: fcntl() locks + * belong to the process, and the SHM write lock (byte 0) and the restart lock + * (byte 2) may be held by other threads. */ + if (ZCG(counted)) { + accel_zts_reader_leave(); + } #else if (lock_file == -1) { return; @@ -393,16 +482,8 @@ static inline void accel_unlock_all(void) mem_usage_unlock_all.l_type = F_UNLCK; mem_usage_unlock_all.l_whence = SEEK_SET; -#ifdef ZTS - /* fcntl() locks belong to the process: unlocking the whole file would also release - * the SHM write lock (byte 0) and the restart lock (byte 2) held by other threads. - * Those are always released by the thread that took them. */ - mem_usage_unlock_all.l_start = 1; - mem_usage_unlock_all.l_len = 1; -#else mem_usage_unlock_all.l_start = 0; mem_usage_unlock_all.l_len = 0; -#endif if (fcntl(lock_file, F_SETLK, &mem_usage_unlock_all) == -1) { zend_accel_error(ACCEL_LOG_DEBUG, "UnlockAll: %s (%d)", strerror(errno), errno); @@ -942,6 +1023,46 @@ static inline bool accel_is_inactive(void) return false; } +#if defined(ZTS) && !defined(ZEND_WIN32) +/* Replaces accel_is_inactive() under ZTS; called under zend_shared_alloc_lock(). + * On success the readers mutex and the write lock on byte 1 are held until + * accel_zts_restart_end(): no thread of this process can become a reader (the count is + * zero and may only grow from zero under the mutex), and other processes can neither + * hold the reader lock nor take it. */ +static bool accel_zts_restart_begin(void) +{ + int readers; + + tsrm_mutex_lock(accel_zts_readers_mutex); + readers = zend_atomic_int_load_ex(&accel_zts_readers); + if (readers == 0 && accel_zts_lock_reader_byte(F_WRLCK)) { + return true; + } + tsrm_mutex_unlock(accel_zts_readers_mutex); + + if (readers == 0) { + /* readers in other processes: keep the force_restart_timeout handling */ + (void) accel_is_inactive(); + } else if (ZCG(accel_directives).force_restart_timeout + && ZCSG(force_restart_time) + && time(NULL) >= ZCSG(force_restart_time) + && time(NULL) >= accel_zts_next_warning) { + /* threads cannot be killed, and this process must not kill itself */ + zend_accel_error(ACCEL_LOG_WARNING, "Restart delayed for %ld seconds by %d threads of process %d still using the cache", + (long)(time(NULL) - ZCSG(force_restart_time) + ZCG(accel_directives).force_restart_timeout), readers, getpid()); + accel_zts_next_warning = time(NULL) + ZCG(accel_directives).force_restart_timeout; + } + return false; +} + +static void accel_zts_restart_end(void) +{ + accel_zts_lock_reader_byte(F_UNLCK); + accel_zts_next_warning = 0; + tsrm_mutex_unlock(accel_zts_readers_mutex); +} +#endif + static int zend_get_stream_timestamp(const char *filename, zend_stat_t *statbuf) { php_stream_wrapper *wrapper; @@ -2708,7 +2829,11 @@ ZEND_RINIT_FUNCTION(zend_accelerator) if (ZCSG(restart_pending)) { zend_shared_alloc_lock(); if (ZCSG(restart_pending)) { /* check again, to ensure that the cache wasn't already cleaned by another process */ +#if defined(ZTS) && !defined(ZEND_WIN32) + if (accel_zts_restart_begin()) { +#else if (accel_is_inactive()) { +#endif zend_accel_error(ACCEL_LOG_DEBUG, "Restarting!"); ZCSG(restart_pending) = false; switch ZCSG(restart_reason) { @@ -2748,6 +2873,9 @@ ZEND_RINIT_FUNCTION(zend_accelerator) ZCSG(last_restart_time)++; } accel_restart_leave(); +#if defined(ZTS) && !defined(ZEND_WIN32) + accel_zts_restart_end(); +#endif } } zend_shared_alloc_unlock(); @@ -2755,6 +2883,19 @@ ZEND_RINIT_FUNCTION(zend_accelerator) ZCG(accelerator_enabled) = ZCSG(accelerator_enabled); +#if defined(ZTS) && !defined(ZEND_WIN32) + /* Under ZTS this is the only place where a request starts to use SHM */ + if (ZCG(accelerator_enabled)) { + if (accel_zts_reader_enter()) { + ZCG(counted) = true; + /* SHM-backed keys may be added to the PCRE cache again */ + ZCG(pcre_reseted) = false; + } else { + ZCG(accelerator_enabled) = false; + } + } +#endif + SHM_PROTECT(); HANDLE_UNBLOCK_INTERRUPTIONS(); @@ -2979,6 +3120,16 @@ static void accel_globals_ctor(zend_accel_globals *accel_globals) static void accel_globals_dtor(zend_accel_globals *accel_globals) { +#if defined(ZTS) && !defined(ZEND_WIN32) + /* The thread is retired without its request cleanup (e.g. another extension + * bailed out in post_deactivate): release its reference, otherwise restarts would + * wait for it forever. Uses only the given globals: ts_free_id() calls this for + * all threads from the current one. */ + if (accel_globals->counted && accel_zts_readers_mutex) { + accel_globals->counted = false; + accel_zts_reader_leave(); + } +#endif zend_string_free(accel_globals->key); if (accel_globals->preloaded_internal_run_time_cache) { pefree(accel_globals->preloaded_internal_run_time_cache, 1); @@ -3304,6 +3455,13 @@ static zend_result accel_post_startup(void) /* from this point further, shared memory is supposed to be OK */ +#if defined(ZTS) && !defined(ZEND_WIN32) + /* before preloading runs its request and before SAPIs fork their children */ + accel_zts_readers_mutex = tsrm_mutex_alloc(); + ZEND_ATOMIC_INT_INIT(&accel_zts_readers, 0); + accel_zts_next_warning = 0; +#endif + /* remember the last restart time in the process memory */ ZCG(last_restart_time) = ZCSG(last_restart_time); @@ -3408,6 +3566,12 @@ static void (*orig_post_shutdown_cb)(void); static void accel_post_shutdown(void) { zend_shared_alloc_shutdown(); +#if defined(ZTS) && !defined(ZEND_WIN32) + if (accel_zts_readers_mutex) { + tsrm_mutex_free(accel_zts_readers_mutex); + accel_zts_readers_mutex = NULL; + } +#endif } void accel_shutdown(void)