ProxyProtocol: free pp_info heap on NetVConnection recycle#13292
Closed
moonchen wants to merge 1 commit into
Closed
ProxyProtocol: free pp_info heap on NetVConnection recycle#13292moonchen wants to merge 1 commit into
moonchen wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a per-connection recycle memory leak when inbound PROXY v2 headers are parsed into NetVConnection::pp_info but the VC is later recycled via a ClassAllocator configured with Destruct_on_free=false.
Changes:
- Add
ProxyProtocol::reset()to explicitly release heap-owned state (additional_dataandtlv) and restore default values. - Call
pp_info.reset()fromUnixNetVConnection::clear()so recycled NetVCs don’t retain/abandon PROXY-protocol allocations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/iocore/net/UnixNetVConnection.cc |
Ensures pp_info heap allocations are released during VC recycle (clear()). |
include/iocore/net/ProxyProtocol.h |
Adds ProxyProtocol::reset() which frees container capacity via swap and resets fields. |
Comment on lines
+86
to
+102
| /** Release owned heap and reset to the default-constructed state. | ||
| * | ||
| * Swap rather than clear() the containers: clear() retains capacity, which would be abandoned | ||
| * (leaked) when the slot is reused, since the NetVConnection allocators are Destruct_on_free=false | ||
| * and so never run ~ProxyProtocol. | ||
| */ | ||
| void | ||
| reset() | ||
| { | ||
| std::string{}.swap(additional_data); | ||
| std::unordered_map<uint8_t, std::string_view>{}.swap(tlv); | ||
| version = ProxyProtocolVersion::UNDEFINED; | ||
| ip_family = AF_UNSPEC; | ||
| type = 0; | ||
| src_addr = {}; | ||
| dst_addr = {}; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Added a TEST_CASE("ProxyProtocol reset") in test_ProxyProtocol.cc (commit 98c0183): it parses a PROXY v2 header carrying TLVs into pp_info, calls reset(), and asserts version == UNDEFINED, ip_family == AF_UNSPEC, type == 0, both addresses cleared, tlv.empty(), and get_tlv(PP2_TYPE_ALPN) == nullopt, plus an idempotent second reset(). Verified locally: All tests passed (14 assertions).
NetVConnection embeds a ProxyProtocol by value, and a PROXY v2 header parsed by has_proxy_protocol() heap-allocates ProxyProtocol::additional_data (the TLV blob) plus the tlv map. The NetVConnection ClassAllocators are Destruct_on_free=false, so ~ProxyProtocol never runs when a VC is recycled and UnixNetVConnection::clear() did not release pp_info -- the next placement-new on the recycled slot abandoned the buffer and map nodes, leaking once per recycled connection that carried a PROXY v2 header. Behind a PROXY-protocol load balancer that is every inbound connection. Add ProxyProtocol::reset() and call it from UnixNetVConnection::clear() (the single recycle chokepoint for the Unix, SSL and QUIC VCs). reset() swaps the members with empty containers rather than clear()ing them, because clear() retains capacity that the recycle would still abandon. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
moonchen
force-pushed
the
mchen/proxyprotocol-free-pp-info
branch
from
6f21391 to
98c0183
Compare
Contributor
Author
|
Superseded by #13293, which carries the identical commit from my fork (moonchen) instead of a branch on the apache repo. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
NetVConnectionembedsProxyProtocol pp_infoby value, andProxyProtocolowns heap (theadditional_datastring and the parsedtlvmap).has_proxy_protocol()parses an inbound PROXY v2 header intopp_infoonce per connection, allocating both.The NetVC
ClassAllocators areDestruct_on_free=false, so~ProxyProtocolnever runs when a VC is recycled andUnixNetVConnection::clear()did not releasepp_info. The slot's next placement-newthen abandons the string buffer and map nodes — leaking once per recycled connection that carried a PROXY v2 header, i.e. every inbound connection behind a PROXY-protocol load balancer.Fix
Add
ProxyProtocol::reset()and call it fromUnixNetVConnection::clear(), the single recycle chokepoint (SSL and QUICclear()chain to it, andclear()runs only on the free path).reset()swaps the members with empty containers rather thanclear()ing them, sinceclear()keeps capacity that the recycle would still abandon. Releasingpp_infoinclear()is safe: it is read only while the VC is live.Targeted rather than flipping the allocators to
Destruct_on_free=true, which would run the full destructor chain on top of the existing manualclear()/free_thread()teardown and needs a separate idempotency audit.