sahvx655-wq opened a new pull request, #3642: URL: https://github.com/apache/brpc/pull/3642
### What problem does this PR solve? Issue Number: none Problem Summary: `butil::str2endpoint(const char*, EndPoint*)` and `butil::hostname2endpoint(const char*, EndPoint*)` write the parsed ip and the `strtol` port into the caller's `EndPoint` before the 0-65535 range check, and leave them there when the check fails. Port `123456789` is `EXTENDED_ENDPOINT_PORT`, and `ExtendedEndPoint::is_extended()` only compares the port, so `"<ip>:123456789"` fails to parse but leaves an `EndPoint` that claims to be the extended endpoint whose ResourcePool id is `<ip>`. The next `reset()` (assignment or destructor) then calls `dec_ref()` on that live `ExtendedEndPoint`. This is reachable from the wire when `-http_header_of_user_ip` is set (off by default, documented for servers behind a proxy). `GetUserAddressFromHeaderImpl()` in `http_rpc_protocol.cpp` passes the header value to `str2endpoint()`, and both `ProcessHttpRequest()` and the auth path then assign `socket->remote_side()` over the failed result. Each request carrying e.g. `X-User-IP: 0.0.0.0:123456789` drops one reference from slot 0, which is the first unix or IPv6 endpoint the process created (a listen address, an IPv6 peer). Once the count reaches zero the slot goes back to the pool while its owners still hold it. A standalone reproducer of that caller pattern on master: ``` listen_addr = unix:/tmp/srv.sock X-User-IP header = 0.0.0.0:123456789 new peer = [2001:db8::1]:443 listen_addr now = [2001:db8::1]:443 F... extended_endpoint.hpp:272 dec_ref] Check failed: old_ref >= 1. ExtendedEndPoint has unexpected reference 0 ``` `hostname2endpoint()` has the same write-before-validate and is the `str2endpoint()` fallback in the list/file/remote-file naming services, so it gets the same fix. Both functions also narrowed the `strtol` result to `int` before the range check, so `127.0.0.1:4294967377` was accepted as port 81; keeping it in a `long` closes that too. ### What is changed and the side effects? Changed: - `str2endpoint()` and `hostname2endpoint()` parse into locals and assign `*point` only on success. - New `EndPointTest.failed_parse_leaves_endpoint_untouched`: fails on master (the forged endpoint is extended, the live endpoint's refcount drops and the `dec_ref` check above fires) and passes with the fix. Tested on macOS with CMake: full build plus `cmake_link_checks`, `test_butil` (`StackTraceTest.find_symbol` fails here on master too, everything else passes), `brpc_uri_unittest`, `brpc_http_rpc_protocol_unittest`, `brpc_naming_service_unittest`, `brpc_naming_service_filter_unittest`. Side effects: - Performance effects: none. - Breaking backward compatibility: a failed parse no longer modifies the output `EndPoint`. Every in-tree caller checks the return value and either falls back to another address or fails, so none depends on the partial result. --- ### Check List: - Please make sure your changes are compilable. - When providing us with a new feature, it is best to add related tests. - Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md). -- 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]
