From 6a937eb9ceb734e300858eefa8f7f084075625f2 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 21 Mar 2024 23:51:44 +0100 Subject: [PATCH 1/6] Remove broken hack in mysqlnd_vio::close_stream What happens is that the persistent network stream resource gets freed, yet stays inside EG(persistent_list). This causes a crash on shutdown when the persistent list is getting cleared, as the engine will try to free the network stream again. The code in close_stream gets confused between persistent vs non-persistent allocations when EG(active) is false. This code was introduced in c3019a1 to fix crashes when the persistent list gets destroyed before module shutdown is called. This is indeed a potential problem that was fixed on the master branch in 5941cda. This fixes the crash reason of GH-10599. --- ext/mysqlnd/mysqlnd_vio.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c index 312bf7e2782ff..c12317c436605 100644 --- a/ext/mysqlnd/mysqlnd_vio.c +++ b/ext/mysqlnd/mysqlnd_vio.c @@ -652,15 +652,11 @@ MYSQLND_METHOD(mysqlnd_vio, close_stream)(MYSQLND_VIO * const net, MYSQLND_STATS bool pers = net->persistent; DBG_INF_FMT("Freeing stream. abstract=%p", net_stream->abstract); /* We removed the resource from the stream, so pass FREE_RSRC_DTOR now to force - * destruction to occur during shutdown, because it won't happen through the resource. */ - /* TODO: The EG(active) check here is dead -- check IN_SHUTDOWN? */ - if (pers && EG(active)) { + * destruction to occur during shutdown, because it won't happen through the resource + * because we removed the resource from the EG resource list(s). */ + if (pers) { php_stream_free(net_stream, PHP_STREAM_FREE_CLOSE_PERSISTENT | PHP_STREAM_FREE_RSRC_DTOR); } else { - /* - otherwise we will crash because the EG(persistent_list) has been freed already, - before the modules are shut down - */ php_stream_free(net_stream, PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_RSRC_DTOR); } net->data->m.set_stream(net, NULL); From e98c7eecbd94544ef70972bd1688c90ef8fc89c9 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 21 Mar 2024 23:54:11 +0100 Subject: [PATCH 2/6] Fix memory leak in mysqlnd_vio::open_pipe This fixes the memory leak part of GH-10599. --- ext/mysqlnd/mysqlnd_vio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c index c12317c436605..7d55418e7cd72 100644 --- a/ext/mysqlnd/mysqlnd_vio.c +++ b/ext/mysqlnd/mysqlnd_vio.c @@ -141,6 +141,7 @@ MYSQLND_METHOD(mysqlnd_vio, open_pipe)(MYSQLND_VIO * const vio, const MYSQLND_CS EG(regular_list).pDestructor = NULL; zend_hash_index_del(&EG(regular_list), net_stream->res->handle); /* ToDO: should it be res->handle, do streams register with addref ?*/ EG(regular_list).pDestructor = origin_dtor; + efree(net_stream->res); net_stream->res = NULL; DBG_RETURN(net_stream); From c04565aadfe6e8d67eaf16d7391c0e9b6c8becf0 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 21 Mar 2024 23:56:10 +0100 Subject: [PATCH 3/6] Factor common code out into mysqlnd_fixup_regular_list() This prevents the code from getting desynced again, which was the reason for the leak of GH-10599. --- ext/mysqlnd/mysqlnd_vio.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c index 7d55418e7cd72..4849633cca661 100644 --- a/ext/mysqlnd/mysqlnd_vio.c +++ b/ext/mysqlnd/mysqlnd_vio.c @@ -112,6 +112,20 @@ MYSQLND_METHOD(mysqlnd_vio, network_write)(MYSQLND_VIO * const vio, const zend_u } /* }}} */ +static void mysqlnd_fixup_regular_list(php_stream *net_stream) +{ + /* + Streams are not meant for C extensions! Thus we need a hack. Every connected stream will + be registered as resource (in EG(regular_list). So far, so good. However, it won't be + unregistered until the script ends. So, we need to take care of that. + */ + dtor_func_t origin_dtor = EG(regular_list).pDestructor; + EG(regular_list).pDestructor = NULL; + zend_hash_index_del(&EG(regular_list), net_stream->res->handle); + EG(regular_list).pDestructor = origin_dtor; + efree(net_stream->res); + net_stream->res = NULL; +} /* {{{ mysqlnd_vio::open_pipe */ static php_stream * @@ -119,7 +133,6 @@ MYSQLND_METHOD(mysqlnd_vio, open_pipe)(MYSQLND_VIO * const vio, const MYSQLND_CS MYSQLND_STATS * const conn_stats, MYSQLND_ERROR_INFO * const error_info) { unsigned int streams_options = 0; - dtor_func_t origin_dtor; php_stream * net_stream = NULL; DBG_ENTER("mysqlnd_vio::open_pipe"); @@ -132,17 +145,7 @@ MYSQLND_METHOD(mysqlnd_vio, open_pipe)(MYSQLND_VIO * const vio, const MYSQLND_CS SET_CLIENT_ERROR(error_info, CR_CONNECTION_ERROR, UNKNOWN_SQLSTATE, "Unknown error while connecting"); DBG_RETURN(NULL); } - /* - Streams are not meant for C extensions! Thus we need a hack. Every connected stream will - be registered as resource (in EG(regular_list). So far, so good. However, it won't be - unregistered until the script ends. So, we need to take care of that. - */ - origin_dtor = EG(regular_list).pDestructor; - EG(regular_list).pDestructor = NULL; - zend_hash_index_del(&EG(regular_list), net_stream->res->handle); /* ToDO: should it be res->handle, do streams register with addref ?*/ - EG(regular_list).pDestructor = origin_dtor; - efree(net_stream->res); - net_stream->res = NULL; + mysqlnd_fixup_regular_list(net_stream); DBG_RETURN(net_stream); } @@ -224,17 +227,7 @@ MYSQLND_METHOD(mysqlnd_vio, open_tcp_or_unix)(MYSQLND_VIO * const vio, const MYS mnd_sprintf_free(hashed_details); } - /* - Streams are not meant for C extensions! Thus we need a hack. Every connected stream will - be registered as resource (in EG(regular_list). So far, so good. However, it won't be - unregistered until the script ends. So, we need to take care of that. - */ - origin_dtor = EG(regular_list).pDestructor; - EG(regular_list).pDestructor = NULL; - zend_hash_index_del(&EG(regular_list), net_stream->res->handle); /* ToDO: should it be res->handle, do streams register with addref ?*/ - efree(net_stream->res); - net_stream->res = NULL; - EG(regular_list).pDestructor = origin_dtor; + mysqlnd_fixup_regular_list(net_stream); DBG_RETURN(net_stream); } /* }}} */ From d5e32cb81e8767f1503690ed0a60af095392c0d9 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 21 Mar 2024 23:59:34 +0100 Subject: [PATCH 4/6] Fix crash in shutdown when using a named pipe with a persistent mysqli connection The code originally posted in GH-10599 triggers the bug for non-persistent connections, but changing the host to `p:.` reveals that there is also a crash bug for persistent connections. --- ext/mysqlnd/mysqlnd_vio.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c index 4849633cca661..fc0e712e97c9f 100644 --- a/ext/mysqlnd/mysqlnd_vio.c +++ b/ext/mysqlnd/mysqlnd_vio.c @@ -145,6 +145,33 @@ MYSQLND_METHOD(mysqlnd_vio, open_pipe)(MYSQLND_VIO * const vio, const MYSQLND_CS SET_CLIENT_ERROR(error_info, CR_CONNECTION_ERROR, UNKNOWN_SQLSTATE, "Unknown error while connecting"); DBG_RETURN(NULL); } + + if (persistent) { + /* This is a similar hack as for mysqlnd_vio::open_tcp_or_unix. + * The main difference here is that we have no access to the hashed key. + * We can however perform a loop over the persistent resource list to find + * which one corresponds to our newly allocated stream. + * This loop is pretty cheap because it will normally either be the last entry or second to last entry + * in the list, depending on whether the socket connection itself is persistent or not. + * That's why we use a reverse loop. */ + Bucket *bucket; + /* Use a bucket loop to make deletion cheap. */ + ZEND_HASH_MAP_REVERSE_FOREACH_BUCKET(&EG(persistent_list), bucket) { + zend_resource *current_res = Z_RES(bucket->val); + if (current_res->ptr == net_stream) { + dtor_func_t origin_dtor = EG(persistent_list).pDestructor; + EG(persistent_list).pDestructor = NULL; + zend_hash_del_bucket(&EG(persistent_list), bucket); + EG(persistent_list).pDestructor = origin_dtor; + pefree(current_res, 1); + break; + } + } ZEND_HASH_FOREACH_END(); +#if ZEND_DEBUG + php_stream_auto_cleanup(net_stream); +#endif + } + mysqlnd_fixup_regular_list(net_stream); DBG_RETURN(net_stream); From 9a569d26f29cfa0dc4faebbfb11f68e43c610f38 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 22 Mar 2024 11:56:07 +0100 Subject: [PATCH 5/6] Use php_stream_auto_cleanup() consistently --- ext/mysqlnd/mysqlnd_vio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c index fc0e712e97c9f..7a9fa313922f8 100644 --- a/ext/mysqlnd/mysqlnd_vio.c +++ b/ext/mysqlnd/mysqlnd_vio.c @@ -249,7 +249,7 @@ MYSQLND_METHOD(mysqlnd_vio, open_tcp_or_unix)(MYSQLND_VIO * const vio, const MYS } #if ZEND_DEBUG /* Shut-up the streams, they don't know what they are doing */ - net_stream->__exposed = 1; + php_stream_auto_cleanup(net_stream); #endif mnd_sprintf_free(hashed_details); } From b30796fb7a9b675904e9b9ffc30cfd2ae58b5fd9 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 22 Mar 2024 13:15:45 +0100 Subject: [PATCH 6/6] Add an assertion for certainty --- ext/mysqlnd/mysqlnd_vio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c index 7a9fa313922f8..67210f083a967 100644 --- a/ext/mysqlnd/mysqlnd_vio.c +++ b/ext/mysqlnd/mysqlnd_vio.c @@ -236,6 +236,7 @@ MYSQLND_METHOD(mysqlnd_vio, open_tcp_or_unix)(MYSQLND_VIO * const vio, const MYS zend_resource *le; if ((le = zend_hash_str_find_ptr(&EG(persistent_list), hashed_details, hashed_details_len))) { + ZEND_ASSERT(le->ptr == net_stream); origin_dtor = EG(persistent_list).pDestructor; /* in_free will let streams code skip destructing - big HACK,