Copilot commented on code in PR #3434:
URL: https://github.com/apache/brpc/pull/3434#discussion_r3788654020
##########
src/brpc/uri.cpp:
##########
@@ -86,10 +86,16 @@ inline const char* SplitHostAndPort(const char* host_begin,
uint64_t multiply = 1;
for (const char* q = host_end - 1; q > host_begin; --q) {
if (*q >= '0' && *q <= '9') {
- port_raw += (*q - '0') * multiply;
- multiply *= 10;
+ // Stop accumulating once out of range. This avoids uint64 overflow
+ // of port_raw/multiply and the narrowing to int below, which would
+ // otherwise turn an out-of-range port into a valid-looking wrong
+ // one (e.g. ":4294967377" truncating to 81).
+ if (port_raw <= 65535) {
+ port_raw += (*q - '0') * multiply;
+ multiply *= 10;
+ }
} else if (*q == ':') {
- *port = static_cast<int>(port_raw);
+ *port = (port_raw <= 65535) ? static_cast<int>(port_raw) : -1;
return q;
Review Comment:
The new `port_raw <= 65535` guard does not actually prevent overflow/wrap
for long digit runs where the numeric value stays <= 65535 for a long time
(e.g., many leading zeros). In that case `multiply *= 10` can wrap (after 64+
digits `10^n` becomes 0 mod 2^64), and an out-of-range port like `:1` followed
by 64 zeros can incorrectly parse as port 0 instead of -1.
##########
test/brpc_uri_unittest.cpp:
##########
@@ -89,6 +89,37 @@ TEST(URITest, only_host) {
ASSERT_EQ(0u, uri.QueryCount());
}
+TEST(URITest, out_of_range_port) {
+ brpc::URI uri;
+ // 4294967377 == 2^32 + 81. Without a range check the accumulated value
+ // narrows to int and yields 81, so port() must not return the wrapped
port.
+ ASSERT_EQ(0, uri.SetHttpURL("foo://www.baidu.com:4294967377/s"));
+ ASSERT_EQ(-1, uri.port());
+ ASSERT_EQ("www.baidu.com", uri.host());
+ ASSERT_EQ("/s", uri.path());
+
+ // Just above the valid range is rejected too.
+ ASSERT_EQ(0, uri.SetHttpURL("foo://www.baidu.com:65536/s"));
+ ASSERT_EQ(-1, uri.port());
+ ASSERT_EQ("www.baidu.com", uri.host());
+
+ // A very long run of digits must not overflow the accumulator.
+ ASSERT_EQ(0,
uri.SetHttpURL("foo://www.baidu.com:999999999999999999999999/s"));
+ ASSERT_EQ(-1, uri.port());
+ ASSERT_EQ("www.baidu.com", uri.host());
Review Comment:
The regression test covers a long run of non-zero digits, but it doesn't
cover the remaining overflow/wrap risk with long runs of leading zeros (e.g.,
`:1` followed by 64 zeros) which can still wrap internal arithmetic and be
mis-parsed as an in-range port. Adding a test for that case would prevent
regressions once the parser is fixed.
--
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]
