From 570f19e92f778616154e893af9851c2274426dc3 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 12 Feb 2024 20:00:12 +0100 Subject: [PATCH 1/2] Fix ZTS crashes with persistent resources in modules 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 09ff8cc49279e..23e1baf1479d4 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_callback_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 3bb32e4da289a..8318214be3651 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_callback_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 12efe56a63b05..f9b619552104e 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_callback_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 cd3877edc6bfe..1866a8f14d12d 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 2356d8857c8dd..860d007cfc01d 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; } From f3a4479381d100689f6693a802e70c4f5c4c323c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 13 Feb 2024 21:29:34 +0100 Subject: [PATCH 2/2] Rename ts_callback_id to ts_apply_for_id --- TSRM/TSRM.c | 2 +- TSRM/TSRM.h | 2 +- Zend/zend.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/TSRM/TSRM.c b/TSRM/TSRM.c index 23e1baf1479d4..5d1ae304098da 100644 --- a/TSRM/TSRM.c +++ b/TSRM/TSRM.c @@ -576,7 +576,7 @@ void ts_free_id(ts_rsrc_id id) TSRM_ERROR((TSRM_ERROR_LEVEL_CORE, "Successfully freed resource id %d", id)); }/*}}}*/ -TSRM_API void ts_callback_id(ts_rsrc_id id, void (*cb)(void *)) +TSRM_API void ts_apply_for_id(ts_rsrc_id id, void (*cb)(void *)) { int rsrc_id = TSRM_UNSHUFFLE_RSRC_ID(id); diff --git a/TSRM/TSRM.h b/TSRM/TSRM.h index 8318214be3651..c9e8edd37224b 100644 --- a/TSRM/TSRM.h +++ b/TSRM/TSRM.h @@ -106,7 +106,7 @@ 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_callback_id(ts_rsrc_id id, void (*cb)(void *)); +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 f9b619552104e..6e7f4890f7eec 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -1129,7 +1129,7 @@ void zend_shutdown(void) /* {{{ */ zend_destroy_rsrc_list(&EG(persistent_list)); #ifdef ZTS - ts_callback_id(executor_globals_id, executor_globals_persistent_list_dtor); + ts_apply_for_id(executor_globals_id, executor_globals_persistent_list_dtor); #endif zend_destroy_modules();