Fix GH-8739: OPcache restart corrupts SHM in threaded SAPIs (ZTS)

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)
 

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) {

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);

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;

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;