sahvx655-wq opened a new pull request, #3591:
URL: https://github.com/apache/brpc/pull/3591
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
`ConvertGrpcTimeoutToUS` (`src/brpc/grpc.cpp`) parses the `grpc-timeout`
request header with `strtol` and then multiplies the result by its unit. The
only validation is that the digit span equals the header length minus one, so
a value of any magnitude gets through and every unit branch overflows
`int64_t`: `18446744073710S` wraps `timeout_value * 1000000` to 448384,
`2562047789H` wraps `timeout_value * 3600 * 1000000` negative, and
`9223372036854775807n` overflows `timeout_value + 500`.
`9223372036854775807u` is returned unchanged, so the addition at
`src/brpc/policy/http_rpc_protocol.cpp:1707` overflows instead. Compiling the
function with `-fsanitize=undefined` aborts on the `S` case: `runtime error:
signed integer overflow: 18446744073710 * 1000000 cannot be represented in
type 'int64_t'`.
Signed overflow is undefined behaviour, and with recovery on the wrapped
values are observable. A peer that asks for an absurdly long deadline instead
gets one 448384us away, or one far in the past, and the service reads that
back through `Controller::deadline_us()` and acts on it. The header is parsed
before any service code runs, on any h2 port that serves gRPC, so nothing
about the input is privileged.
### What is changed and the side effects?
Changed:
- Reject a `grpc-timeout` value outside the range the gRPC wire format allows
(`TimeoutValue` is "a positive integer as ASCII string of at most 8
digits"), returning -1 the way the function already does for every other
malformed header. One bound covers all four arithmetic sites: 99999999
hours
in microseconds is 3.6e17, which still leaves room for the caller to add
the
current time.
- Extend the `GrpcTest.GrpcTimeOut` table with the overflowing values plus a
9-digit and a negative one.
Side effects:
- Performance effects: one comparison per gRPC request carrying the header.
- Breaking backward compatibility: in-spec timeouts behave exactly as before.
A value with more than 8 digits is now ignored instead of producing a
wrapped deadline. The only brpc client that can emit one is a caller with
`timeout_ms` above 100000000 (over 27 hours), and such a client still
enforces its own deadline. A negative value returns -1 rather than negative
microseconds, which the single call site already treated identically.
### Check List:
- Tests: `GrpcTest.GrpcTimeOut` fails on master for `18446744073710S`
(deadline 448384us away), `9223372036854775807u` (negative deadline) and
`100000000S`, and passes with the fix. All 8 cases of
`brpc_grpc_protocol_unittest` pass, along with
`brpc_http_rpc_protocol_unittest` (62), `brpc_http_message_unittest` (28)
and `brpc_h2_unsent_message_unittest` (5).
- Compilable: full cmake build with `-DBUILD_UNIT_TESTS=ON`.
--
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]
