This is an automated email from the ASF dual-hosted git repository.
wwbmmm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 3cc7d8e74 leave EndPoint untouched on failed
str2endpoint/hostname2endpoint (#3642)
3cc7d8e74 is described below
commit 3cc7d8e74a0cb10ad6e00ece1dc9498ac8ef0038
Author: sahvx655-wq <[email protected]>
AuthorDate: Sat Oct 31 12:35:04 2026 +0530
leave EndPoint untouched on failed str2endpoint/hostname2endpoint (#3642)
str2endpoint() and hostname2endpoint() stored the parsed ip and port
into the caller's EndPoint before range-checking the port. A failed
parse of "<ip>:123456789" left port == EXTENDED_ENDPOINT_PORT behind,
so the next reset() of that EndPoint called dec_ref() on the live
ExtendedEndPoint whose resource id equals the parsed ip.
-http_header_of_user_ip feeds the request header straight into
str2endpoint(), and the HTTP server then assigns remote_side() over the
failed result, so a client could release references held by unix or
IPv6 endpoints until their slot was returned to the pool and reused.
Parse into locals and assign the EndPoint only on success. Keeping the
strtol() result in a long also stops an out-of-range port from being
narrowed to int before the range check.
---
src/butil/endpoint.cpp | 22 ++++++++++++++++------
test/endpoint_unittest.cpp | 31 +++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/src/butil/endpoint.cpp b/src/butil/endpoint.cpp
index d58dd1441..5625e9bf5 100644
--- a/src/butil/endpoint.cpp
+++ b/src/butil/endpoint.cpp
@@ -282,12 +282,16 @@ int str2endpoint(const char* str, EndPoint* point) {
return -1;
}
buf[i] = '\0';
- if (str2ip(buf, &point->ip) != 0) {
+ // Parse into locals and touch `point' only on success: an out-of-range
+ // port (e.g. EXTENDED_ENDPOINT_PORT) left in `point' would make it look
+ // like an extended endpoint whose resource id is the parsed ip.
+ ip_t ip;
+ if (str2ip(buf, &ip) != 0) {
return -1;
}
++i;
char* end = nullptr;
- point->port = strtol(str + i, &end, 10);
+ const long port = strtol(str + i, &end, 10);
if (end == str + i) {
return -1;
} else if (*end) {
@@ -296,9 +300,11 @@ int str2endpoint(const char* str, EndPoint* point) {
return -1;
}
}
- if (point->port < 0 || point->port > 65535) {
+ if (port < 0 || port > 65535) {
return -1;
}
+ point->ip = ip;
+ point->port = port;
return 0;
}
@@ -334,14 +340,16 @@ int hostname2endpoint(const char* str, EndPoint* point) {
}
buf[i] = '\0';
- if (hostname2ip(buf, &point->ip) != 0) {
+ // See str2endpoint() above for why `point' is assigned only on success.
+ ip_t ip;
+ if (hostname2ip(buf, &ip) != 0) {
return -1;
}
if (str[i] == ':') {
++i;
}
char* end = nullptr;
- point->port = strtol(str + i, &end, 10);
+ const long port = strtol(str + i, &end, 10);
if (end == str + i) {
return -1;
} else if (*end) {
@@ -350,9 +358,11 @@ int hostname2endpoint(const char* str, EndPoint* point) {
return -1;
}
}
- if (point->port < 0 || point->port > 65535) {
+ if (port < 0 || port > 65535) {
return -1;
}
+ point->ip = ip;
+ point->port = port;
return 0;
}
diff --git a/test/endpoint_unittest.cpp b/test/endpoint_unittest.cpp
index 1abb06203..b741b18da 100644
--- a/test/endpoint_unittest.cpp
+++ b/test/endpoint_unittest.cpp
@@ -389,6 +389,37 @@ TEST(EndPointTest, extended_endpoint) {
ASSERT_EQ(1, eep2->_ref_count.load());
}
+TEST(EndPointTest, failed_parse_leaves_endpoint_untouched) {
+ butil::EndPoint ep;
+ ASSERT_EQ(0, butil::str2endpoint("unix:forged.sock", &ep));
+ ExtendedEndPoint* eep = ExtendedEndPoint::address(ep);
+ ASSERT_TRUE(eep);
+ ASSERT_EQ(1, eep->_ref_count.load());
+
+ // The ip field of an extended endpoint holds its resource id, so this
+ // string names `ep' with the port that marks extended endpoints.
+ char forged[64];
+ snprintf(forged, sizeof(forged), "%s:%d",
+ butil::ip2str(ep.ip).c_str(),
butil::details::EXTENDED_ENDPOINT_PORT);
+ {
+ butil::EndPoint tmp;
+ ASSERT_EQ(-1, butil::str2endpoint(forged, &tmp));
+ ASSERT_FALSE(ExtendedEndPoint::is_extended(tmp));
+ ASSERT_EQ(-1, butil::hostname2endpoint(forged, &tmp));
+ ASSERT_FALSE(ExtendedEndPoint::is_extended(tmp));
+ // Callers fall back to another address after a failed parse.
+ tmp = butil::EndPoint(butil::IP_ANY, 80);
+ }
+ ASSERT_EQ(1, eep->_ref_count.load());
+
+ // An out-of-range port must not be narrowed to int before the range check.
+ butil::EndPoint ep2;
+ ASSERT_EQ(-1, butil::str2endpoint("127.0.0.1:4294967377", &ep2));
+ ASSERT_EQ(-1, butil::hostname2endpoint("127.0.0.1:4294967377", &ep2));
+ ASSERT_EQ(butil::IP_ANY, ep2.ip);
+ ASSERT_EQ(0, ep2.port);
+}
+
TEST(EndPointTest, endpoint_compare) {
butil::EndPoint ep1, ep2, ep3;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
