Copilot commented on code in PR #3642:
URL: https://github.com/apache/brpc/pull/3642#discussion_r4355724155
##########
src/butil/endpoint.cpp:
##########
@@ -279,12 +279,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) {
Review Comment:
This fixes the string-parsing overloads, but the same partial-write hazard
still exists for the `(const char*, int, EndPoint*)` overloads shown in
context: they assign `point->ip` before validating `port`, so a failed port
check can still leave `EndPoint` mutated (including corrupting an existing
extended endpoint state/refcount if `*point` was previously extended). To fully
implement the PR’s safety guarantee, consider applying the same pattern there:
resolve into a local `ip_t` and only assign `point->ip/port` after the port
range check passes.
##########
src/butil/endpoint.cpp:
##########
@@ -293,9 +297,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;
Review Comment:
After validating `port` is within 0–65535, this assignment implicitly
narrows from `long` to the type of `EndPoint::port`. To avoid compiler warnings
(and potential -Werror build breaks on some toolchains), cast explicitly (e.g.,
`static_cast<int>(port)` if `port` is an `int`) or store `port` as the
destination type after validation.
##########
src/butil/endpoint.cpp:
##########
@@ -293,9 +297,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;
Review Comment:
This fixes the string-parsing overloads, but the same partial-write hazard
still exists for the `(const char*, int, EndPoint*)` overloads shown in
context: they assign `point->ip` before validating `port`, so a failed port
check can still leave `EndPoint` mutated (including corrupting an existing
extended endpoint state/refcount if `*point` was previously extended). To fully
implement the PR’s safety guarantee, consider applying the same pattern there:
resolve into a local `ip_t` and only assign `point->ip/port` after the port
range check passes.
##########
test/endpoint_unittest.cpp:
##########
@@ -357,6 +357,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));
Review Comment:
The test intent is that a failed parse leaves the output `EndPoint`
untouched. Right now `tmp` starts default-initialized, so the assertions mostly
verify it’s not accidentally becoming an extended endpoint. To make the test
more direct/robust, set `tmp` to a non-default sentinel value before each
failed call and assert both `ip` and `port` are unchanged afterward (in
addition to the refcount check).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
