sahvx655-wq commented on code in PR #3604:
URL: https://github.com/apache/brpc/pull/3604#discussion_r4374472443
##########
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:
Right, `Create` hands back a creation reference that nothing in the test
ever drops, and the controller only fails `sending_sock` for short/pooled
connections, so each iteration left a socket behind. Now calling
`ReleaseAdditionalReference()` straight after the controller takes its
reference, so the socket is recycled once the controller and the message let
go. Fixed in 320f5f5.
##########
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:
Same fix here in 320f5f5: the creation reference is released right after
`Address`, so the `SocketUniquePtr` and the in-flight message are the only
owners and the fd closes at the end of each iteration, including on an early
`ASSERT` bail-out. Releasing the additional reference is safe even if the error
path fails the socket, since `ReleaseAdditionalReference` only drops it once. I
haven't rebuilt the suite since this push, so CI is what confirms it here.
--
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]
