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

ext/opcache/ZendAccelerator.c

@@ -333,13 +333,73 @@ 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 cmd, 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, cmd, &lock) != -1;
+}
+
+/* Adds (+1, only from RINIT) or releases (-1) a reference of the calling thread.
+ * Does not use ZCG() as it is also called from accel_globals_dtor(). */
+static bool accel_zts_readers_add(int delta)
+{
+	int count;
+
+#ifndef HAVE_NO_ATOMICS
+	count = zend_atomic_int_load_ex(&accel_zts_readers);
+	while (count > 0 && count + delta > 0) {
+		if (zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count + delta)) {
+			return true;
+		}
+	}
+#endif
+
+	tsrm_mutex_lock(accel_zts_readers_mutex);
+	count = zend_atomic_int_load_ex(&accel_zts_readers);
+	/* waits while another process restarts the cache (it holds the write lock) */
+	if (count == 0 && !accel_zts_lock_reader_byte(F_SETLKW, 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 + delta));
+	if (count + delta == 0) {
+		/* the last reader of this process; unlock before another thread may relock */
+		accel_zts_lock_reader_byte(F_SETLK, F_UNLCK);
+	}
+	tsrm_mutex_unlock(accel_zts_readers_mutex);
+	return true;
+}
+#endif
+
 /* Creates a read lock for SHM access */
 static inline zend_result accel_activate_add(void)
 {
 #ifdef ZEND_WIN32
 	SHM_UNPROTECT();
 	INCREMENT(mem_usage);
 	SHM_PROTECT();
+#elif defined(ZTS)
+	/* A thread becomes a reader only in RINIT, see accel_zts_readers_add(). A request
+	 * that was not admitted there must not start to use SHM later. */
+	return FAILURE;
 #else
 	struct flock mem_usage_lock;
 
@@ -366,6 +426,12 @@ static inline void accel_deactivate_sub(void)
 		ZCG(counted) = false;
 		SHM_PROTECT();
 	}
+#elif defined(ZTS)
+	/* releases the reference taken in RINIT (accel_activate_add() never succeeds under ZTS) */
+	if (ZCG(counted)) {
+		accel_zts_readers_add(-1);
+		ZCG(counted) = false;
+	}
 #else
 	struct flock mem_usage_unlock;
 
@@ -382,7 +448,9 @@ static inline void accel_deactivate_sub(void)
 
 static inline void accel_unlock_all(void)
 {
-#ifdef ZEND_WIN32
+#if defined(ZEND_WIN32) || defined(ZTS)
+	/* Under ZTS 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. */
 	accel_deactivate_sub();
 #else
 	if (lock_file == -1) {
@@ -934,6 +1002,48 @@ 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;
+	time_t now;
+
+	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_SETLK, F_WRLCK)) {
+		return true;
+	}
+	tsrm_mutex_unlock(accel_zts_readers_mutex);
+
+	now = time(NULL);
+	if (readers == 0) {
+		/* readers in other processes: as without ZTS, force_restart_timeout kills them,
+		 * here with all their threads */
+		(void) accel_is_inactive();
+	} else if (ZCG(accel_directives).force_restart_timeout && ZCSG(force_restart_time)
+			&& now >= ZCSG(force_restart_time) && now >= accel_zts_next_warning) {
+		/* Threads cannot be killed and this process must not kill itself: the restart
+		 * waits for the running requests, new ones run uncached meanwhile */
+		zend_accel_error(ACCEL_LOG_WARNING, "Restart delayed for %ld seconds by %d threads of process %d still using the cache",
+			(long)(now - ZCSG(force_restart_time) + ZCG(accel_directives).force_restart_timeout), readers, getpid());
+		accel_zts_next_warning = now + ZCG(accel_directives).force_restart_timeout;
+	}
+	return false;
+}
+
+static void accel_zts_restart_end(void)
+{
+	accel_zts_lock_reader_byte(F_SETLK, 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;
@@ -2700,7 +2810,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) {
@@ -2740,13 +2854,29 @@ 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();
 	}
 
 	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_readers_add(1)) {
+			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();
 
@@ -2971,6 +3101,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_readers_add(-1);
+	}
+#endif
 	zend_string_free(accel_globals->key);
 	if (accel_globals->preloaded_internal_run_time_cache) {
 		pefree(accel_globals->preloaded_internal_run_time_cache, 1);
@@ -3296,6 +3436,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);
 
@@ -3400,6 +3547,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)

ext/opcache/tests/gh8739.phpt

@@ -0,0 +1,56 @@
+--TEST--
+GH-8739: OPcache restart in a server with several workers
+--DESCRIPTION--
+A scheduled restart happens in the next request once no request uses the cache, and the cache
+works after it. Under ZTS this checks the per-process reader count and the reader lock shared by
+the worker processes: a reference that is never released would delay the restart forever.
+--EXTENSIONS--
+opcache
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+--CONFLICTS--
+server
+--FILE--
+<?php
+$dir = __DIR__ . '/gh8739';
+@mkdir($dir);
+file_put_contents("$dir/status.php", <<<'PHP'
+<?php
+$status = opcache_get_status(false);
+echo $status['opcache_enabled'] ? 'enabled' : 'disabled',
+    ', restarts: ', $status['opcache_statistics']['manual_restarts'],
+    ', ', opcache_is_script_cached(__FILE__) ? 'cached' : 'not cached', "\n";
+PHP);
+file_put_contents("$dir/reset.php", "<?php\nvar_dump(opcache_reset());\n");
+
+include __DIR__ . '/php_cli_server.inc';
+
+/* elsewhere the workers outlive the server without PDEATHSIG, see php_cli_server_pdeathsig.phpt */
+if (PHP_OS === 'Linux' || PHP_OS === 'FreeBSD') {
+    putenv('PHP_CLI_SERVER_WORKERS=2');
+}
+$ini = '-d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.file_update_protection=0';
+$ext = ini_get('extension_dir');
+if (file_exists($ext . DIRECTORY_SEPARATOR . (PHP_OS_FAMILY === 'Windows' ? 'php_opcache.dll' : 'opcache.so'))) {
+    $ini .= ' -d extension_dir=' . $ext . ' -d zend_extension=opcache';
+}
+php_cli_server_start($ini);
+
+$base = 'http://' . PHP_CLI_SERVER_ADDRESS . '/gh8739/';
+foreach (['status', 'reset', 'status', 'status'] as $script) {
+    echo @file_get_contents($base . $script . '.php') ?: "no response\n";
+}
+?>
+--CLEAN--
+<?php
+$dir = __DIR__ . '/gh8739';
+@unlink("$dir/status.php");
+@unlink("$dir/reset.php");
+@rmdir($dir);
+?>
+--EXPECT--
+enabled, restarts: 0, cached
+bool(true)
+enabled, restarts: 1, cached
+enabled, restarts: 1, cached