fix(protocol): fix attachment size limit issue when size exceeds uint32 #3169

InJunee · 2025-12-09T08:36:06Z

Change attachment_size from int32 to int64 in protobuf definitions (baidu_rpc_meta.proto, rpc_dump.proto, nshead_meta.proto)
Fix type mismatches in ProcessRpcRequest and ProcessRpcResponse:
- Change req_size/res_size from int to size_t
- Change body_without_attachment_size from int to size_t
- Add overflow checks and proper type casting
Fix size inconsistency in PackRpcHeader and SerializeRpcHeaderAndMeta:
- Change payload_size parameter from int to size_t
- Add overflow check for meta_size + payload_size exceeding UINT32_MAX
Update format strings to use PRId64 and %zu for proper large value display

This fix allows the protocol to correctly handle attachment sizes greater than 4GB (uint32 max), preventing integer overflow and size mismatches.

Fixes #1348

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

Performance effects:
Breaking backward compatibility:

Check List:

Please make sure your changes are compilable.
When providing us with a new feature, it is best to add related tests.
Please follow Contributor Covenant Code of Conduct.

- Change attachment_size from int32 to int64 in protobuf definitions (baidu_rpc_meta.proto, rpc_dump.proto, nshead_meta.proto) - Fix type mismatches in ProcessRpcRequest and ProcessRpcResponse: - Change req_size/res_size from int to size_t - Change body_without_attachment_size from int to size_t - Add overflow checks and proper type casting - Fix size inconsistency in PackRpcHeader and SerializeRpcHeaderAndMeta: - Change payload_size parameter from int to size_t - Add overflow check for meta_size + payload_size exceeding UINT32_MAX - Update format strings to use PRId64 and %zu for proper large value display This fix allows the protocol to correctly handle attachment sizes greater than 4GB (uint32 max), preventing integer overflow and size mismatches. Fixes apache#1348

wwbmmm · 2025-12-09T13:01:23Z

src/brpc/policy/baidu_rpc_meta.proto

    optional int32 compress_type = 3;
    optional int64 correlation_id = 4;
-    optional int32 attachment_size = 5;
+    optional int64 attachment_size = 5;


You cannot directly change the type of an existing protobuf field. This will cause client and server of different version not compatible.
You could add a new field, like optional int64 attachment_size_long = 13.
If the attachment size not exceed int32, you can use the original attachment_size field for compatibility. If the attachment size exceed int32, you can use the attachment_size_long field.

wwbmmm · 2025-12-09T13:09:02Z

src/brpc/policy/baidu_rpc_protocol.cpp

+                   << " + payload_size=" << payload_size 
+                   << " = " << total_size 
+                   << ") exceeds uint32_t maximum (" << UINT32_MAX << ")";
+        // Truncate to maximum, but this will cause protocol error


You know this will cause protocol error, so what's the meaning of this code?
Maybe you can use pack32(UINT32_MAX) as a flag, then pack64(total_size) after it?
Then you can update the corresponding parsing code in ParseRpcMessage.

wwbmmm · 2025-12-09T13:13:16Z

src/brpc/nshead_meta.proto

    optional int64 correlation_id = 2;
    optional int64 log_id = 3;
-    optional int32 attachment_size = 4;
+    optional int64 attachment_size = 4;


No need to change nshead protocol.

…ility - Add attachment_size_long (int64) field to support sizes > INT32_MAX - Keep attachment_size (int32) for backward compatibility - Implement helper functions for reading/writing attachment sizes - Extend header format: use UINT32_MAX as flag for 64-bit body_size - Update ParseRpcMessage to support both normal (12-byte) and extended (20-byte) header formats - Fix rpc_replay tool to handle new attachment_size fields - Remove unused variables and redundant checks This fix allows the protocol to correctly handle attachment sizes greater than 4GB while maintaining full backward compatibility with existing clients and servers. Fixes apache#1348

InJunee · 2025-12-09T15:31:41Z

I have made modifications to the code according to your comments. Please check it. @wwbmmm

wwbmmm · 2025-12-10T02:37:38Z

tools/rpc_replay/rpc_replay.cpp

+                } else {
+                    req.serialized_data() = sample->request.movable();
+                }
            } else {


This else is never reached

wwbmmm · 2025-12-10T02:40:49Z

Please fix the CI failure, there are some compilation errors.

- Change %d to %zu for request_size and response_size - Use standard INT32_MAX instead of custom constant - Fix response_size format string (was incorrectly using request_size)

Remove the unreachable else branch that was never executed. The logic is already handled in the inner if-else block.

InJunee · 2025-12-10T09:10:48Z

I have made modifications to the code. Please check it. @wwbmmm

Copilot

Pull request overview

This PR fixes attachment size handling in the brpc protocol to support sizes exceeding uint32 limits (>4GB). The changes introduce backward-compatible int64 fields in protobuf schemas and implement an extended header format for large message bodies.

Key Changes

Added attachment_size_long (int64) fields to RpcMeta and RpcDumpMeta with backward compatibility for existing attachment_size (int32) fields
Implemented extended 20-byte header format for messages with body_size > UINT32_MAX, while maintaining the standard 12-byte format for smaller messages
Updated type handling from int to size_t for size calculations and added proper PRId64/%zu format specifiers

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/brpc/policy/baidu_rpc_meta.proto	Adds attachment_size_long field for sizes > INT32_MAX while preserving original field for backward compatibility
src/brpc/rpc_dump.proto	Adds attachment_size_long field with same backward compatibility approach
src/brpc/nshead_meta.proto	Changes attachment_size from int32 to int64 (breaking change)
src/brpc/policy/baidu_rpc_protocol.cpp	Implements helper functions for backward-compatible attachment size handling, extended header format, type conversions, and updated format strings
tools/rpc_replay/rpc_replay.cpp	Adds backward-compatible attachment size reading logic for replay functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-10T11:07:16Z

src/brpc/policy/baidu_rpc_protocol.cpp


            butil::IOBuf req_buf;
-            int body_without_attachment_size = req_size - meta.attachment_size();
+            const int64_t attachment_size = GetAttachmentSize(meta);


Redundant call to GetAttachmentSize(meta). The attachment_size was already retrieved and validated at lines 772-780. Consider reusing the same variable to avoid duplicate calls and improve code clarity.

Suggested change

const int64_t attachment_size = GetAttachmentSize(meta);

// attachment_size already retrieved and validated earlier

Copilot · 2025-12-10T11:07:17Z

src/brpc/policy/baidu_rpc_protocol.cpp

+        if (attachment_size > 0) {
+            if (static_cast<size_t>(attachment_size) > req_size) {


Missing validation for negative attachment_size values. The GetAttachmentSize() function returns int64_t which could be negative (either from malicious input or bugs). When a negative int64_t is cast to size_t at line 774, it becomes a very large unsigned value that would correctly fail the > req_size check. However, the condition should explicitly check attachment_size < 0 before the cast to make the validation logic clearer and more robust.

Consider adding: if (attachment_size < 0 || static_cast<size_t>(attachment_size) > req_size) {

Copilot · 2025-12-10T11:07:17Z

src/brpc/policy/baidu_rpc_protocol.cpp

+        if (attachment_size > 0) {
+            if (static_cast<size_t>(attachment_size) > res_size) {


Missing validation for negative attachment_size values. The GetAttachmentSize() function returns int64_t which could be negative. When a negative int64_t is cast to size_t at line 1065, it becomes a very large unsigned value that would correctly fail the > res_size check. However, the condition should explicitly check attachment_size < 0 before the cast to make the validation logic clearer and more robust.

Consider adding: if (attachment_size < 0 || static_cast<size_t>(attachment_size) > res_size) {

Copilot · 2025-12-10T11:07:17Z

tools/rpc_replay/rpc_replay.cpp

+                if (attachment_size > 0 && 
+                    static_cast<size_t>(attachment_size) < sample->request.size()) {


Missing validation for negative attachment_size values. When attachment_size is negative (from malicious or corrupted dump files), casting it to size_t at line 193 will produce a very large unsigned value. While this would fail the < sample->request.size() check, an explicit negative check would make the validation clearer.

Consider adding: if (attachment_size > 0 && attachment_size <= static_cast<int64_t>(sample->request.size())) { to explicitly validate the range before the cast.

Copilot · 2025-12-10T11:07:17Z

src/brpc/nshead_meta.proto

    optional int64 correlation_id = 2;
    optional int64 log_id = 3;
-    optional int32 attachment_size = 4;
+    optional int64 attachment_size = 4;


This protobuf field change from int32 to int64 is a breaking change that breaks backward compatibility. Unlike baidu_rpc_meta.proto and rpc_dump.proto which preserve the old int32 field and add a new int64 field for compatibility, this directly changes the field type. This will cause issues when old clients/servers communicate with new ones, as they expect different wire formats for field 4.

Consider adding a new field attachment_size_long (similar to the pattern in other proto files) and keeping the original attachment_size as int32 for backward compatibility.

Suggested change

optional int64 attachment_size = 4;

// For backward compatibility, keep attachment_size as int32.

optional int32 attachment_size = 4;

// Use attachment_size_long for large attachments.

optional int64 attachment_size_long = 10;

Copilot · 2025-12-10T11:07:18Z

src/brpc/policy/baidu_rpc_protocol.cpp

            }

            messages = BaiduProxyPBMessages::Get();
+            const int64_t attachment_size = GetAttachmentSize(meta);


Redundant call to GetAttachmentSize(meta). The attachment_size was already retrieved at line 772 and validated at lines 773-780. Consider reusing the same variable to avoid duplicate calls and improve code clarity.

Suggested change

const int64_t attachment_size = GetAttachmentSize(meta);

// Use the previously retrieved attachment_size variable here.

Remove duplicate calls to GetAttachmentSize(meta) at lines 819 and 889. The attachment_size is already retrieved and validated at line 772, so we can reuse the same variable to avoid redundant calls and improve code clarity.

- Add explicit negative value validation for attachment_size before casting to size_t - Remove redundant GetAttachmentSize calls in ProcessRpcRequest - Mark GetAttachmentSizeFromDump as unused to avoid compilation warnings - Improve validation logic in rpc_replay to validate before casting - Revert nshead_meta.proto attachment_size to int32 as per maintainer feedback Fixes issue apache#1348

InJunee · 2025-12-11T08:12:11Z

I have made modifications to the code. Please check it. @wwbmmm

wwbmmm · 2025-12-11T12:21:35Z

LGTM

chenBright

Please add some UTs for Extended format, for example, in the brpc_server_unittest.cpp file.

src/brpc/policy/baidu_rpc_protocol.cpp

- add extended header parse UT in brpc_server_unittest.cpp - switch unused marker to ALLOW_UNUSED for GetAttachmentSizeFromDump - include needed headers for packing extended header Fixes issue apache#1348

InJunee · 2025-12-15T11:01:05Z

@chenBright I have made some modifications to the code. Please have a look. Please let me know where the code still needs to be modified so that the code can be merged.

chenBright

LGTM

InJunee · 2025-12-18T11:23:53Z

It's a great honor to receive your approval. May I ask when my PR can be merged? This is of great significance to me. @chenBright

chenBright · 2025-12-18T12:40:09Z

src/brpc/policy/baidu_rpc_protocol.cpp

+// 1. Header format:
+//    - Normal format (12 bytes): [PRPC][body_size(32bit)][meta_size(32bit)]
+//    - Extended format (20 bytes): [PRPC][UINT32_MAX][meta_size(32bit)][body_size(64bit)]
+//      Extended format is used when body_size > UINT32_MAX


If body_size equals UINT32_MAX, the client uses the normal format, while the server uses the extended format.

Normal format (12 bytes): [PRPC][body_size(32bit)][meta_size(32bit)]

Extended format (16 bytes): [LRPC][body_size(64bit)][meta_size(32bit)] , L represents long

The two formats have the same structure, only the magic number and body size differ. This makes them simpler to process.

@wwbmmm What do you think of the approach?

Is there anything else that needs to be modified in the code?

I think there's nothing else that needs to be changed.

wwbmmm reviewed Dec 9, 2025

View reviewed changes

wwbmmm reviewed Dec 10, 2025

View reviewed changes

InJune added 2 commits December 10, 2025 17:03

fix: correct format specifiers for size_t values

7ec37a0

- Change %d to %zu for request_size and response_size - Use standard INT32_MAX instead of custom constant - Fix response_size format string (was incorrectly using request_size)

fix: remove unreachable else branch in rpc_replay

5b5b623

Remove the unreachable else branch that was never executed. The logic is already handled in the inner if-else block.

wwbmmm requested a review from Copilot December 10, 2025 11:02

Copilot started reviewing on behalf of wwbmmm December 10, 2025 11:03 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

InJune added 2 commits December 11, 2025 08:45

refactor: remove redundant GetAttachmentSize calls

e27ed16

Remove duplicate calls to GetAttachmentSize(meta) at lines 819 and 889. The attachment_size is already retrieved and validated at line 772, so we can reuse the same variable to avoid redundant calls and improve code clarity.

InJunee requested a review from wwbmmm December 11, 2025 01:30

chenBright reviewed Dec 15, 2025

View reviewed changes

src/brpc/policy/baidu_rpc_protocol.cpp Outdated Show resolved Hide resolved

test: cover baidu extended header and fix unused macro

e1fdd37

- add extended header parse UT in brpc_server_unittest.cpp - switch unused marker to ALLOW_UNUSED for GetAttachmentSizeFromDump - include needed headers for packing extended header Fixes issue apache#1348

chenBright approved these changes Dec 16, 2025

View reviewed changes

chenBright reviewed Dec 18, 2025

View reviewed changes

	const int64_t attachment_size = GetAttachmentSize(meta);
	// attachment_size already retrieved and validated earlier

		if (attachment_size > 0) {
		if (static_cast<size_t>(attachment_size) > req_size) {

		if (attachment_size > 0 &&
		static_cast<size_t>(attachment_size) < sample->request.size()) {

-    optional int64 attachment_size = 4;
+    // For backward compatibility, keep attachment_size as int32.
+    optional int32 attachment_size = 4;
+    // Use attachment_size_long for large attachments.
+    optional int64 attachment_size_long = 10;

	const int64_t attachment_size = GetAttachmentSize(meta);
	// Use the previously retrieved attachment_size variable here.

fix(protocol): fix attachment size limit issue when size exceeds uint32 #3169

Are you sure you want to change the base?

fix(protocol): fix attachment size limit issue when size exceeds uint32 #3169

Uh oh!

Conversation

InJunee commented Dec 9, 2025

What problem does this PR solve?

What is changed and the side effects?

Check List:

Uh oh!

wwbmmm Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

wwbmmm Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

wwbmmm Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

InJunee commented Dec 9, 2025

Uh oh!

wwbmmm Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

wwbmmm commented Dec 10, 2025

Uh oh!

InJunee commented Dec 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

InJunee commented Dec 11, 2025

Uh oh!

wwbmmm commented Dec 11, 2025

Uh oh!

chenBright left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

InJunee commented Dec 15, 2025

Uh oh!

chenBright left a comment

Choose a reason for hiding this comment

Uh oh!

InJunee commented Dec 18, 2025

Uh oh!

chenBright Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenBright Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

InJunee Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

chenBright Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

chenBright Dec 18, 2025 •

edited

Loading