Skip to content

Commit 5941cda

Browse files
authored
Fix ZTS crashes with persistent resources in modules (php#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 phpGH-12974.
1 parent be34b96 commit 5941cda

File tree

5 files changed

+38
-17
lines changed

5 files changed

+38
-17
lines changed

TSRM/TSRM.c

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,27 @@ void ts_free_id(ts_rsrc_id id)
576576
TSRM_ERROR((TSRM_ERROR_LEVEL_CORE, "Successfully freed resource id %d", id));
577577
}/*}}}*/
578578

579+
TSRM_API void ts_apply_for_id(ts_rsrc_id id, void (*cb)(void *))
580+
{
581+
int rsrc_id = TSRM_UNSHUFFLE_RSRC_ID(id);
582+
583+
tsrm_mutex_lock(tsmm_mutex);
584+
585+
if (tsrm_tls_table && resource_types_table) {
586+
for (int i = 0; i < tsrm_tls_table_size; i++) {
587+
tsrm_tls_entry *p = tsrm_tls_table[i];
588+
589+
while (p) {
590+
if (p->count > rsrc_id && p->storage[rsrc_id]) {
591+
cb(p->storage[rsrc_id]);
592+
}
593+
p = p->next;
594+
}
595+
}
596+
}
597+
598+
tsrm_mutex_unlock(tsmm_mutex);
599+
}
579600

580601
/*
581602
* Utility Functions

TSRM/TSRM.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,9 @@ TSRM_API void ts_free_thread(void);
104104
/* deallocates all occurrences of a given id */
105105
TSRM_API void ts_free_id(ts_rsrc_id id);
106106

107+
/* Runs a callback on all resources of the given id.
108+
* The caller is responsible for ensuring the underlying resources don't data-race. */
109+
TSRM_API void ts_apply_for_id(ts_rsrc_id id, void (*cb)(void *));
107110

108111
/* Debug support */
109112
#define TSRM_ERROR_LEVEL_ERROR 1

Zend/zend.c

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -826,13 +826,19 @@ static void executor_globals_ctor(zend_executor_globals *executor_globals) /* {{
826826
}
827827
/* }}} */
828828

829-
static void executor_globals_dtor(zend_executor_globals *executor_globals) /* {{{ */
829+
static void executor_globals_persistent_list_dtor(void *storage)
830830
{
831-
zend_ini_dtor(executor_globals->ini_directives);
831+
zend_executor_globals *executor_globals = storage;
832832

833833
if (&executor_globals->persistent_list != global_persistent_list) {
834834
zend_destroy_rsrc_list(&executor_globals->persistent_list);
835835
}
836+
}
837+
838+
static void executor_globals_dtor(zend_executor_globals *executor_globals) /* {{{ */
839+
{
840+
zend_ini_dtor(executor_globals->ini_directives);
841+
836842
if (executor_globals->zend_constants != GLOBAL_CONSTANTS_TABLE) {
837843
zend_hash_destroy(executor_globals->zend_constants);
838844
free(executor_globals->zend_constants);
@@ -1122,6 +1128,9 @@ void zend_shutdown(void) /* {{{ */
11221128
zend_vm_dtor();
11231129

11241130
zend_destroy_rsrc_list(&EG(persistent_list));
1131+
#ifdef ZTS
1132+
ts_apply_for_id(executor_globals_id, executor_globals_persistent_list_dtor);
1133+
#endif
11251134
zend_destroy_modules();
11261135

11271136
virtual_cwd_deactivate();

ext/odbc/php_odbc.c

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,13 +168,7 @@ static void _close_odbc_conn(zend_resource *rsrc)
168168
SQLFreeEnv(conn->henv);
169169
}
170170
efree(conn);
171-
/* See https://github.com/php/php-src/issues/12974 why we need to check the if */
172-
#ifdef ZTS
173-
if (odbc_module_entry.module_started)
174-
#endif
175-
{
176-
ODBCG(num_links)--;
177-
}
171+
ODBCG(num_links)--;
178172
}
179173
/* }}} */
180174

ext/pgsql/pgsql.c

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -315,14 +315,8 @@ static void _close_pgsql_plink(zend_resource *rsrc)
315315
PQclear(res);
316316
}
317317
PQfinish(link);
318-
/* See https://github.com/php/php-src/issues/12974 why we need to check the if */
319-
#ifdef ZTS
320-
if (pgsql_module_entry.module_started)
321-
#endif
322-
{
323-
PGG(num_persistent)--;
324-
PGG(num_links)--;
325-
}
318+
PGG(num_persistent)--;
319+
PGG(num_links)--;
326320
rsrc->ptr = NULL;
327321
}
328322

0 commit comments

Comments
 (0)