reject out-of-range port in SplitHostAndPort

[ubeddulla]

SplitHostAndPort parses the port from a URL or Host header by accumulating digits into a uint64 and narrowing the result with static_cast, with no range check, so a value above INT_MAX silently wraps to a valid-looking but wrong port (":4294967377" becomes 81) and a long run of digits overflows the accumulator. str2endpoint and hostname2endpoint already reject ports outside 0-65535, so this stops accumulating past the range and maps any out-of-range value to -1 to keep the URL parser consistent with them. The added regression test in brpc_uri_unittest.cpp covers the wraparound, the just-over-limit case, a very long digit run, and the Host header path.

[Copilot]

Pull request overview

This PR targets brpc::URI URL/Host parsing by hardening SplitHostAndPort so out-of-range ports don’t silently wrap into valid-looking values, aligning URI parsing behavior with other endpoint parsers in the codebase.

Changes:

• Adds a regression test covering port wraparound, just-over-limit ports, very long digit runs, and the Host-header parsing path.
• Updates SplitHostAndPort to stop accumulating and return -1 when the parsed port is out of the valid range (0–65535).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/brpc/uri.cpp	Updates port parsing in SplitHostAndPort to reject out-of-range ports instead of narrowing/wrapping.
test/brpc_uri_unittest.cpp	Adds regression coverage for out-of-range port parsing via both URL parsing and SetHostAndPort.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ubeddulla]

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.

[ubeddulla]

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.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/brpc/uri.cpp:85

• SplitHostAndPort does host_end - 1 unconditionally, which is undefined behavior when called with an empty host range (e.g. URLs like foo:///path can make start == p, and SetHostAndPort("") is also possible). Add an early guard for host_begin >= host_end before doing pointer arithmetic.

                                    int* port) {
    for (const char* q = host_end - 1; q > host_begin; --q) {

src/brpc/uri.cpp:104

• When the host ends with a colon (e.g. "example.com:"), the inner loop runs zero iterations and port_raw stays 0, so the parsed port becomes 0 rather than being treated as invalid/unset. This is inconsistent with str2endpoint/hostname2endpoint (they reject empty ports) and is unlikely to be what a URL/Host header intended.

        } else if (*q == ':') {
            // [q + 1, host_end) is all digits. Accumulate it forward and clamp
            // to -1 as soon as it leaves the valid range, so an out-of-range
            // port never narrows/wraps into a valid-looking wrong one (e.g.
            // ":4294967377" truncating to 81, or a long run of digits

test/brpc_uri_unittest.cpp:115

• The comment says this case is “padded with a long run of leading zeros”, but the current value starts with 1 and has only trailing zeros. Either update the wording or make the test value actually start with many 0s so it exercises the intended leading-zero scenario.

    // An out-of-range value padded with a long run of leading zeros must not
    // wrap the accumulator into a valid-looking port either.
    ASSERT_EQ(0, uri.SetHttpURL(
            "foo://www.baidu.com:1000000000000000000000000000000000000"
            "0000000000000000000000000000/s"));

[wwbmmm]

LGTM