sahvx655-wq commented on code in PR #3591:
URL: https://github.com/apache/brpc/pull/3591#discussion_r4227181495
##########
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:
Reworded the quote so it no longer says *positive* - the bound here is only
about the 8-digit limit.
Zero is left going through on purpose. `grpc-timeout: 0S` returns 0 today
and the call site in `ProcessHttpRequest` turns that into `gettimeofday_us() +
0`, i.e. an already-expired deadline, which is the honest reading of the
header. Making the check `<= 0` would return -1, and -1 means no deadline at
all at that call site, so a peer asking for an immediate deadline would get an
unbounded one instead. That is a behaviour change in the wrong direction and
unrelated to the overflow this PR fixes.
##########
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:
Same point as the thread on the constant, answered there. Short version: the
comment no longer claims the format requires a positive integer, and the check
still accepts 0 deliberately, because 0 maps to an already-expired deadline
while -1 means no deadline at all.
--
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]
