Possible Memory Leak with SSL-enabled MySQL connections

[orware]

Description

This was an issue that was primarily uncovered when creating a database connection testing utility that measured the time it took to connect to a MySQL database repeatedly within an infinite loop so that the utility could run indefinitely as a process managed by supervisord.

Leaving multiple instances of this utility running on a server (each testing connection times for a different database) for an extended period would eventually result in the entire server becoming unresponsive and I eventually realized it appeared to be due to a memory leak of some sort.

After some investigation, it appears that the memory usage for PHP MySQL connections (both using the mysqli and PDO MySQL options) increases when you are connecting to MySQL databases that use SSL.

In local testing, switching my utility program to connect to a non-SSL MySQL database did not result in the same sort of memory increases over time, leading me to the conclusion that this is likely only an issue specific for SSL-enabled MySQL connections.

It's possible that the memory usage increase over time is expected for some reason, but it felt unexpected so I feel this is more of a memory leak bug that can hopefully be addressed eventually for PHP users.

The main difference between creating the two types of connections (either for mysqli or the PDO MySQL option) is that I am passing in a path to a certificate file for the connection to be established correctly so I'm not sure if the extensions somehow maintain some extra memory every time it loads in that certificate and doesn't release it when the connection is closed, or if there is something else specific to SSL-enabled connections that is resulting in the extra memory usage not being freed.

For the SSL-enabled MySQL connections my utility resulted in output like this:

The script starts out with this memory usage around iteration 1 of the utility:
After PDO Connection Attempt:
    The script is now using: 476392 B of memory.
    The script is now using: 465 KB of memory.
    Peak usage: 466 KB of memory.

And memory usage to the following after about 130 iterations of the utility:
After PDO Connection Attempt:
    The script is now using: 617384 B of memory.
    The script is now using: 603 KB of memory.
    Peak usage: 603 KB of memory.

But I expected this output instead, where memory usage remains pretty much consistent for non-SSL enabled MySQL connections:

For the non-SSL enabled connections this was iteration 1:
After PDO Connection Attempt:
    The script is now using: 473328 B of memory.
    The script is now using: 462 KB of memory.
    Peak usage: 463 KB of memory.

And after 400+ iterations the memory usage was still basically the same:
After PDO Connection Attempt:
    The script is now using: 473584 B of memory.
    The script is now using: 462 KB of memory.
    Peak usage: 463 KB of memory.

This issue likely wouldn't be noticeable for the majority of PHP users since PHP's short lived connections would ensure the memory is freed at the end of the request, but it would come into play more so for any users making use of a cloud MySQL database connection, which typically rely on SSL for security, and connect to that database via a process that might run continuously, which would still be a small number of users overall.

Testing instructions:

You may use the following link and save a local version to test with:
https://3v4l.org/BOnq5

Grab the latest cacert.pem file from:
https://curl.se/docs/caextract.html
(and place it in the same folder as the file above)

And then lastly create a config.ini file in the same folder with the following contents:

ca_root_path="./cacert.pem"

[database]
host=localhost
username=root
password=
database=garbage-collection
port=3306
use_ssl=0

;[database]
;host=
;username=
;password=
;database=garbage-collection
;port=3306
;use_ssl=1

[common-parameters]
number_of_iterations=0
iteration_delay=1
show_output=1

The first [database] section is intended for a local database that would be used for testing a non-SSL enabled database connection while the second [database] section is intended for an SSL-enabled connection and you can simply comment out one of the sections depending on what you want to test.

After creating a local garbage-collection database you should be able to confirm the "happy path" of using a non-SSL enabled database connection using the script and running it from the command line and noting that memory usage does not seem to increase in that scenario.

Then you can test out an SSL-enabled database pretty easily using PlanetScale which would likely be the quickest / free option with the Hobby plan that's available, although I also tried out Digital Ocean's Managed MySQL database option as well and confirmed the same behavior shared above as well for it's SSL-enabled database connections (I had to switch out the cacert.pem usage for their provided ca-certificate.crt file and adjust a few other pieces of information such as the port number when testing out the Digital Ocean option).

If there's anything else I can assist with please let me know!

PHP Version

PHP 8.1.5

Operating System

Confirmed on Windows 11 and Ubuntu 20.04

[whataboutpereira]

I haven't had a chance to dig deeper, but my continuously running script has already started to run out of memory right after switching SSL on for PDO MySQL.

[orware]

@whataboutpereira thank you for helping add some outside confirmation to the conversation for this one!

I'm actually planning on working on a Golang equivalent type script/test to see if the same thing occurs for SSL-enabled connections there too, although my current expectation is that the same memory leaking issue wouldn't be present in comparison to what I've been observing with my PHP example shared back in July above.

I'm hoping however that someone with more knowledge regarding how the PHP MySQL and OpenSSL libraries interact can shed more light on how this memory leak issue might be fixed though since it would be helpful for creating longer-lived processes that might need to connect/disconnect from a secure MySQL connection many times.

[nielsdos]

I can confirm this leak on 8.1+.
I identified the source of the leak inside mysqlnd_vio::enable_ssl(): it's the stream context that's 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 this issue report already implicitly mentioned: there can be long-running PHP scripts.

The fix to me seems simple: we actually want to transfer the ownership of context as it never escapes this function, so we can decrease its refcount after setting the stream context like so:

diff --git a/ext/mysqlnd/mysqlnd_vio.c b/ext/mysqlnd/mysqlnd_vio.c
index e2194bca13..9cb2b4a594 100644
--- a/ext/mysqlnd/mysqlnd_vio.c
+++ b/ext/mysqlnd/mysqlnd_vio.c
@@ -561,6 +561,9 @@ 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. */
+	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)
 	{

Unfortunately, I'm out of time for today so I'll have to test this and figure out how to make a .phpt tomorrow.

[orware]

Thank you so much for your in-depth analysis of my issue report @nielsdos and I look forward to reading up on any other discoveries you might find as you continue your testing!

[nielsdos]

Well, thank you actually for the easy reproducer!
I think the issue I mentioned above is the only leak and I created a PR to fix it.
Thanks.