[sahvx655-wq]

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.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open issues

• The new error string collapses two distinct failure modes (negative vs larger-than-payload) into a… · New
• meta.attachment_size() is evaluated multiple times in the condition and again in the error… · New

What changed in this PR

Tightens baidu_std request/response parsing validation to reject negative RpcMeta.attachment_size, preventing signed overflow/UB and payload/body desynchronization.

Changes:

• Add attachment_size < 0 guards in both request and response parsing paths.
• Update failure messages to reflect invalid attachment_size values.

File	Description
src/​brpc/​policy/​baidu_rpc_protocol.cpp	Adds lower-bound validation for attachment_size in request/response parsing and updates related SetFailed messages.

_{Review effort: Lite}

[wwbmmm]

[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.

[sahvx655-wq]

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.

[Copilot]

🟡 Changes recommended

The new tests leave socket creation references unreleased, leaking socket resources and request-side file descriptors.

4 open issues

• Release socket creation reference in response-test iterations · New
• Release socket and descriptor creation references in request tests · New
• The new error string collapses two distinct failure modes (negative vs larger-than-payload) into a…
• meta.attachment_size() is evaluated multiple times in the condition and again in the error…

_{Review effort: Balanced}