From 5941cdaaad4af357f4deb5e3ab1325b87cff7cd6 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Tue, 13 Feb 2024 21:43:03 +0100
Subject: [PATCH] Fix ZTS crashes with persistent resources in modules (#13381)

On shutdown in ZTS the following happens:
- https://github.com/php/php-src/blob/master/Zend/zend.c#L1124-L1125
  gets executed. This destroys global persistent resources and destroys
  the modules. Furthermore, the modules are unloaded too.
- Further down, `ts_free_id(executor_globals_id)` gets executed, which
  calls `executor_globals_dtor`. This function destroys persistent
  resources for each thread.

Notice that in the last step, the modules that the persistent resource
belong to may already have been destroyed. This means that accessing
globals will cause a crash (I previously fixed this with ifdef magic),
or when the module is dynamically loaded we'll try jumping to a
destructor that is no longer loaded in memory. These scenarios cause
crashes.

It's not possible to move the `ts_free_id` call upwards, because that
may break assumptions of callers, and furthermore this would deallocate
the executor globals structure, which means that any access to those
will cause a segfault.

This patch adds a new API to the TSRM that allows running a callback on
a certain resource type. We use this API to destroy the persistent
resources in all threads prior to the module destruction, and keep the
rest of the resource dtor intact.

I verified this fix on Apache with postgres, both dynamically and
statically.

Fixes GH-12974.
---
 TSRM/TSRM.c         | 21 +++++++++++++++++++++
 TSRM/TSRM.h         |  3 +++
 Zend/zend.c         | 13 +++++++++++--
 ext/odbc/php_odbc.c |  8 +-------
 ext/pgsql/pgsql.c   | 10 ++--------
 5 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/TSRM/TSRM.c b/TSRM/TSRM.c
index 09ff8cc4927..5d1ae304098 100644
--- a/TSRM/TSRM.c
+++ b/TSRM/TSRM.c
@@ -576,6 +576,27 @@ void ts_free_id(ts_rsrc_id id)
 	TSRM_ERROR((TSRM_ERROR_LEVEL_CORE, "Successfully freed resource id %d", id));
 }/*}}}*/
 
+TSRM_API void ts_apply_for_id(ts_rsrc_id id, void (*cb)(void *))
+{
+	int rsrc_id = TSRM_UNSHUFFLE_RSRC_ID(id);
+
+	tsrm_mutex_lock(tsmm_mutex);
+
+	if (tsrm_tls_table && resource_types_table) {
+		for (int i = 0; i < tsrm_tls_table_size; i++) {
+			tsrm_tls_entry *p = tsrm_tls_table[i];
+
+			while (p) {
+				if (p->count > rsrc_id && p->storage[rsrc_id]) {
+					cb(p->storage[rsrc_id]);
+				}
+				p = p->next;
+			}
+		}
+	}
+
+	tsrm_mutex_unlock(tsmm_mutex);
+}
 
 /*
  * Utility Functions
diff --git a/TSRM/TSRM.h b/TSRM/TSRM.h
index 3bb32e4da28..c9e8edd3722 100644
--- a/TSRM/TSRM.h
+++ b/TSRM/TSRM.h
@@ -104,6 +104,9 @@ TSRM_API void ts_free_thread(void);
 /* deallocates all occurrences of a given id */
 TSRM_API void ts_free_id(ts_rsrc_id id);
 
+/* Runs a callback on all resources of the given id.
+ * The caller is responsible for ensuring the underlying resources don't data-race. */
+TSRM_API void ts_apply_for_id(ts_rsrc_id id, void (*cb)(void *));
 
 /* Debug support */
 #define TSRM_ERROR_LEVEL_ERROR	1
diff --git a/Zend/zend.c b/Zend/zend.c
index 12efe56a63b..6e7f4890f7e 100644
--- a/Zend/zend.c
+++ b/Zend/zend.c
@@ -826,13 +826,19 @@ static void executor_globals_ctor(zend_executor_globals *executor_globals) /* {{
 }
 /* }}} */
 
-static void executor_globals_dtor(zend_executor_globals *executor_globals) /* {{{ */
+static void executor_globals_persistent_list_dtor(void *storage)
 {
-	zend_ini_dtor(executor_globals->ini_directives);
+	zend_executor_globals *executor_globals = storage;
 
 	if (&executor_globals->persistent_list != global_persistent_list) {
 		zend_destroy_rsrc_list(&executor_globals->persistent_list);
 	}
+}
+
+static void executor_globals_dtor(zend_executor_globals *executor_globals) /* {{{ */
+{
+	zend_ini_dtor(executor_globals->ini_directives);
+
 	if (executor_globals->zend_constants != GLOBAL_CONSTANTS_TABLE) {
 		zend_hash_destroy(executor_globals->zend_constants);
 		free(executor_globals->zend_constants);
@@ -1122,6 +1128,9 @@ void zend_shutdown(void) /* {{{ */
 	zend_vm_dtor();
 
 	zend_destroy_rsrc_list(&EG(persistent_list));
+#ifdef ZTS
+	ts_apply_for_id(executor_globals_id, executor_globals_persistent_list_dtor);
+#endif
 	zend_destroy_modules();
 
 	virtual_cwd_deactivate();
diff --git a/ext/odbc/php_odbc.c b/ext/odbc/php_odbc.c
index cd3877edc6b..1866a8f14d1 100644
--- a/ext/odbc/php_odbc.c
+++ b/ext/odbc/php_odbc.c
@@ -168,13 +168,7 @@ static void _close_odbc_conn(zend_resource *rsrc)
 		SQLFreeEnv(conn->henv);
 	}
 	efree(conn);
-	/* See https://github.com/php/php-src/issues/12974 why we need to check the if */
-#ifdef ZTS
-	if (odbc_module_entry.module_started)
-#endif
-	{
-		ODBCG(num_links)--;
-	}
+	ODBCG(num_links)--;
 }
 /* }}} */
 
diff --git a/ext/pgsql/pgsql.c b/ext/pgsql/pgsql.c
index 2356d8857c8..860d007cfc0 100644
--- a/ext/pgsql/pgsql.c
+++ b/ext/pgsql/pgsql.c
@@ -315,14 +315,8 @@ static void _close_pgsql_plink(zend_resource *rsrc)
 		PQclear(res);
 	}
 	PQfinish(link);
-	/* See https://github.com/php/php-src/issues/12974 why we need to check the if */
-#ifdef ZTS
-	if (pgsql_module_entry.module_started)
-#endif
-	{
-		PGG(num_persistent)--;
-		PGG(num_links)--;
-	}
+	PGG(num_persistent)--;
+	PGG(num_links)--;
 	rsrc->ptr = NULL;
 }