This is an automated email from the ASF dual-hosted git repository.
cmcfarlen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new a31fea8459 cleanup ProxyProtocol and prevent memory leak (#12680)
a31fea8459 is described below
commit a31fea84592568938e8c0809676afd995c25f1d4
Author: Chris McFarlen <[email protected]>
AuthorDate: Mon Nov 24 12:00:20 2025 -0600
cleanup ProxyProtocol and prevent memory leak (#12680)
* cleanup ProxyProtocol and prevent memory leak
* cleanup headers
---
include/iocore/net/ProxyProtocol.h | 58 ++++-
include/proxy/http/HttpTransact.h | 4 +
src/iocore/net/ProxyProtocol.cc | 9 +-
src/iocore/net/unit_tests/test_ProxyProtocol.cc | 290 ++++++++++++++++++++++++
4 files changed, 352 insertions(+), 9 deletions(-)
diff --git a/include/iocore/net/ProxyProtocol.h
b/include/iocore/net/ProxyProtocol.h
index 110584be7e..2cc4848dfb 100644
--- a/include/iocore/net/ProxyProtocol.h
+++ b/include/iocore/net/ProxyProtocol.h
@@ -64,7 +64,23 @@ public:
: version(pp_ver), ip_family(family), src_addr(src), dst_addr(dst)
{
}
- ~ProxyProtocol() { ats_free(additional_data); }
+ ProxyProtocol(const ProxyProtocol &other)
+ : version(other.version), ip_family(other.ip_family),
src_addr(other.src_addr), dst_addr(other.dst_addr)
+ {
+ if (!other.additional_data.empty()) {
+ set_additional_data(other.additional_data);
+ }
+ }
+ ProxyProtocol(ProxyProtocol &&other)
+ : version(other.version), ip_family(other.ip_family),
src_addr(other.src_addr), dst_addr(other.dst_addr)
+ {
+ if (!other.additional_data.empty()) {
+ set_additional_data(other.additional_data);
+ }
+ other.additional_data.clear();
+ other.tlv.clear();
+ }
+ ~ProxyProtocol() = default;
int set_additional_data(std::string_view data);
void set_ipv4_addrs(in_addr_t src_addr, uint16_t src_port, in_addr_t
dst_addr, uint16_t dst_port);
void set_ipv6_addrs(const in6_addr &src_addr, uint16_t src_port, const
in6_addr &dst_addr, uint16_t dst_port);
@@ -78,8 +94,46 @@ public:
IpEndpoint dst_addr = {};
std::unordered_map<uint8_t, std::string_view> tlv;
+ ProxyProtocol &
+ operator=(const ProxyProtocol &other)
+ {
+ if (&other == this) {
+ return *this;
+ }
+ version = other.version;
+ ip_family = other.ip_family;
+ src_addr = other.src_addr;
+ dst_addr = other.dst_addr;
+ if (!other.additional_data.empty()) {
+ set_additional_data(other.additional_data);
+ } else {
+ additional_data.clear();
+ tlv.clear();
+ }
+ return *this;
+ }
+
+ ProxyProtocol &
+ operator=(ProxyProtocol &&other)
+ {
+ version = other.version;
+ ip_family = other.ip_family;
+ src_addr = other.src_addr;
+ dst_addr = other.dst_addr;
+
+ additional_data.clear();
+ tlv.clear();
+
+ if (!other.additional_data.empty()) {
+ set_additional_data(other.additional_data);
+ }
+ other.additional_data.clear();
+ other.tlv.clear();
+ return *this;
+ }
+
private:
- char *additional_data = nullptr;
+ std::string additional_data;
};
const size_t PPv1_CONNECTION_HEADER_LEN_MAX = 108;
diff --git a/include/proxy/http/HttpTransact.h
b/include/proxy/http/HttpTransact.h
index 01bcb8182e..151ad844a1 100644
--- a/include/proxy/http/HttpTransact.h
+++ b/include/proxy/http/HttpTransact.h
@@ -25,6 +25,7 @@
#include <cstddef>
+#include "iocore/net/ProxyProtocol.h"
#include "tsutil/DbgCtl.h"
#include "tscore/ink_assert.h"
#include "tscore/ink_platform.h"
@@ -883,6 +884,9 @@ public:
delete[] ranges;
ranges = nullptr;
range_setup = RangeSetup_t::NONE;
+
+ // This avoids a potential leak since sometimes this class is not
destructed (ClassAllocated via HttpSM)
+ pp_info.~ProxyProtocol();
return;
}
diff --git a/src/iocore/net/ProxyProtocol.cc b/src/iocore/net/ProxyProtocol.cc
index 27b1c84301..0422e70771 100644
--- a/src/iocore/net/ProxyProtocol.cc
+++ b/src/iocore/net/ProxyProtocol.cc
@@ -552,14 +552,9 @@ ProxyProtocol::set_additional_data(std::string_view data)
{
uint16_t len = data.length();
Dbg(dbg_ctl_proxyprotocol_v2, "Parsing %d byte additional data", len);
- additional_data = static_cast<char *>(ats_malloc(len));
- if (additional_data == nullptr) {
- Dbg(dbg_ctl_proxyprotocol_v2, "Memory allocation failed");
- return -1;
- }
- data.copy(additional_data, len);
+ additional_data.assign(data);
- const char *p = additional_data;
+ const char *p = additional_data.data();
const char *end = p + len;
while (p != end) {
if (end - p < 3) {
diff --git a/src/iocore/net/unit_tests/test_ProxyProtocol.cc
b/src/iocore/net/unit_tests/test_ProxyProtocol.cc
index 920cd971fc..56e3aed064 100644
--- a/src/iocore/net/unit_tests/test_ProxyProtocol.cc
+++ b/src/iocore/net/unit_tests/test_ProxyProtocol.cc
@@ -625,3 +625,293 @@ TEST_CASE("ProxyProtocol v2 Builder",
"[ProxyProtocol][ProxyProtocolv2]")
CHECK(memcmp(expected, buf, len) == 0);
}
}
+
+TEST_CASE("ProxyProtocol Rule of 5", "[ProxyProtocol]")
+{
+ SECTION("Copy constructor with no additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+ ats_ip_pton("198.51.100.1:443", original.dst_addr);
+
+ ProxyProtocol copy(original);
+
+ CHECK(copy.version == original.version);
+ CHECK(copy.ip_family == original.ip_family);
+ CHECK(copy.src_addr == original.src_addr);
+ CHECK(copy.dst_addr == original.dst_addr);
+ }
+
+ SECTION("Copy constructor with additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+
+ // Set properly formatted TLV data: type=0x01, length=0x0002, value="h2"
+ std::string_view tlv_data("\x01\x00\x02h2", 5);
+ original.set_additional_data(tlv_data);
+
+ ProxyProtocol copy(original);
+
+ CHECK(copy.version == original.version);
+ CHECK(copy.ip_family == original.ip_family);
+ CHECK(copy.src_addr == original.src_addr);
+
+ // Verify the data was copied
+ auto original_tlv = original.get_tlv(0x01);
+ auto copy_tlv = copy.get_tlv(0x01);
+ REQUIRE(original_tlv.has_value());
+ REQUIRE(copy_tlv.has_value());
+ CHECK(original_tlv.value() == "h2");
+ CHECK(copy_tlv.value() == "h2");
+ }
+
+ SECTION("Move constructor with no additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+ ats_ip_pton("198.51.100.1:443", original.dst_addr);
+
+ ProxyProtocol moved(std::move(original));
+
+ CHECK(moved.version == ProxyProtocolVersion::V2);
+ CHECK(moved.ip_family == AF_INET);
+ }
+
+ SECTION("Move constructor with additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+
+ // Set properly formatted TLV data: type=0x01, length=0x0002, value="h2"
+ std::string_view tlv_data("\x01\x00\x02h2", 5);
+ original.set_additional_data(tlv_data);
+
+ auto original_tlv = original.get_tlv(0x01);
+ REQUIRE(original_tlv.has_value());
+ CHECK(original_tlv.value() == "h2");
+
+ ProxyProtocol moved(std::move(original));
+
+ // Verify the moved object has the data
+ CHECK(moved.version == ProxyProtocolVersion::V2);
+ auto moved_tlv = moved.get_tlv(0x01);
+ REQUIRE(moved_tlv.has_value());
+ CHECK(moved_tlv.value() == "h2");
+
+ // Original should have been moved from (additional_data set to nullptr)
+ auto original_tlv_after = original.get_tlv(0x01);
+ CHECK_FALSE(original_tlv_after.has_value());
+ }
+
+ SECTION("Copy assignment with no additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+
+ ProxyProtocol copy;
+ copy = original;
+
+ CHECK(copy.version == original.version);
+ CHECK(copy.ip_family == original.ip_family);
+ CHECK(copy.src_addr == original.src_addr);
+ }
+
+ SECTION("Copy assignment with additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+
+ // Set properly formatted TLV data: type=0x02, length=0x0003, value="abc"
+ std::string_view tlv_data("\x02\x00\x03"
+ "abc",
+ 6);
+ original.set_additional_data(tlv_data);
+
+ ProxyProtocol copy;
+ copy = original;
+
+ CHECK(copy.version == original.version);
+ CHECK(copy.ip_family == original.ip_family);
+
+ // Verify deep copy - both should have independent data
+ auto original_tlv = original.get_tlv(0x02);
+ auto copy_tlv = copy.get_tlv(0x02);
+ REQUIRE(original_tlv.has_value());
+ REQUIRE(copy_tlv.has_value());
+ CHECK(original_tlv.value() == "abc");
+ CHECK(copy_tlv.value() == "abc");
+ }
+
+ SECTION("Copy assignment overwrites existing additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2; // Must be V2 for get_tlv
to work
+ original.ip_family = AF_INET;
+
+ // Set properly formatted TLV data for original: type=0x01, length=0x0008,
value="original"
+ std::string_view orig_tlv_data("\x01\x00\x08original", 11);
+ original.set_additional_data(orig_tlv_data);
+
+ ProxyProtocol copy;
+ copy.version = ProxyProtocolVersion::V2;
+ copy.ip_family = AF_INET6;
+
+ // Set properly formatted TLV data for copy: type=0x02, length=0x0004,
value="copy"
+ std::string_view copy_tlv_data("\x02\x00\x04copy", 7);
+ copy.set_additional_data(copy_tlv_data);
+
+ copy = original;
+
+ // After assignment, copy should have original's data
+ CHECK(copy.version == ProxyProtocolVersion::V2);
+ CHECK(copy.ip_family == AF_INET);
+
+ // Verify copy now has original's TLV data
+ auto copy_tlv = copy.get_tlv(0x01);
+ REQUIRE(copy_tlv.has_value());
+ CHECK(copy_tlv.value() == "original");
+
+ // Old TLV should be gone
+ auto old_tlv = copy.get_tlv(0x02);
+ CHECK_FALSE(old_tlv.has_value());
+ }
+
+ SECTION("Move assignment with no additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+
+ ProxyProtocol target;
+ target = std::move(original);
+
+ CHECK(target.version == ProxyProtocolVersion::V2);
+ CHECK(target.ip_family == AF_INET);
+ }
+
+ SECTION("Move assignment with additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+ ats_ip_pton("192.0.2.1:50000", original.src_addr);
+
+ // Set properly formatted TLV data: type=0x01, length=0x0004, value="test"
+ std::string_view tlv_data("\x01\x00\x04test", 7);
+ original.set_additional_data(tlv_data);
+
+ auto original_tlv = original.get_tlv(0x01);
+ REQUIRE(original_tlv.has_value());
+ CHECK(original_tlv.value() == "test");
+
+ ProxyProtocol target;
+ target = std::move(original);
+
+ // Verify target has the data
+ CHECK(target.version == ProxyProtocolVersion::V2);
+ auto target_tlv = target.get_tlv(0x01);
+ REQUIRE(target_tlv.has_value());
+ CHECK(target_tlv.value() == "test");
+
+ // Original should have been moved from
+ auto original_tlv_after = original.get_tlv(0x01);
+ CHECK_FALSE(original_tlv_after.has_value());
+ }
+
+ SECTION("Move assignment overwrites existing additional_data")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2; // Must be V2 for get_tlv
to work
+ original.ip_family = AF_INET;
+
+ // Set properly formatted TLV data for original: type=0x01, length=0x0008,
value="original"
+ std::string_view orig_tlv_data("\x01\x00\x08original", 11);
+ original.set_additional_data(orig_tlv_data);
+
+ ProxyProtocol target;
+ target.version = ProxyProtocolVersion::V2;
+ target.ip_family = AF_INET6;
+
+ // Set properly formatted TLV data for target: type=0x02, length=0x0006,
value="target"
+ std::string_view target_tlv_data("\x02\x00\x06target", 9);
+ target.set_additional_data(target_tlv_data);
+
+ target = std::move(original);
+
+ // After move assignment, target should have original's data
+ CHECK(target.version == ProxyProtocolVersion::V2);
+ CHECK(target.ip_family == AF_INET);
+
+ // Verify target has original's TLV
+ auto target_tlv = target.get_tlv(0x01);
+ REQUIRE(target_tlv.has_value());
+ CHECK(target_tlv.value() == "original");
+
+ // Original should have been moved from
+ auto original_tlv = original.get_tlv(0x01);
+ CHECK_FALSE(original_tlv.has_value());
+ }
+
+ SECTION("Destructor cleans up additional_data")
+ {
+ // Test that destructor properly frees memory
+ // This is mainly tested through sanitizers (ASAN)
+ {
+ ProxyProtocol pp;
+ pp.version = ProxyProtocolVersion::V2; // get_tlv requires V2
+
+ // Set properly formatted TLV data: type=0x01, length=0x0004,
value="test"
+ std::string_view tlv_data("\x01\x00\x04test", 7);
+ pp.set_additional_data(tlv_data);
+
+ auto tlv = pp.get_tlv(0x01);
+ REQUIRE(tlv.has_value());
+ CHECK(tlv.value() == "test");
+ }
+ // Object destroyed here - ASAN will catch any memory leaks
+ }
+
+ SECTION("Multiple copy and move operations")
+ {
+ ProxyProtocol original;
+ original.version = ProxyProtocolVersion::V2;
+ original.ip_family = AF_INET;
+
+ // Set properly formatted TLV data: type=0x01, length=0x0008,
value="original"
+ std::string_view tlv_data("\x01\x00\x08original", 11);
+ original.set_additional_data(tlv_data);
+
+ ProxyProtocol copy1(original);
+ ProxyProtocol copy2;
+ copy2 = copy1;
+
+ ProxyProtocol moved1(std::move(copy2));
+ ProxyProtocol moved2;
+ moved2 = std::move(moved1);
+
+ // Final object should have the data
+ CHECK(moved2.version == ProxyProtocolVersion::V2);
+ auto final_tlv = moved2.get_tlv(0x01);
+ REQUIRE(final_tlv.has_value());
+ CHECK(final_tlv.value() == "original");
+
+ // Original should still have its data (wasn't moved from)
+ auto orig_tlv = original.get_tlv(0x01);
+ REQUIRE(orig_tlv.has_value());
+ CHECK(orig_tlv.value() == "original");
+ }
+}
