From c90060340b7481afca19619da55afecc44f321a4 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 15 Mar 2023 21:18:37 +0100 Subject: [PATCH 1/4] Fix GH-10737: PHP 8.1.16 segfaults on line 597 of sapi/apache2handler/sapi_apache2.c The TSRM keeps a hashtable mapping the thread IDs to the thread resource pointers. It's possible that the thread disappears without us knowing, and then another thread gets spawned some time later with the same ID as the disappeared thread. Note that since it's a new thread the TSRM key pointer and cached pointer will be NULL. The Apache request handler `php_handler()` will try to fetch some fields from the SAPI globals. It uses a lazy thread resource allocation by calling `ts_resource(0);`. This allocates a thread resource and sets up the TSRM pointers if they haven't been set up yet. At least, that's what's supposed to happen. But since we are in a situation where the thread ID still has the resources of the *old* thread associated in the hashtable, the loop in `ts_resource_ex` will find that thread resource and assume the thread has been setup already. But this is not the case since this thread is actually a new thread, just reusing the ID of the old one, without any relation whatsoever to the old thread. Because of this assumption, the TSRM pointers will not be setup, leading to a NULL pointer dereference when trying to access the SAPI globals. We can easily detect this scenario: if we're in the fallback path, and the pointer is NULL, and we're looking for our own thread resource, we know we're actually reusing a thread ID. In that case, we'll free up the old thread resources gracefully (gracefully because there might still be resources open like database connection which need to be shut down cleanly). After freeing the resources, we'll create the new resources for this thread as if the stale resources never existed in the first place. From that point forward, it is as if that situation never occurred. The fact that this situation happens isn't that bad because a child process containing threads will eventually be respawned anyway by the SAPI, so the stale thread resources won't remain forever. Note that we can't simply assign our own TSRM pointers to the existing thread resource for our ID, since it was actually from a different thread (just with the same ID!). Furthermore, the dynamically loaded extensions have their own pointer, which is only set when their constructor is called, so we'd have to call their constructor anyway... I also tried to call the dtor and then the ctor again for those resources on the pre-existing thread resource to reuse storage, but that didn't work properly because other code doesn't expect something like that to happen, which breaks assumptions, and this in turn caused Valgrind to (rightfully) complain about memory bugs. Note 2: I also had to fix a bug in the core globals destruction because it always assumed that the thread destroying them was the owning thread, which on TSRM shutdown isn't always the case. A similar bug was fixed recently with the JIT globals. --- TSRM/TSRM.c | 75 +++++++++++++++++++++++++++++++++++++----------- main/main.c | 2 +- main/php_ticks.c | 4 +-- main/php_ticks.h | 2 +- 4 files changed, 62 insertions(+), 21 deletions(-) diff --git a/TSRM/TSRM.c b/TSRM/TSRM.c index 7cd924318ed4..450ca853ea42 100644 --- a/TSRM/TSRM.c +++ b/TSRM/TSRM.c @@ -367,7 +367,13 @@ TSRM_API ts_rsrc_id ts_allocate_fast_id(ts_rsrc_id *rsrc_id, size_t *offset, siz return *rsrc_id; }/*}}}*/ +static void set_thread_local_storage_resource_to(tsrm_tls_entry *thread_resource) +{ + tsrm_tls_set(thread_resource); + TSRMLS_CACHE = thread_resource; +} +/* Must be called with tsmm_mutex held */ static void allocate_new_resource(tsrm_tls_entry **thread_resources_ptr, THREAD_T thread_id) {/*{{{*/ int i; @@ -383,8 +389,7 @@ static void allocate_new_resource(tsrm_tls_entry **thread_resources_ptr, THREAD_ (*thread_resources_ptr)->next = NULL; /* Set thread local storage to this new thread resources structure */ - tsrm_tls_set(*thread_resources_ptr); - TSRMLS_CACHE = *thread_resources_ptr; + set_thread_local_storage_resource_to(*thread_resources_ptr); if (tsrm_new_thread_begin_handler) { tsrm_new_thread_begin_handler(thread_id); @@ -407,17 +412,27 @@ static void allocate_new_resource(tsrm_tls_entry **thread_resources_ptr, THREAD_ if (tsrm_new_thread_end_handler) { tsrm_new_thread_end_handler(thread_id); } - - tsrm_mutex_unlock(tsmm_mutex); }/*}}}*/ +static void ts_free_resources(tsrm_tls_entry *thread_resources) +{ + for (int i = 0; i < thread_resources->count; i++) { + if (resource_types_table[i].dtor) { + resource_types_table[i].dtor(thread_resources->storage[i]); + } + if (!resource_types_table[i].fast_offset) { + free(thread_resources->storage[i]); + } + } + free(thread_resources->storage); +} /* fetches the requested resource for the current thread */ TSRM_API void *ts_resource_ex(ts_rsrc_id id, THREAD_T *th_id) {/*{{{*/ THREAD_T thread_id; int hash_value; - tsrm_tls_entry *thread_resources; + tsrm_tls_entry *thread_resources, **last_thread_resources; if (!th_id) { /* Fast path for looking up the resources for the current @@ -448,16 +463,20 @@ TSRM_API void *ts_resource_ex(ts_rsrc_id id, THREAD_T *th_id) if (!thread_resources) { allocate_new_resource(&tsrm_tls_table[hash_value], thread_id); + tsrm_mutex_unlock(tsmm_mutex); return ts_resource_ex(id, &thread_id); } else { + last_thread_resources = &tsrm_tls_table[hash_value]; do { if (thread_resources->thread_id == thread_id) { break; } + last_thread_resources = &thread_resources->next; if (thread_resources->next) { thread_resources = thread_resources->next; } else { allocate_new_resource(&thread_resources->next, thread_id); + tsrm_mutex_unlock(tsmm_mutex); return ts_resource_ex(id, &thread_id); /* * thread_resources = thread_resources->next; @@ -466,7 +485,40 @@ TSRM_API void *ts_resource_ex(ts_rsrc_id id, THREAD_T *th_id) } } while (thread_resources); } + + /* It's possible that the current thread resources are requested, and that we get here. + * This means that the TSRM key pointer and cached pointer are NULL, but there is still + * a thread resource associated with this ID in the hashtable. This can occur if a thread + * goes away, but its resources are never cleaned up, and then that thread ID is reused. + * Since we don't always have a way to know when a thread goes away, we can't clean up + * the thread's resources before the new thread spawns. + * To solve this issue, we'll free up the old thread resources gracefully (gracefully + * because there might still be resources open like database connection which need to + * be shut down cleanly). After freeing up, we'll create the new resources for this thread + * as if the stale resources never existed in the first place. From that point forward, + * it is as if that situation never occurred. + * The fact that this situation happens isn't that bad because a child process containing + * threads will eventually be respawned anyway by the SAPI, so the stale threads won't last + * forever. */ + TSRM_ASSERT(thread_resources->thread_id == thread_id); + if (thread_id == tsrm_thread_id() && !tsrm_tls_get()) { + tsrm_tls_entry *next = thread_resources->next; + /* In case that extensions don't use the pointer passed from the dtor, but incorrectly + * use the global pointer, we need to setup the global pointer temporarily here. */ + set_thread_local_storage_resource_to(thread_resources); + /* Free up the old resource from the old thread instance */ + ts_free_resources(thread_resources); + free(thread_resources); + /* Allocate a new resource at the same point in the linked list, and relink the next pointer */ + allocate_new_resource(last_thread_resources, thread_id); + thread_resources = *last_thread_resources; + thread_resources->next = next; + /* We don't have to tail-call ts_resource_ex, we can take the fast path to the return + * because we already have the correct pointer. */ + } + tsrm_mutex_unlock(tsmm_mutex); + /* Read a specific resource from the thread's resources. * This is called outside of a mutex, so have to be aware about external * changes to the structure as we read it. @@ -479,7 +531,6 @@ TSRM_API void *ts_resource_ex(ts_rsrc_id id, THREAD_T *th_id) void ts_free_thread(void) {/*{{{*/ tsrm_tls_entry *thread_resources; - int i; THREAD_T thread_id = tsrm_thread_id(); int hash_value; tsrm_tls_entry *last=NULL; @@ -492,17 +543,7 @@ void ts_free_thread(void) while (thread_resources) { if (thread_resources->thread_id == thread_id) { - for (i=0; icount; i++) { - if (resource_types_table[i].dtor) { - resource_types_table[i].dtor(thread_resources->storage[i]); - } - } - for (i=0; icount; i++) { - if (!resource_types_table[i].fast_offset) { - free(thread_resources->storage[i]); - } - } - free(thread_resources->storage); + ts_free_resources(thread_resources); if (last) { last->next = thread_resources->next; } else { diff --git a/main/main.c b/main/main.c index 3d7ca2913838..4868d2df4003 100644 --- a/main/main.c +++ b/main/main.c @@ -1936,7 +1936,7 @@ static void core_globals_dtor(php_core_globals *core_globals) free(core_globals->php_binary); } - php_shutdown_ticks(); + php_shutdown_ticks(core_globals); } /* }}} */ diff --git a/main/php_ticks.c b/main/php_ticks.c index 004314583bdb..70201ddecd08 100644 --- a/main/php_ticks.c +++ b/main/php_ticks.c @@ -34,9 +34,9 @@ void php_deactivate_ticks(void) zend_llist_clean(&PG(tick_functions)); } -void php_shutdown_ticks(void) +void php_shutdown_ticks(php_core_globals *core_globals) { - zend_llist_destroy(&PG(tick_functions)); + zend_llist_destroy(&core_globals->tick_functions); } static int php_compare_tick_functions(void *elem1, void *elem2) diff --git a/main/php_ticks.h b/main/php_ticks.h index 5edf7a483bba..270ea5348fd2 100644 --- a/main/php_ticks.h +++ b/main/php_ticks.h @@ -19,7 +19,7 @@ int php_startup_ticks(void); void php_deactivate_ticks(void); -void php_shutdown_ticks(void); +void php_shutdown_ticks(php_core_globals *core_globals); void php_run_ticks(int count); BEGIN_EXTERN_C() From d52bb54fc149ac6740d6641e70abf326f8e4a47c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 17 Mar 2023 08:46:40 +0100 Subject: [PATCH 2/4] Run dtors and frees in reverse order to respect dependencies --- TSRM/TSRM.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/TSRM/TSRM.c b/TSRM/TSRM.c index 450ca853ea42..92c4f6934446 100644 --- a/TSRM/TSRM.c +++ b/TSRM/TSRM.c @@ -416,10 +416,13 @@ static void allocate_new_resource(tsrm_tls_entry **thread_resources_ptr, THREAD_ static void ts_free_resources(tsrm_tls_entry *thread_resources) { - for (int i = 0; i < thread_resources->count; i++) { + /* Need to destroy in reverse order to respect dependencies. */ + for (int i = thread_resources->count - 1; i >= 0; i--) { if (resource_types_table[i].dtor) { resource_types_table[i].dtor(thread_resources->storage[i]); } + } + for (int i = thread_resources->count - 1; i >= 0; i--) { if (!resource_types_table[i].fast_offset) { free(thread_resources->storage[i]); } From 3a230c2f877ba368d0000e3516ae038008c49c89 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 19 Mar 2023 02:39:13 +0100 Subject: [PATCH 3/4] Unify TSRM freeing routine and make sure it's always in reverse order --- TSRM/TSRM.c | 55 +++++++++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/TSRM/TSRM.c b/TSRM/TSRM.c index 92c4f6934446..369a4c1ee37d 100644 --- a/TSRM/TSRM.c +++ b/TSRM/TSRM.c @@ -161,6 +161,23 @@ TSRM_API int tsrm_startup(int expected_threads, int expected_resources, int debu return 1; }/*}}}*/ +static void ts_free_resources(tsrm_tls_entry *thread_resources) +{ + /* Need to destroy in reverse order to respect dependencies. */ + for (int i = thread_resources->count - 1; i >= 0; i--) { + if (!resource_types_table[i].done) { + if (resource_types_table[i].dtor) { + resource_types_table[i].dtor(thread_resources->storage[i]); + } + + if (!resource_types_table[i].fast_offset) { + free(thread_resources->storage[i]); + } + } + } + + free(thread_resources->storage); +} /* Shutdown TSRM (call once for the entire process) */ TSRM_API void tsrm_shutdown(void) @@ -183,25 +200,13 @@ TSRM_API void tsrm_shutdown(void) tsrm_tls_entry *p = tsrm_tls_table[i], *next_p; while (p) { - int j; - next_p = p->next; - for (j=0; jcount; j++) { - if (p->storage[j]) { - if (resource_types_table) { - if (!resource_types_table[j].done) { - if (resource_types_table[j].dtor) { - resource_types_table[j].dtor(p->storage[j]); - } - - if (!resource_types_table[j].fast_offset) { - free(p->storage[j]); - } - } - } - } + if (resource_types_table) { + /* This call will already free p->storage for us */ + ts_free_resources(p); + } else { + free(p->storage); } - free(p->storage); free(p); p = next_p; } @@ -414,22 +419,6 @@ static void allocate_new_resource(tsrm_tls_entry **thread_resources_ptr, THREAD_ } }/*}}}*/ -static void ts_free_resources(tsrm_tls_entry *thread_resources) -{ - /* Need to destroy in reverse order to respect dependencies. */ - for (int i = thread_resources->count - 1; i >= 0; i--) { - if (resource_types_table[i].dtor) { - resource_types_table[i].dtor(thread_resources->storage[i]); - } - } - for (int i = thread_resources->count - 1; i >= 0; i--) { - if (!resource_types_table[i].fast_offset) { - free(thread_resources->storage[i]); - } - } - free(thread_resources->storage); -} - /* fetches the requested resource for the current thread */ TSRM_API void *ts_resource_ex(ts_rsrc_id id, THREAD_T *th_id) {/*{{{*/ From b7726e882102fd30ac7f3774d8330e87b67c39f0 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 19 Mar 2023 13:30:27 +0100 Subject: [PATCH 4/4] Change misleading do-while loop condition No semantic changes. The condition is misleading because thread_resources can never be NULL. What we actually want to do is check whether we found the resources corresponding to the current thread ID. Suggested-by: ylavic --- TSRM/TSRM.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/TSRM/TSRM.c b/TSRM/TSRM.c index 369a4c1ee37d..62be0a421481 100644 --- a/TSRM/TSRM.c +++ b/TSRM/TSRM.c @@ -459,10 +459,7 @@ TSRM_API void *ts_resource_ex(ts_rsrc_id id, THREAD_T *th_id) return ts_resource_ex(id, &thread_id); } else { last_thread_resources = &tsrm_tls_table[hash_value]; - do { - if (thread_resources->thread_id == thread_id) { - break; - } + while (thread_resources->thread_id != thread_id) { last_thread_resources = &thread_resources->next; if (thread_resources->next) { thread_resources = thread_resources->next; @@ -470,12 +467,8 @@ TSRM_API void *ts_resource_ex(ts_rsrc_id id, THREAD_T *th_id) allocate_new_resource(&thread_resources->next, thread_id); tsrm_mutex_unlock(tsmm_mutex); return ts_resource_ex(id, &thread_id); - /* - * thread_resources = thread_resources->next; - * break; - */ } - } while (thread_resources); + } } /* It's possible that the current thread resources are requested, and that we get here.