{{ message }}
Repository navigation
reject negative attachment_size in baidu_std request/response parsing - #3604
Open
sahvx655-wq wants to merge 1 commit into
Open
sahvx655-wq wants to merge 1 commit into
sahvx655-wq wants to merge 1 commit into
Conversation
attachment_size is a peer-controlled int32 but both guards only check an upper bound, so a negative value reaches res_size - attachment_size (int) and overflows for INT32_MIN. Reject attachment_size < 0 at both sites.
Contributor
There was a problem hiding this comment.
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
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 < 0guards in both request and response parsing paths. - Update failure messages to reflect invalid
attachment_sizevalues.
Review effort: Lite
Comment on lines
+712
to
+715
| 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)", |
Comment on lines
+1022
to
1026
Copilot stopped reviewing on behalf of
wwbmmm due to an error
October 21, 2026 03:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
RpcMeta.attachment_sizeisoptional int32(baidu_rpc_meta.proto) and is copied from the peer. InProcessRpcRequestandProcessRpcResponse(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 reachesWith
attachment_size = INT32_MINand a small body (an empty body is a valid baidu_std message),res_size - attachment_sizeoverflows signedint, 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=0givesruntime 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:
attachment_size < 0at the request and response guards, so the laterintsubtraction always has a non-negative operand.Side effects:
Performance effects: none.
Breaking backward compatibility: none. A valid
attachment_sizeis a byte count and is always>= 0; only malformed negative values are now rejected with the existingSetFailedpath.Check List:
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.This fix was found and prepared with AI assistance and reviewed by me before submitting.