ubeddulla commented on code in PR #3434:
URL: https://github.com/apache/brpc/pull/3434#discussion_r3789559208
##########
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:
Good catch. I dropped the backward accumulate-with-multiplier entirely and
now parse the port forward from the colon, clamping to -1 as soon as it passes
65535. So a long run of leading zeros can't wrap the multiplier anymore, and
":1" followed by 64 zeros correctly comes out as -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:
Added a leading-zeros case (a 1 followed by a long run of zeros, which stays
0 for many digits) plus an in-range ":00080" to confirm valid values with
leading zeros still parse.
--
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]
