Copilot commented on code in PR #3169:
URL: https://github.com/apache/brpc/pull/3169#discussion_r2606216733
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -792,9 +886,10 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
}
butil::IOBuf req_buf;
- int body_without_attachment_size = req_size -
meta.attachment_size();
+ const int64_t attachment_size = GetAttachmentSize(meta);
Review Comment:
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.
```suggestion
// attachment_size already retrieved and validated earlier
```
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -723,9 +816,10 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
}
messages = BaiduProxyPBMessages::Get();
+ const int64_t attachment_size = GetAttachmentSize(meta);
Review Comment:
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.
```suggestion
// Use the previously retrieved attachment_size variable here.
```
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -676,12 +768,13 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
break;
}
- const int req_size = static_cast<int>(msg->payload.size());
- if (meta.has_attachment_size()) {
- if (req_size < meta.attachment_size()) {
+ const size_t req_size = msg->payload.size();
+ const int64_t attachment_size = GetAttachmentSize(meta);
+ if (attachment_size > 0) {
+ if (static_cast<size_t>(attachment_size) > req_size) {
Review Comment:
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) {`
##########
tools/rpc_replay/rpc_replay.cpp:
##########
@@ -181,13 +181,23 @@ static void* replay_thread(void* arg) {
memcpy(&nshead_req.head, sample->meta.nshead().c_str(),
sample->meta.nshead().length());
nshead_req.body = sample->request;
req_ptr = &nshead_req;
- } else if (sample->meta.attachment_size() > 0) {
- sample->request.cutn(
- &req.serialized_data(),
- sample->request.size() - sample->meta.attachment_size());
- cntl->request_attachment() = sample->request.movable();
} else {
- req.serialized_data() = sample->request.movable();
+ // Get attachment size with backward compatibility
+ int64_t attachment_size = 0;
+ if (sample->meta.has_attachment_size_long()) {
+ attachment_size = sample->meta.attachment_size_long();
+ } else if (sample->meta.has_attachment_size()) {
+ attachment_size =
static_cast<int64_t>(sample->meta.attachment_size());
+ }
+ if (attachment_size > 0 &&
+ static_cast<size_t>(attachment_size) <
sample->request.size()) {
Review Comment:
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.
##########
src/brpc/nshead_meta.proto:
##########
@@ -30,7 +30,7 @@ message NsheadMeta {
optional int64 correlation_id = 2;
optional int64 log_id = 3;
- optional int32 attachment_size = 4;
+ optional int64 attachment_size = 4;
Review Comment:
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.
```suggestion
// 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;
```
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -963,16 +1058,17 @@ void ProcessRpcResponse(InputMessageBase* msg_base) {
}
// Parse response message iff error code from meta is 0
butil::IOBuf res_buf;
- const int res_size = msg->payload.length();
+ const size_t res_size = msg->payload.length();
butil::IOBuf* res_buf_ptr = &msg->payload;
- if (meta.has_attachment_size()) {
- if (meta.attachment_size() > res_size) {
+ const int64_t attachment_size = GetAttachmentSize(meta);
+ if (attachment_size > 0) {
+ if (static_cast<size_t>(attachment_size) > res_size) {
Review Comment:
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) {`
--
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]
