[go: up one dir, main page]

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible Memory Leak with SSL-enabled MySQL connections #8979

Closed
orware opened this issue Jul 12, 2022 · 5 comments
Closed

Possible Memory Leak with SSL-enabled MySQL connections #8979

orware opened this issue Jul 12, 2022 · 5 comments

Comments

@orware
Copy link
orware commented Jul 12, 2022

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
Copy link

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
Copy link
Author
orware commented Sep 4, 2022

@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
Copy link
Member

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
Copy link
Author
orware commented Mar 21, 2023

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 added a commit to nielsdos/php-src that referenced this issue Mar 22, 2023
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.
@nielsdos
Copy link
Member

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.

nielsdos added a commit that referenced this issue Mar 24, 2023
* PHP-8.1:
  Fix GH-8979: Possible Memory Leak with SSL-enabled MySQL connections
nielsdos added a commit that referenced this issue Mar 24, 2023
* PHP-8.2:
  Fix GH-10907: Unable to serialize processed SplFixedArrays in PHP 8.2.4
  Fix GH-8979: Possible Memory Leak with SSL-enabled MySQL connections
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@orware @cmb69 @nielsdos @whataboutpereira and others