[php-src] PR #24276: Fix GH-8739: OPcache restart corrupts SHM in threaded SAPIs

Pull Request: https://github.com/php/php-src/pull/24276 Author: pavimus Prepared with an AI assistant (Claude), which also ran the reproduction and the tests on my machine; I reviewed the result. The text below is AI-generated (quoted, as asked in CONTRIBUTING.md). > We run PHP 8.5 ZTS under FrankenPHP and hit crashes on every OPcache restart under load. This > PR fixes the restart race for threaded SAPIs, including multi-process ones (Apache event/worker), > building on the approach discussed in #22281 by @nacholibre, who is credited as co-author of the > main commit. > > ### Problem > > Readers of the OPcache SHM are tracked with an fcntl() read > lock on byte 1 of the lock file, > and a scheduled restart (OOM, hash overflow, opcache_reset()) > runs in RINIT only when > accel_is_inactive() finds no reader. POSIX record locks belong > to the process, so under ZTS: > > 1. F_GETLK ignores the locks of the calling process: threads of > the restarting process are > invisible and the restart wipes SHM while they execute code from it (GH-8739 for threaded > SAPIs; FrankenPHP: php/frankenphp#2170, php/frankenphp#1737). > 2. Any thread finishing its request calls accel_unlock_all(), > which unlocks the whole file and > thereby the reader lock of the entire process; with several multi-threaded processes (Apache > event/worker) another process then sees no reader. (It also drops the SHM write lock of > other > threads — fixed by the first commit, see below.) > > Signatures: zend_mm_heap corrupted, SIGSEGV with > opline->handler == NULL, garbage in > op_array/prev_execute_data. > > ### Model > > As suggested by @morrisonlevi in #22281: Windows — atomic model only (unchanged); POSIX NTS > — > process model only (unchanged); POSIX ZTS — both. Threads of a process are counted in a > process-local atomic counter, and the process holds the fcntl() > reader lock on byte 1 whenever > the counter is not zero, so other processes still see it and the kernel still releases it when > a > process dies. This is the composition @nacholibre proposed in #22281 on 2026-06-18. > > ### Changes (all under #if defined(ZTS) && > !defined(ZEND_WIN32), ZendAccelerator.c only) > > - **One acquisition point.** A request becomes a reader only in RINIT, right after the restart > block and before the last_restart_time check, and stays one > until post_deactivate > (ZCG(counted), as on Windows). Lazy acquisition > (accel_activate_add()) fails under ZTS, so > a request that was not admitted never starts to use SHM later (file cache, interned strings). > - **Counter.** Enter: CAS "increment only if > 0"; otherwise, under a > process-local mutex, > F_SETLK F_RDLCK byte 1 and count 0 -> 1. Leave: CAS > "decrement only if > 1"; otherwise, under > the mutex, 1 -> 0 and F_UNLCK byte 1. > fcntl() and the mutex are only used on 0 <-> 1. > - **Restart gate.** The restarting thread (holding > zend_shared_alloc_lock()) takes the mutex, > requires a zero counter and a non-blocking F_SETLK F_WRLCK on > byte 1, and holds both until > the restart is done. No thread of the process can become a reader (the counter only grows > from > zero under the mutex), 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. Check and acquisition are > one > kernel operation, so there is no window between "no readers" and "restart > starts" (the stock > F_GETLK probe, and #22281, check first and lock later). > - **accel_unlock_all()** releases only the thread's > reference under ZTS, never the whole file. > - **force_restart_timeout.** Other processes are still killed > by kill_all_lockers(). Threads > of the own process cannot be killed: a warning (PID, number of threads, delay) is logged at > most > once per timeout period. > - **Thread retirement.** If a thread is destroyed while still counted (its > post_deactivate was > skipped), the globals destructor releases its reference (using only the passed globals: > ts_free_id() runs destructors of all threads from one > thread). > - Builds without real atomics (HAVE_NO_ATOMICS) skip the > lock-free paths and always take the > mutex. > > **No ABI change:** no change to > zend_accel_globals/zend_accel_shared_globals, > no new > ZEND_API symbol, no change to > Zend/zend_atomic.h (the existing load/store/CAS are enough). > **No lock on the read path:** one uncontended CAS per request on enter and on leave. > **NTS and Windows:** no change in the compiled code (we compared > ZendAccelerator.o of NTS debug > builds: only __LINE__ immediates differ). > > ### Behaviour while a restart waits > > ZCSG(accelerator_enabled) is false: new requests run uncached > and are not counted, so the restart > runs as soon as the requests that were running when it was scheduled are done. A thread that > wants > to become a reader while the restart itself runs waits on the mutex for its duration. > > **FrankenPHP worker mode:** OPcache RINIT/post_deactivate run > once per worker lifetime and a > worker keeps classes from SHM between HTTP requests, so a worker is a reader for its whole life > and a restart waits until workers are restarted (classic requests run uncached meanwhile). Not > counting workers would bring the crash back. The SAPI can restart its workers asynchronously > from > zend_accel_schedule_restart_hook. > > ### Known limitations > > - **Tracing JIT:** on AArch64 restarts with tracing JIT also crashed because of a separate JIT > bug (stub veneers are not reset by zend_jit_restart()), fixed > in #24273. With both fixes we > still saw rare tracing-JIT-only issues under ZTS (3/40 runs where the restart did not happen > within 40 s, 1/10 crash on Apache) that we have not diagnosed yet. Without JIT and with > opcache.jit=function we saw none. > - **Preloading + JIT:** requests that start while a restart is pending are not counted but > still > run preloaded functions, including JIT code that zend_jit_restart()®i[ > ËÙÿFà#Å­’$ rewinds (this also applies > across NTS processes today). Out of scope here. > - pcntl_fork() from a request with active readers is not > supported (the child copies a positive > counter but not the lock). > > ### Testing > > Reproduction harness (Docker; FrankenPHP 1.13.0 classic/worker built from source against this > branch, Apache 2.4 event MPM 3-4 children x 32 threads with mod_php ZTS; a generated app of > 2520 > class files with inheritance, 50-500 ms requests; > opcache.memory_consumption=32, SHM filled under > load, then files touched every 5 s to force OOM restarts, or > opcache_reset()). AArch64 Linux. > > | Scenario | stock PHP-8.4 | #22281 | this PR | > | --- | --- | --- | --- | > | FrankenPHP 32 threads, OOM restarts | 5/5 crash | 0/5 | 0/10 (40 restarts) | > | same, opcache.jit=function | — | — | 0/10 (40 restarts) | > | FrankenPHP, hash overflow restarts | 5/5 crash | 0/5 | 0/10 | > | FrankenPHP worker mode + classic requests | 5/5 crash | no restart | no restart (by design, > see above) | > | FrankenPHP 1 thread (control) | 0/5 | 0/5 | 0/5 | > | Apache event, OOM restarts | 5/5 crash | 5/5 crash | 0/10 (40 restarts) | > | Apache event, opcache_reset() every 5 s | 5/5 crash | 5/5 > crash | 0/10 (70 restarts) | > | Apache event, a child killed with SIGKILL before every reset | — | — | 0/3, 21/21 > restarts done | > | 45 min each: FrankenPHP 8..64 threads with load bursts (69 threads started, 13 retired) / > Apache event, file edits every 8 s | — | — | no crash, 146 / 105 restarts | > > make test for ext/opcache and > Zend/tests (5881 tests): identical results to stock on ZTS and > NTS debug builds (0 failures). Builds with --enable-werror, > also with --disable-opcache-jit. > Merges into PHP-8.5 cleanly, into PHP-8.6 with a trivial conflict; master needs C11 atomics > instead > of zend_atomic (branch available). > > **Performance** (FrankenPHP classic, 32 threads, wrk, builds interleaved): hello world at 64 > connections 81.3k (stock) vs 81.8k RPS (5 rounds, medians); at 1 connection — every request > takes > the 0 <-> 1 path with the mutex and two fcntl() — 16.1k > vs 16.2k RPS. Under contention stock pays > two fcntl() per request, this PR none on the fast path. > > ### Relation to other work > > - #22281 (@nacholibre): same idea (process-local counter, eager accounting in RINIT); this PR > adds > the multi-process composition proposed there, closes the check-then-act window, avoids the > ABI > change and handles the timeout. Credit to @nacholibre for the approach. > - #21778 (epoch-based reclamation) and #14803 (lock on the read path, rejected for performance) > address the same restart race differently. > - The first commit is also proposed separately as #24272 (multi-process ZTS crashes without any > restart). 🤖 Generated with [Claude Code](https://claude.com/claude-code)