Skip to content

Commit e110a90

Browse files
superdav42claude
andcommitted
Fix GH-8739: opcache_reset()/invalidate() races via epoch-based reclamation
opcache_reset() and opcache_invalidate() destroy shared memory (hash table memset, interned strings restore, SHM watermark reset) while concurrent reader threads/processes still hold pointers into that memory. This causes zend_mm_heap corrupted crashes under concurrent load in both ZTS (FrankenPHP, parallel) and FPM configurations. A prior fix attempt (PR #14803) wrapped cache lookups in zend_shared_alloc_lock(), which was rejected by Dmitry Stogov because readers must remain lock-free. This patch introduces epoch-based reclamation (EBR), a proven lock-free pattern also used by RCU in the Linux kernel and by Crossbeam in Rust: - Readers publish their epoch on request start (one atomic store) and clear it on request end (one atomic store). No locks acquired. - Writers (opcache_reset path) increment the global epoch and defer the actual hash/interned-string cleanup until all pre-epoch readers have completed. The deferred reset completes at the next request boundary after the drain is complete. - The existing immediate-cleanup fast path is retained for the case when accel_is_inactive() reports no active readers. Additional safety: - When the per-slot pool (256 slots) is exhausted, readers fall back to an aggregate overflow counter that unconditionally blocks deferred reclamation while non-zero. This preserves safety for installations with more concurrent readers than slots. - UNASSIGNED vs OVERFLOW sentinels distinguish "slot not yet attempted" from "allocation failed", avoiding per-request atomic contention on the slot-next counter once slots are exhausted. - A full memory barrier after zend_accel_hash_clean()'s memset ensures readers on weak-memory architectures see the zeroed table before any subsequent insert. - A defensive ZCG(locked) check in accel_try_complete_deferred_reset() prevents the ZEND_ASSERT(!ZCG(locked)) failure that would otherwise fire if the completer is re-entered while the SHM lock is held. Fixes GH-8739 Fixes GH-14471 Fixes GH-18517 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent fbd3017 commit e110a90

File tree

7 files changed

+424
-0
lines changed

7 files changed

+424
-0
lines changed

NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ PHP NEWS
3434
. Fixed bug GH-21593 (Borked function JIT JMPNZ smart branch). (ilutov)
3535
. Fixed bug GH-21460 (COND optimization regression). (Dmitry, Arnaud)
3636
. Fixed faulty returns out of zend_try block in zend_jit_trace(). (ilutov)
37+
. Fixed bug GH-8739 (opcache_reset()/opcache_invalidate() race causes
38+
zend_mm_heap corrupted under concurrent load in ZTS and FPM).
39+
(superdav42)
3740

3841
- OpenSSL:
3942
. Fix a bunch of memory leaks and crashes on edge cases. (ndossche)

ext/opcache/ZendAccelerator.c

Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,8 @@ static zend_result (*orig_post_startup_cb)(void);
136136

137137
static zend_result accel_post_startup(void);
138138
static zend_result accel_finish_startup(void);
139+
static void zend_reset_cache_vars(void);
140+
static void accel_interned_strings_restore_state(void);
139141

140142
static void preload_shutdown(void);
141143
static void preload_activate(void);
@@ -402,6 +404,175 @@ static inline void accel_unlock_all(void)
402404
#endif
403405
}
404406

407+
/* Epoch-based reclamation for safe opcache_reset()/opcache_invalidate()
408+
* under concurrent load (GH-8739). Readers publish their epoch on request
409+
* start and clear it on request end. Writers defer the destructive cleanup
410+
* until every pre-reset reader has finished, so lock-free readers never
411+
* race with hash memset or interned strings restore.
412+
*/
413+
414+
void accel_epoch_init(void)
415+
{
416+
int i;
417+
418+
/* epoch 0 is reserved as the "never deferred" sentinel for drain_epoch */
419+
ZCSG(current_epoch) = 1;
420+
ZCSG(drain_epoch) = 0;
421+
ZCSG(reset_deferred) = false;
422+
ZCSG(epoch_slot_next) = 0;
423+
ZCSG(epoch_overflow_active) = 0;
424+
425+
for (i = 0; i < ACCEL_EPOCH_MAX_SLOTS; i++) {
426+
ZCSG(epoch_slots)[i].epoch = ACCEL_EPOCH_INACTIVE;
427+
}
428+
}
429+
430+
static int32_t accel_epoch_alloc_slot(void)
431+
{
432+
int32_t slot;
433+
434+
#if defined(ZEND_WIN32)
435+
slot = (int32_t)InterlockedIncrement((volatile LONG *)&ZCSG(epoch_slot_next)) - 1;
436+
#elif defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 7))
437+
slot = __atomic_fetch_add(&ZCSG(epoch_slot_next), 1, __ATOMIC_SEQ_CST);
438+
#elif defined(__GNUC__)
439+
slot = __sync_fetch_and_add(&ZCSG(epoch_slot_next), 1);
440+
#else
441+
slot = ZCSG(epoch_slot_next)++;
442+
#endif
443+
444+
if (slot < 0 || slot >= ACCEL_EPOCH_MAX_SLOTS) {
445+
return ACCEL_EPOCH_SLOT_OVERFLOW;
446+
}
447+
return slot;
448+
}
449+
450+
void accel_epoch_enter(void)
451+
{
452+
int32_t slot = ZCG(epoch_slot);
453+
454+
if (UNEXPECTED(slot == ACCEL_EPOCH_SLOT_UNASSIGNED)) {
455+
slot = accel_epoch_alloc_slot();
456+
ZCG(epoch_slot) = slot;
457+
}
458+
459+
if (EXPECTED(slot >= 0)) {
460+
uint64_t epoch = ACCEL_ATOMIC_LOAD_64(&ZCSG(current_epoch));
461+
ZCG(local_epoch) = epoch;
462+
ACCEL_ATOMIC_STORE_64(&ZCSG(epoch_slots)[slot].epoch, epoch);
463+
} else {
464+
/* per-reader slots exhausted - fall back to the aggregate counter
465+
* so deferred reclamation still waits for this reader */
466+
ACCEL_ATOMIC_INC_32(&ZCSG(epoch_overflow_active));
467+
ZCG(local_epoch) = 0;
468+
}
469+
}
470+
471+
void accel_epoch_leave(void)
472+
{
473+
int32_t slot = ZCG(epoch_slot);
474+
475+
if (EXPECTED(slot >= 0)) {
476+
ACCEL_ATOMIC_STORE_64(&ZCSG(epoch_slots)[slot].epoch, ACCEL_EPOCH_INACTIVE);
477+
} else if (slot == ACCEL_EPOCH_SLOT_OVERFLOW) {
478+
ACCEL_ATOMIC_DEC_32(&ZCSG(epoch_overflow_active));
479+
}
480+
}
481+
482+
static uint64_t accel_min_active_epoch(void)
483+
{
484+
uint64_t min_epoch = ACCEL_EPOCH_INACTIVE;
485+
int i;
486+
487+
for (i = 0; i < ACCEL_EPOCH_MAX_SLOTS; i++) {
488+
uint64_t e = ACCEL_ATOMIC_LOAD_64(&ZCSG(epoch_slots)[i].epoch);
489+
if (e < min_epoch) {
490+
min_epoch = e;
491+
}
492+
}
493+
return min_epoch;
494+
}
495+
496+
bool accel_deferred_reset_pending(void)
497+
{
498+
return ZCSG(reset_deferred);
499+
}
500+
501+
void accel_try_complete_deferred_reset(void)
502+
{
503+
uint64_t drain_epoch;
504+
uint64_t min_epoch;
505+
uint32_t overflow;
506+
507+
if (!ZCSG(reset_deferred)) {
508+
return;
509+
}
510+
511+
/* re-entering zend_shared_alloc_lock() would hit ZEND_ASSERT(!ZCG(locked)) */
512+
if (ZCG(locked)) {
513+
return;
514+
}
515+
516+
overflow = ACCEL_ATOMIC_LOAD_32(&ZCSG(epoch_overflow_active));
517+
if (overflow > 0) {
518+
return;
519+
}
520+
521+
drain_epoch = ACCEL_ATOMIC_LOAD_64(&ZCSG(drain_epoch));
522+
min_epoch = accel_min_active_epoch();
523+
524+
if (min_epoch <= drain_epoch) {
525+
return;
526+
}
527+
528+
zend_shared_alloc_lock();
529+
530+
if (ZCSG(reset_deferred)) {
531+
/* re-check under lock in case another thread already completed,
532+
* or a reader published drain_epoch after our lock-free check */
533+
min_epoch = accel_min_active_epoch();
534+
if (min_epoch > ACCEL_ATOMIC_LOAD_64(&ZCSG(drain_epoch))) {
535+
zend_accel_error(ACCEL_LOG_DEBUG,
536+
"Completing deferred opcache reset (drain_epoch=%" PRIu64
537+
", min_active_epoch=%" PRIu64 ")",
538+
drain_epoch, min_epoch);
539+
540+
accel_restart_enter();
541+
542+
zend_map_ptr_reset();
543+
zend_reset_cache_vars();
544+
zend_accel_hash_clean(&ZCSG(hash));
545+
546+
if (ZCG(accel_directives).interned_strings_buffer) {
547+
accel_interned_strings_restore_state();
548+
}
549+
550+
zend_shared_alloc_restore_state();
551+
552+
if (ZCSG(preload_script)) {
553+
preload_restart();
554+
}
555+
556+
#ifdef HAVE_JIT
557+
zend_jit_restart();
558+
#endif
559+
560+
ZCSG(accelerator_enabled) = ZCSG(cache_status_before_restart);
561+
if (ZCSG(last_restart_time) < ZCG(request_time)) {
562+
ZCSG(last_restart_time) = ZCG(request_time);
563+
} else {
564+
ZCSG(last_restart_time)++;
565+
}
566+
567+
ZCSG(reset_deferred) = false;
568+
569+
accel_restart_leave();
570+
}
571+
}
572+
573+
zend_shared_alloc_unlock();
574+
}
575+
405576
/* Interned strings support */
406577

407578
/* O+ disables creation of interned strings by regular PHP compiler, instead,
@@ -2692,6 +2863,14 @@ ZEND_RINIT_FUNCTION(zend_accelerator)
26922863
ZCG(counted) = false;
26932864
}
26942865

2866+
/* publish epoch before any shared memory access so a concurrent
2867+
* opcache_reset() waits for us (GH-8739) */
2868+
accel_epoch_enter();
2869+
2870+
if (UNEXPECTED(ZCSG(reset_deferred))) {
2871+
accel_try_complete_deferred_reset();
2872+
}
2873+
26952874
if (ZCSG(restart_pending)) {
26962875
zend_shared_alloc_lock();
26972876
if (ZCSG(restart_pending)) { /* check again, to ensure that the cache wasn't already cleaned by another process */
@@ -2735,6 +2914,35 @@ ZEND_RINIT_FUNCTION(zend_accelerator)
27352914
ZCSG(last_restart_time)++;
27362915
}
27372916
accel_restart_leave();
2917+
} else if (!ZCSG(reset_deferred)) {
2918+
/* Readers are active — defer the destructive cleanup to
2919+
* accel_try_complete_deferred_reset() at the next request
2920+
* boundary, after all current readers have drained (GH-8739) */
2921+
zend_accel_error(ACCEL_LOG_DEBUG,
2922+
"Deferring opcache restart: active readers detected");
2923+
ZCSG(restart_pending) = false;
2924+
2925+
switch ZCSG(restart_reason) {
2926+
case ACCEL_RESTART_OOM:
2927+
ZCSG(oom_restarts)++;
2928+
break;
2929+
case ACCEL_RESTART_HASH:
2930+
ZCSG(hash_restarts)++;
2931+
break;
2932+
case ACCEL_RESTART_USER:
2933+
ZCSG(manual_restarts)++;
2934+
break;
2935+
}
2936+
2937+
/* new readers publish current_epoch+1, which is > drain_epoch,
2938+
* so they don't block the drain check */
2939+
ACCEL_ATOMIC_STORE_64(&ZCSG(drain_epoch),
2940+
ACCEL_ATOMIC_LOAD_64(&ZCSG(current_epoch)));
2941+
ACCEL_ATOMIC_INC_64(&ZCSG(current_epoch));
2942+
2943+
ZCSG(cache_status_before_restart) = ZCSG(accelerator_enabled);
2944+
ZCSG(accelerator_enabled) = false;
2945+
ZCSG(reset_deferred) = true;
27382946
}
27392947
}
27402948
zend_shared_alloc_unlock();
@@ -2789,6 +2997,17 @@ zend_result accel_post_deactivate(void)
27892997
return SUCCESS;
27902998
}
27912999

3000+
/* leave the epoch before try_complete, so our own slot doesn't block
3001+
* the drain check (GH-8739) */
3002+
if (!file_cache_only && accel_shared_globals) {
3003+
SHM_UNPROTECT();
3004+
accel_epoch_leave();
3005+
if (UNEXPECTED(ZCSG(reset_deferred))) {
3006+
accel_try_complete_deferred_reset();
3007+
}
3008+
SHM_PROTECT();
3009+
}
3010+
27923011
zend_shared_alloc_safe_unlock(); /* be sure we didn't leave cache locked */
27933012
accel_unlock_all();
27943013
ZCG(counted) = false;
@@ -2936,6 +3155,8 @@ static zend_result zend_accel_init_shm(void)
29363155

29373156
zend_reset_cache_vars();
29383157

3158+
accel_epoch_init();
3159+
29393160
ZCSG(oom_restarts) = 0;
29403161
ZCSG(hash_restarts) = 0;
29413162
ZCSG(manual_restarts) = 0;
@@ -2962,6 +3183,7 @@ static void accel_globals_ctor(zend_accel_globals *accel_globals)
29623183
memset(accel_globals, 0, sizeof(zend_accel_globals));
29633184
accel_globals->key = zend_string_alloc(ZCG_KEY_LEN, true);
29643185
GC_MAKE_PERSISTENT_LOCAL(accel_globals->key);
3186+
accel_globals->epoch_slot = ACCEL_EPOCH_SLOT_UNASSIGNED;
29653187
}
29663188

29673189
static void accel_globals_dtor(zend_accel_globals *accel_globals)

ext/opcache/ZendAccelerator.h

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,59 @@
5252
#include "zend_compile.h"
5353
#include "zend_API.h"
5454

55+
#include <stdint.h>
56+
5557
#include "Optimizer/zend_optimizer.h"
5658
#include "zend_accelerator_hash.h"
5759
#include "zend_accelerator_debug.h"
5860

61+
/* Concurrent readers tracked precisely; excess readers fall back to
62+
* ZCSG(epoch_overflow_active) (GH-8739) */
63+
#define ACCEL_EPOCH_MAX_SLOTS 256
64+
65+
#define ACCEL_EPOCH_SLOT_UNASSIGNED (-1) /* slot not yet claimed */
66+
#define ACCEL_EPOCH_SLOT_OVERFLOW (-2) /* claim failed, slots exhausted */
67+
#define ACCEL_EPOCH_INACTIVE UINT64_MAX
68+
69+
/* 64-bit atomics for epoch tracking; zend_atomic.h only covers 32-bit */
70+
#if defined(ZEND_WIN32)
71+
# define ACCEL_ATOMIC_LOAD_64(ptr) ((uint64_t)InterlockedCompareExchange64((volatile LONG64 *)(ptr), 0, 0))
72+
# define ACCEL_ATOMIC_STORE_64(ptr, val) ((void)InterlockedExchange64((volatile LONG64 *)(ptr), (LONG64)(val)))
73+
# define ACCEL_ATOMIC_INC_64(ptr) ((uint64_t)InterlockedIncrement64((volatile LONG64 *)(ptr)))
74+
# define ACCEL_ATOMIC_INC_32(ptr) ((uint32_t)InterlockedIncrement((volatile LONG *)(ptr)))
75+
# define ACCEL_ATOMIC_DEC_32(ptr) ((uint32_t)InterlockedDecrement((volatile LONG *)(ptr)))
76+
# define ACCEL_ATOMIC_LOAD_32(ptr) ((uint32_t)InterlockedCompareExchange((volatile LONG *)(ptr), 0, 0))
77+
#elif defined(__clang__) && defined(__has_feature) && __has_feature(c_atomic)
78+
# define ACCEL_ATOMIC_LOAD_64(ptr) __c11_atomic_load((_Atomic(uint64_t) *)(ptr), __ATOMIC_ACQUIRE)
79+
# define ACCEL_ATOMIC_STORE_64(ptr, val) __c11_atomic_store((_Atomic(uint64_t) *)(ptr), (uint64_t)(val), __ATOMIC_RELEASE)
80+
# define ACCEL_ATOMIC_INC_64(ptr) (__c11_atomic_fetch_add((_Atomic(uint64_t) *)(ptr), 1, __ATOMIC_SEQ_CST) + 1)
81+
# define ACCEL_ATOMIC_INC_32(ptr) (__c11_atomic_fetch_add((_Atomic(uint32_t) *)(ptr), 1, __ATOMIC_SEQ_CST) + 1)
82+
# define ACCEL_ATOMIC_DEC_32(ptr) (__c11_atomic_fetch_sub((_Atomic(uint32_t) *)(ptr), 1, __ATOMIC_SEQ_CST) - 1)
83+
# define ACCEL_ATOMIC_LOAD_32(ptr) __c11_atomic_load((_Atomic(uint32_t) *)(ptr), __ATOMIC_ACQUIRE)
84+
#elif defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 7))
85+
# define ACCEL_ATOMIC_LOAD_64(ptr) __atomic_load_n((volatile uint64_t *)(ptr), __ATOMIC_ACQUIRE)
86+
# define ACCEL_ATOMIC_STORE_64(ptr, val) __atomic_store_n((volatile uint64_t *)(ptr), (uint64_t)(val), __ATOMIC_RELEASE)
87+
# define ACCEL_ATOMIC_INC_64(ptr) __atomic_add_fetch((volatile uint64_t *)(ptr), 1, __ATOMIC_SEQ_CST)
88+
# define ACCEL_ATOMIC_INC_32(ptr) __atomic_add_fetch((volatile uint32_t *)(ptr), 1, __ATOMIC_SEQ_CST)
89+
# define ACCEL_ATOMIC_DEC_32(ptr) __atomic_sub_fetch((volatile uint32_t *)(ptr), 1, __ATOMIC_SEQ_CST)
90+
# define ACCEL_ATOMIC_LOAD_32(ptr) __atomic_load_n((volatile uint32_t *)(ptr), __ATOMIC_ACQUIRE)
91+
#elif defined(__GNUC__)
92+
# define ACCEL_ATOMIC_LOAD_64(ptr) __sync_fetch_and_or((volatile uint64_t *)(ptr), 0)
93+
# define ACCEL_ATOMIC_STORE_64(ptr, val) do { __sync_synchronize(); *(volatile uint64_t *)(ptr) = (uint64_t)(val); __sync_synchronize(); } while (0)
94+
# define ACCEL_ATOMIC_INC_64(ptr) __sync_add_and_fetch((volatile uint64_t *)(ptr), 1)
95+
# define ACCEL_ATOMIC_INC_32(ptr) __sync_add_and_fetch((volatile uint32_t *)(ptr), 1)
96+
# define ACCEL_ATOMIC_DEC_32(ptr) __sync_sub_and_fetch((volatile uint32_t *)(ptr), 1)
97+
# define ACCEL_ATOMIC_LOAD_32(ptr) __sync_fetch_and_or((volatile uint32_t *)(ptr), 0)
98+
#else
99+
/* fallback: volatile only - correct on x86 TSO, not on weak-memory arches */
100+
# define ACCEL_ATOMIC_LOAD_64(ptr) (*(volatile uint64_t *)(ptr))
101+
# define ACCEL_ATOMIC_STORE_64(ptr, val) (*(volatile uint64_t *)(ptr) = (uint64_t)(val))
102+
# define ACCEL_ATOMIC_INC_64(ptr) (++(*(volatile uint64_t *)(ptr)))
103+
# define ACCEL_ATOMIC_INC_32(ptr) (++(*(volatile uint32_t *)(ptr)))
104+
# define ACCEL_ATOMIC_DEC_32(ptr) (--(*(volatile uint32_t *)(ptr)))
105+
# define ACCEL_ATOMIC_LOAD_32(ptr) (*(volatile uint32_t *)(ptr))
106+
#endif
107+
59108
#ifndef PHPAPI
60109
# ifdef ZEND_WIN32
61110
# define PHPAPI __declspec(dllimport)
@@ -117,6 +166,12 @@ typedef struct _zend_early_binding {
117166
uint32_t cache_slot;
118167
} zend_early_binding;
119168

169+
/* per-reader epoch publication slot, padded to a cache line against false sharing */
170+
typedef struct _zend_accel_epoch_slot {
171+
volatile uint64_t epoch;
172+
char padding[56];
173+
} zend_accel_epoch_slot;
174+
120175
typedef struct _zend_persistent_script {
121176
zend_script script;
122177
zend_long compiler_halt_offset; /* position of __HALT_COMPILER or -1 */
@@ -219,6 +274,9 @@ typedef struct _zend_accel_globals {
219274
#endif
220275
void *preloaded_internal_run_time_cache;
221276
size_t preloaded_internal_run_time_cache_size;
277+
/* EBR state: slot index into ZCSG(epoch_slots) or ACCEL_EPOCH_SLOT_* */
278+
int32_t epoch_slot;
279+
uint64_t local_epoch;
222280
/* preallocated shared-memory block to save current script */
223281
void *mem;
224282
zend_persistent_script *current_persistent_script;
@@ -282,6 +340,14 @@ typedef struct _zend_accel_shared_globals {
282340
void *jit_traces;
283341
const void **jit_exit_groups;
284342

343+
/* Epoch-based reclamation for safe opcache_reset()/invalidate() (GH-8739) */
344+
volatile uint64_t current_epoch; /* bumped on each deferred reset */
345+
volatile uint64_t drain_epoch; /* readers publishing <= this block reclamation */
346+
volatile bool reset_deferred;
347+
volatile int32_t epoch_slot_next; /* monotonic slot allocator */
348+
volatile uint32_t epoch_overflow_active; /* readers that didn't get a slot */
349+
zend_accel_epoch_slot epoch_slots[ACCEL_EPOCH_MAX_SLOTS];
350+
285351
/* Interned Strings Support (must be the last element) */
286352
ZEND_SET_ALIGNED(ZEND_STRING_TABLE_POS_ALIGNMENT, zend_string_table interned_strings);
287353
} zend_accel_shared_globals;
@@ -328,6 +394,13 @@ void accelerator_shm_read_unlock(void);
328394
zend_string *accel_make_persistent_key(zend_string *path);
329395
zend_op_array *persistent_compile_file(zend_file_handle *file_handle, int type);
330396

397+
/* Epoch-based reclamation API (GH-8739) */
398+
void accel_epoch_init(void);
399+
void accel_epoch_enter(void);
400+
void accel_epoch_leave(void);
401+
bool accel_deferred_reset_pending(void);
402+
void accel_try_complete_deferred_reset(void);
403+
331404
#define IS_ACCEL_INTERNED(str) \
332405
((char*)(str) >= (char*)ZCSG(interned_strings).start && (char*)(str) < (char*)ZCSG(interned_strings).top)
333406

0 commit comments

Comments
 (0)