You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "sahvx655-wq (via GitHub)" <gi...@apache.org> on 2026/10/13 16:03:51 UTC
[PR] reject negative attachment_size in baidu_std request/response parsing (brpc)
sahvx655-wq opened a new pull request, #3604:
URL: https://github.com/apache/brpc/pull/3604
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
`RpcMeta.attachment_size` is `optional int32` (baidu_rpc_meta.proto) and is copied from the peer. In `ProcessRpcRequest` and `ProcessRpcResponse` (src/brpc/policy/baidu_rpc_protocol.cpp) it is validated only against an upper bound (`req_size < attachment_size`, `attachment_size > res_size`). A negative value passes both guards and reaches
```
int body_without_attachment_size = res_size - meta.attachment_size();
```
With `attachment_size = INT32_MIN` and a small body (an empty body is a valid baidu_std message), `res_size - attachment_size` overflows signed `int`, which is undefined behavior; the UBSan job in CI aborts on it. For other negative values the subtraction yields a size larger than the payload, so the body/attachment split silently desyncs.
Reproduced the arithmetic with the real types under `-fsanitize=undefined`: `attachment_size=INT32_MIN`, `req_size=0` gives `runtime error: signed integer overflow: 0 - -2147483648 cannot be represented in type 'int'` and aborts; with the negative value rejected it returns cleanly.
Siblings checked: hulu uses `cutn(user_message_size)` (bounded by IOBuf, no subtraction) and sofa has no attachment, so neither shares the pattern. Both affected sites are in this file.
### What is changed and the side effects?
Changed:
- Reject `attachment_size < 0` at the request and response guards, so the later `int` subtraction always has a non-negative operand.
Side effects:
- Performance effects: none.
- Breaking backward compatibility: none. A valid `attachment_size` is a byte count and is always `>= 0`; only malformed negative values are now rejected with the existing `SetFailed` path.
---
### Check List:
- Changes are compilable; the diff only tightens an existing condition.
- Verified with a UBSan reproducer of the exact subtraction (before: abort on `INT32_MIN`; after: clean reject). No committed unit test since this parser path needs a full server/client round-trip to exercise and I could not build the suite in my environment; happy to add one if you prefer.
- Follows the Code of Conduct.
This fix was found and prepared with AI assistance and reviewed by me before submitting.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject negative attachment_size in baidu_std request/response parsing (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3604:
URL: https://github.com/apache/brpc/pull/3604#discussion_r4332672028
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -709,9 +709,10 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
const int req_size = static_cast<int>(msg->payload.size());
if (meta.has_attachment_size()) {
- if (req_size < meta.attachment_size()) {
+ if (meta.attachment_size() < 0 ||
+ req_size < meta.attachment_size()) {
cntl->SetFailed(EREQUEST,
- "attachment_size=%d is larger than request_size=%d",
+ "Invalid attachment_size=%d (request_size=%d)",
Review Comment:
The new error string collapses two distinct failure modes (negative vs larger-than-payload) into a generic “Invalid”, which makes diagnosing peer/protocol issues harder. Consider using separate messages (or include the reason) so logs clearly indicate whether the value was negative or exceeded the payload size.
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -1018,9 +1019,10 @@ void ProcessRpcResponse(InputMessageBase* msg_base) {
const int res_size = msg->payload.length();
butil::IOBuf* res_buf_ptr = &msg->payload;
if (meta.has_attachment_size()) {
- if (meta.attachment_size() > res_size) {
+ if (meta.attachment_size() < 0 ||
+ meta.attachment_size() > res_size) {
cntl->SetFailed(
- ERESPONSE, "attachment_size=%d is larger than response_size=%d",
+ ERESPONSE, "Invalid attachment_size=%d (response_size=%d)",
meta.attachment_size(), res_size);
Review Comment:
`meta.attachment_size()` is evaluated multiple times in the condition and again in the error message. Capturing it once into a local (e.g., `const int32_t attachment_size = meta.attachment_size();`) improves readability and ensures the same value is used consistently across checks and logging.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject negative attachment_size in baidu_std request/response parsing (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject negative attachment_size in baidu_std request/response parsing (brpc)
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject negative attachment_size in baidu_std request/response parsing (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3604:
URL: https://github.com/apache/brpc/pull/3604#issuecomment-6360732521
[replied by brpc-oncall robot] The negative-value guards correctly close the signed-overflow/desync at both the request and response sites, and no compatibility concern for well-formed messages. One request before merge: add a regression test. The claim that this path needs a full round trip is not accurate: test/brpc_channel_unittest.cpp already constructs a policy::MostCommonMessage and calls brpc::policy::ProcessRpcResponse directly (see ResponseSocketTest), so a case setting a negative attachment_size can assert ERESPONSE and that no crash occurs. Please add such a case (and, if feasible, a request-side one) so this guard does not silently regress. The split error messages suggested by Copilot are not necessary: the updated message already logs both the offending value and the payload size, which is enough to distinguish the two cases.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] reject negative attachment_size in baidu_std request/response parsing (brpc)
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on PR #3604:
URL: https://github.com/apache/brpc/pull/3604#issuecomment-6381361554
Fair point on the round trip, I'd missed that `ResponseSocketTest` already drives `ProcessRpcResponse` directly. Pushed two cases in `test/brpc_channel_unittest.cpp` under `BaiduAttachmentSizeTest`, each run with `INT32_MIN` and `-1`. The response one follows the `ResponseSocketTest` setup (versioned call id, sending socket bound to the controller) and asserts `ERESPONSE` with the response message and attachment left empty. The request one starts a `Server` on an ephemeral port, hands `ProcessRpcRequest` a message whose socket is one end of a socketpair with the server as `arg`, then reads the baidu_std reply back off the other end and checks the `RpcMeta` carries `EREQUEST` with no body after the meta.
I built the suite locally against the unpatched parser first to make sure the cases actually bite: both paths served the payload with `error_code=0`, because the oversized length computed from the negative value just makes `cutn` swallow the whole payload and the body is delivered as if the attachment_size were sane. With the guard in place both report the expected error. The rest of `brpc_channel_unittest` is unaffected; the only failure in my run is `parse_hostname`, which needs the CA file and live DNS and is unrelated. Left the error message as is, since as you say it already prints both the value and the payload size.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org