Copilot commented on code in PR #3580:
URL: https://github.com/apache/brpc/pull/3580#discussion_r4178359637
##########
src/brpc/details/controller_private_accessor.h:
##########
@@ -58,6 +58,24 @@ class ControllerPrivateAccessor {
return _cntl->_current_call.sending_sock.get();
}
+ bool does_response_match_request_socket(
+ CallId cid, SocketId response_socket_id) const {
+ const Controller::Call* call = nullptr;
+ if (cid == _cntl->_correlation_id || cid == _cntl->current_id()) {
+ call = &_cntl->_current_call;
+ } else if (_cntl->_unfinished_call != nullptr &&
+ cid == _cntl->get_id(_cntl->_unfinished_call->nretry)) {
+ call = _cntl->_unfinished_call;
+ } else {
+ return false;
+ }
+ // A regular client request always records the socket used to send it.
+ // Keep direct response injection available to internal tools and tests
+ // that do not issue a request through Controller::IssueRPC().
+ return call->sending_sock == nullptr ||
+ call->sending_sock->id() == response_socket_id;
Review Comment:
The unversioned base correlation ID is treated as the current attempt even
when a real request has populated `sending_sock`. `IssueRPC()` only sends
`current_id()` (`base + nretry + 1`), while the base ID is reserved for
timeout/cancel handling, so a response carrying the base ID on the current
socket can still mutate the response and complete the RPC despite not matching
any sent attempt. Restrict the base-ID compatibility path to direct injection
where no sending socket exists.
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -968,7 +968,16 @@ void ProcessRpcResponse(InputMessageBase* msg_base) {
}
return;
}
-
+
+ if (cntl == nullptr || !ControllerPrivateAccessor(cntl)
+ .does_response_match_request_socket(cid, msg->socket()->id())) {
+ LOG(WARNING) << "correlation_id=" << cid.value
+ << " of the response from " << *msg->socket()
+ << " does not match a request sent over it, drop it";
+ CHECK_EQ(0, bthread_id_unlock(cid));
+ return;
+ }
Review Comment:
A mismatched baidu_std response carrying stream settings is dropped without
resetting the advertised remote stream IDs. The adjacent lock-failure path
sends RST frames because otherwise those peer-side streams remain open; a
socket mismatch is also a rejected response and needs the same cleanup before
returning.
--
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]
