From 9a46a08fcc88459e59cb209071836f3cd1abb92f Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 22 Mar 2023 21:49:37 +0100 Subject: [PATCH] Fix GH-8979: Possible Memory Leak with SSL-enabled MySQL connections The stream context inside `mysqlnd_vio::enable_ssl()` is leaking. In particular: when `php_stream_context_set()` get called the refcount of `context` is increased by 1, which means that `context` will now have a refcount of 2. Later on we remove the context from the stream by calling `php_stream_context_set(stream, NULL)` but that leaves our `context` with a refcount of 1, and therefore it's never destroyed. In my test case this yielded a leak of 1456 bytes per connection (but could be more depending on your settings ofc). Annoyingly, Valgrind doesn't find it because the context is still in the `EG(regular_list)` and will thus be destroyed at the end of the request. However, I still think this bug needs to be fixed because as the users in the issue report already mentioned: there can be long-running PHP scripts. Fix it by decreasing the refcount to transfer the ownership. --- ext/mysqlnd/mysqlnd_vio.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c index e2194bca136a1..2bd77906a1b9a 100644 --- a/ext/mysqlnd/mysqlnd_vio.c +++ b/ext/mysqlnd/mysqlnd_vio.c @@ -561,6 +561,10 @@ MYSQLND_METHOD(mysqlnd_vio, enable_ssl)(MYSQLND_VIO * const net) } } php_stream_context_set(net_stream, context); + /* php_stream_context_set() increases the refcount of context, but we just want to transfer ownership + * hence the need to decrease the refcount so the refcount will be equal to 1. */ + ZEND_ASSERT(GC_REFCOUNT(context->res) == 2); + GC_DELREF(context->res); if (php_stream_xport_crypto_setup(net_stream, STREAM_CRYPTO_METHOD_TLS_CLIENT, NULL) < 0 || php_stream_xport_crypto_enable(net_stream, 1) < 0) {