Copilot commented on code in PR #3604:
URL: https://github.com/apache/brpc/pull/3604#discussion_r4366440018
##########
test/brpc_channel_unittest.cpp:
##########
@@ -2420,6 +2421,129 @@ TEST(ResponseSocketTest,
real_rpc_responses_match_sending_socket) {
server.Join();
}
+// RpcMeta.attachment_size is a peer-controlled int32. Before it was
+// lower-bounded, a negative value passed the "larger than payload" guard and
+// `payload_size - attachment_size` overflowed (INT32_MIN) or produced a bogus
+// body/attachment split (any other negative value).
+TEST(BaiduAttachmentSizeTest, negative_response_attachment_size_is_rejected) {
+ const int32_t attachment_sizes[] = { INT32_MIN, -1 };
+ for (int32_t attachment_size : attachment_sizes) {
+ SCOPED_TRACE(attachment_size);
+ brpc::Controller cntl;
+ test::EchoResponse res;
+ cntl._response = &res;
+ ASSERT_EQ(0, bthread_id_lock_and_reset_range(
+ cntl.call_id(), nullptr, 2));
+ ASSERT_EQ(0, bthread_id_unlock(cntl.current_id()));
+ brpc::SocketId sending_id;
+ ASSERT_EQ(0, brpc::Socket::Create(brpc::SocketOptions(), &sending_id));
+ ASSERT_EQ(0, brpc::Socket::Address(
+ sending_id, &cntl._current_call.sending_sock));
+ brpc::Socket* socket = cntl._current_call.sending_sock.get();
Review Comment:
Each response-test iteration leaks the socket's creation reference: the
controller's default `CONNECTION_TYPE_UNKNOWN` only releases `sending_sock` on
completion. Release the additional reference after acquiring the controller's
reference so the socket can be recycled when its remaining owners release it.
##########
test/brpc_channel_unittest.cpp:
##########
@@ -2420,6 +2421,129 @@ TEST(ResponseSocketTest,
real_rpc_responses_match_sending_socket) {
server.Join();
}
+// RpcMeta.attachment_size is a peer-controlled int32. Before it was
+// lower-bounded, a negative value passed the "larger than payload" guard and
+// `payload_size - attachment_size` overflowed (INT32_MIN) or produced a bogus
+// body/attachment split (any other negative value).
+TEST(BaiduAttachmentSizeTest, negative_response_attachment_size_is_rejected) {
+ const int32_t attachment_sizes[] = { INT32_MIN, -1 };
+ for (int32_t attachment_size : attachment_sizes) {
+ SCOPED_TRACE(attachment_size);
+ brpc::Controller cntl;
+ test::EchoResponse res;
+ cntl._response = &res;
+ ASSERT_EQ(0, bthread_id_lock_and_reset_range(
+ cntl.call_id(), nullptr, 2));
+ ASSERT_EQ(0, bthread_id_unlock(cntl.current_id()));
+ brpc::SocketId sending_id;
+ ASSERT_EQ(0, brpc::Socket::Create(brpc::SocketOptions(), &sending_id));
+ ASSERT_EQ(0, brpc::Socket::Address(
+ sending_id, &cntl._current_call.sending_sock));
+ brpc::Socket* socket = cntl._current_call.sending_sock.get();
+
+ brpc::policy::RpcMeta meta;
+ meta.set_correlation_id(cntl.current_id().value);
+ meta.mutable_response()->set_error_code(0);
+ meta.set_attachment_size(attachment_size);
+ auto* msg = brpc::policy::MostCommonMessage::Get();
+ butil::IOBufAsZeroCopyOutputStream meta_stream(&msg->meta);
+ ASSERT_TRUE(meta.SerializeToZeroCopyStream(&meta_stream));
+ test::EchoResponse response;
+ response.set_message("must not be delivered");
+ butil::IOBufAsZeroCopyOutputStream payload_stream(&msg->payload);
+ ASSERT_TRUE(response.SerializeToZeroCopyStream(&payload_stream));
+ socket->ReAddress(&msg->_socket);
+ socket->PostponeEOF();
+
+ brpc::policy::ProcessRpcResponse(msg);
+ EXPECT_EQ(brpc::ERESPONSE, cntl.ErrorCode()) << cntl.ErrorText();
+ EXPECT_TRUE(res.message().empty());
+ EXPECT_TRUE(cntl.response_attachment().empty());
+ }
+}
+
+TEST(BaiduAttachmentSizeTest, negative_request_attachment_size_is_rejected) {
+ MyEchoService service;
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&service, brpc::SERVER_DOESNT_OWN_SERVICE));
+ ASSERT_EQ(0, server.Start(0, nullptr));
+
+ const int32_t attachment_sizes[] = { INT32_MIN, -1 };
+ for (int32_t attachment_size : attachment_sizes) {
+ SCOPED_TRACE(attachment_size);
+ int fds[2];
+ ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM, 0, fds));
+ butil::fd_guard request_fd(fds[0]);
+ butil::fd_guard peer_fd(fds[1]);
+ brpc::SocketOptions options;
+ options.fd = request_fd;
+ brpc::SocketId socket_id;
+ ASSERT_EQ(0, brpc::Socket::Create(options, &socket_id));
+ request_fd.release();
+ brpc::SocketUniquePtr socket;
+ ASSERT_EQ(0, brpc::Socket::Address(socket_id, &socket));
Review Comment:
Each request-test iteration leaks a socket and its descriptor because
`SocketUniquePtr` releases only the addressed reference, leaving the creation
reference alive. This manually created socket is not owned by the server's
acceptor and has no read-event handler to detect peer closure. Release the
additional reference after `Address` so the remaining owners control its
lifetime, including on later assertion failures.
--
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]
