MDEV-37556 Memory leak in proxy protocol with name resolution enabled#5135
MDEV-37556 Memory leak in proxy protocol with name resolution enabled#5135vaintroub wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a memory leak in the proxy protocol for non-loopback connections (MDEV-37556) by freeing any previously resolved hostname before overwriting it in thd_set_peer_addr. It also adds a corresponding integration test test_proxy_header_dbug_remote_connection to verify the fix. The reviewer suggested improving portability by statically defining the binary representation of the IPv6 address instead of using inet_pton, which can introduce platform-specific socket library dependencies.
There was a problem hiding this comment.
Pull request overview
Fixes a connection-time memory leak when Proxy Protocol is enabled and hostname resolution is active, by ensuring previously resolved hostnames are freed before being overwritten on subsequent thd_set_peer_addr() calls.
Changes:
- Free any previously resolved
THD::main_security_ctx.hostbefore callingip_to_hostname()again inthd_set_peer_addr(). - Add a debug-only
mysql_client_testcase that forces non-loopback peer/proxied addresses via DBUG hooks to exercise the double-resolution path under MTR.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/mysql_client_test.c |
Adds a debug-only regression test that fakes peer address + DNS resolution to reproduce the double ip_to_hostname() scenario with Proxy Protocol. |
sql/sql_connect.cc |
Frees an existing resolved hostname in main_security_ctx.host before overwriting it to prevent leaks on repeated thd_set_peer_addr() calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| --loose-enable-performance-schema | ||
| --max-allowed-packet=32000000 | ||
| --proxy-protocol-networks=::1/32,127.0.0.0/8,localhost | ||
| --proxy-protocol-networks=::1/32,127.0.0.0/8,localhost,2001:db8::6:6 | ||
| --sequence=on |
vuvova
left a comment
vuvova
left a comment
There was a problem hiding this comment.
fix looks ok. two gemini comments look reasonable and probably deserve to be addressed.
When proxy protocol is used and --skip-name-resolve is not set, thd_set_peer_addr() is called twice per connection: once for the real TCP peer (in check_connection), and again for the proxied address (in handle_proxy_header). Each call invokes ip_to_hostname(), which allocates a hostname string (unless loopback connection is used) and stores it as thd->main_security_ctx.host. That code missed to free previously allocated hostname, which results into memory leak. This is now fixed. Also added debug-only test to mysql_client_test, which fakes DNS and IP resolution the same way some perfschema tests do, to emulate remote TCP connection in MTR.
Copilot comments deserved to be addressed, which I also did now. Gemini not so much, it just nitpicked about inet_ntop, with its concerns about portability, but if it read the test attentively, it would find that it is already used, in the same file. |
Right. Sorry. I meant "Two non-resolved AI comments". Indeed, they were Copilot's |
When proxy protocol is used and --skip-name-resolve is not set, thd_set_peer_addr() is called twice per connection: once for the real TCP peer (in check_connection), and again for the proxied address (in handle_proxy_header). Each call invokes ip_to_hostname(), which allocates a hostname string (unless loopback connection is used) and stores it as thd->main_security_ctx.host. That code missed to free previously allocated hostname, which results into memory leak.
This is now fixed. Also added debug-only test to mysql_client_test, which fakes DNS and IP resolution the same way some perfschema tests do, to emulate remote TCP connection in MTR.