Copilot commented on code in PR #3591:
URL: https://github.com/apache/brpc/pull/3591#discussion_r4218890056
##########
src/brpc/grpc.cpp:
##########
@@ -169,6 +169,11 @@ void PercentDecode(const std::string& str, std::string*
str_out) {
}
}
+// The gRPC wire format defines TimeoutValue as "a positive integer as ASCII
+// string of at most 8 digits", so this is the largest value a conforming peer
+// can send. See doc/PROTOCOL-HTTP2.md in grpc/grpc.
+static const int64_t MAX_GRPC_TIMEOUT_VALUE = 99999999;
Review Comment:
The comment above states the wire format requires a **positive** integer,
but the current check allows `timeout_value == 0`. If you want strict
conformance (and to avoid treating `grpc-timeout: 0S` as an immediate
deadline), update the validation to reject zero (e.g., `timeout_value <= 0`).
If zero is intentionally supported for backward-compatibility, the comment
should be adjusted to avoid claiming “positive” while accepting 0.
##########
src/brpc/grpc.cpp:
##########
@@ -169,6 +169,11 @@ void PercentDecode(const std::string& str, std::string*
str_out) {
}
}
+// The gRPC wire format defines TimeoutValue as "a positive integer as ASCII
+// string of at most 8 digits", so this is the largest value a conforming peer
+// can send. See doc/PROTOCOL-HTTP2.md in grpc/grpc.
+static const int64_t MAX_GRPC_TIMEOUT_VALUE = 99999999;
Review Comment:
Consider making this a compile-time constant (e.g., `constexpr int64_t ...`)
and/or placing it in an unnamed namespace (or using a `k`-style constant if
that matches local conventions). This prevents accidental ODR-use/linkage
surprises and communicates intent more clearly.
##########
src/brpc/grpc.cpp:
##########
@@ -186,6 +191,13 @@ int64_t ConvertGrpcTimeoutToUS(const std::string*
grpc_timeout) {
if ((size_t)(endptr - grpc_timeout->data()) != grpc_timeout->size() - 1) {
return -1;
}
+ // A value outside this range is not a valid timeout, and a large one makes
+ // the conversions below overflow int64_t, so treat it as no deadline like
+ // any other malformed value. strtol() saturates at LONG_MAX/LONG_MIN for
+ // input that does not fit, which is rejected here as well.
+ if (timeout_value < 0 || timeout_value > MAX_GRPC_TIMEOUT_VALUE) {
Review Comment:
The comment above states the wire format requires a **positive** integer,
but the current check allows `timeout_value == 0`. If you want strict
conformance (and to avoid treating `grpc-timeout: 0S` as an immediate
deadline), update the validation to reject zero (e.g., `timeout_value <= 0`).
If zero is intentionally supported for backward-compatibility, the comment
should be adjusted to avoid claiming “positive” while accepting 0.
##########
test/brpc_grpc_protocol_unittest.cpp:
##########
@@ -233,7 +233,15 @@ TEST_F(GrpcTest, GrpcTimeOut) {
"HHHH", "-1",
"112", "-1",
"H999m", "-1",
- "", "-1"
+ "", "-1",
+ // beyond the 8 digits the gRPC wire format allows; the large ones
+ // overflow the conversion to microseconds
+ "100000000S", "-1",
+ "18446744073710S", "-1",
+ "2562047789H", "-1",
+ "9223372036854775807u", "-1",
+ "9223372036854775807n", "-1",
+ "-1S", "-1"
Review Comment:
The comment describes cases “beyond 8 digits” / “large ones overflow”, but
the list also includes `-1S` (negative, not an 8+ digit value). Consider
splitting this into two comment blocks (oversized vs negative) or rewording to
cover both (e.g., “invalid: >8 digits or negative; previously could overflow
conversion”).
--
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]
