bound grpc-timeout value before converting to microseconds

[sahvx655-wq]

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.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings

• The comment above states the wire format requires a positive integer, but the current check… · New
• The comment above states the wire format requires a positive integer, but the current check… · New
• Consider making this a compile-time constant (e.g., constexpr int64_t ...) and/or placing it in… · New
• The comment describes cases “beyond 8 digits” / “large ones overflow”, but the list also includes… · New

What changed in this PR

This PR hardens gRPC grpc-timeout parsing to prevent int64_t overflow/UB by bounding the parsed timeout value to the range allowed by the gRPC wire format, and extends unit tests to cover previously overflowing inputs.

Changes:

• Add an upper bound check (and malformed-value handling) in ConvertGrpcTimeoutToUS before converting to microseconds.
• Introduce a constant representing the maximum conforming TimeoutValue (8 ASCII digits).
• Expand GrpcTest.GrpcTimeOut test vectors with overflow-sized and invalid values.

File	Description
src/​brpc/​grpc.cpp	Adds a max timeout bound to prevent overflow during unit conversion.
test/​brpc_grpc_protocol_unittest.cpp	Adds regression cases for large/invalid grpc-timeout headers.

🧠 Review effort: Lite

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[wwbmmm]

LGTM

[sahvx655-wq]

Pushed a comment-only follow-up for the two bot findings that were actually right. The constant's comment no longer quotes the wire format as requiring a positive integer, because the check deliberately lets 0 through, and the test comment now mentions the negative entry alongside the oversized ones. Nothing else moved: the diff is two comment blocks, so the bound and the six new table entries behave exactly as they did when CI went green.

I did not take the other two. Rejecting 0 would be a behaviour change in the wrong direction - 0S currently yields a deadline of gettimeofday_us() + 0, already expired, whereas -1 means no deadline at all at that call site, so a peer asking for an immediate deadline would end up unbounded. And static const at file scope is what the other ~20 constants under src/brpc use, with static already giving internal linkage, so there is no ODR hazard to fix by switching this one site to constexpr. Re-checked the patched conversion standalone under -fsanitize=undefined -fno-sanitize-recover=all: all five overflow inputs return -1, 99999999H tops out at 3.6e17, and 1H/4m/5u/6n/0S are unchanged.

[Copilot]

🟢 Approval recommended

The bound safely covers all unit conversions and the regression cases exercise the affected behavior.

0 open findings

4 resolved since last review

• The comment above states the wire format requires a positive integer, but the current check…
• The comment above states the wire format requires a positive integer, but the current check…
• The comment describes cases “beyond 8 digits” / “large ones overflow”, but the list also includes…
• Consider making this a compile-time constant (e.g., constexpr int64_t ...) and/or placing it in…

🧠 Review effort: Balanced