Fix GH-8979: Possible Memory Leak with SSL-enabled MySQL connections #10909
Conversation
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.
Do you think it would be too complicated to add one ? real question, I have no idea honestly. |
|
This looks sensible but I'd like for @kamil-tekiela to have a look as AFAIK he did some stuff on the MySQLnd driver recently |
|
I don't really know how I can't find any uses of this function with second argument NULL. Are we sure it should not be fixed within the function? Again, not really understanding how it works. We don't have tests for SSL and I don't know how we could add one. We would need an SSL server but I don't know if we can do that in CI. |
I used the reporter's database service. I made an account & database on that. I can send you the DB credentials along with a .zip with the script & ini configuration file in private if you want to test it.
Works fine under ASAN.
That call is fine. What is happening is that
Yeah this sounds pretty hard tbh. I know you can run mysql in a mode that allows both SSL and non-SSL connections. But I don't know how this would work with certificates etc for CI. |
Yes, please. If you can send it to my email and I will review over the weekend. |
Great! Done that now, thanks :) |
Fixes GH-8979
The stream context inside
mysqlnd_vio::enable_ssl()is leaking. In particular: whenphp_stream_context_set()get called the refcount ofcontextis increased by 1, which means thatcontextwill now have a refcount of 2. Later on we remove the context from the stream by callingphp_stream_context_set(stream, NULL)but that leaves ourcontextwith 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.
We don't seem to have any SSL mysqlnd tests? I tested the bug using the reproduction instructions in the linked issue which uses an external server behind SSL.