Copilot commented on code in PR #3449:
URL: https://github.com/apache/brpc/pull/3449#discussion_r3788760939
##########
src/brpc/policy/sofa_pbrpc_protocol.cpp:
##########
@@ -189,11 +189,12 @@ ParseResult ParseSofaMessage(butil::IOBuf* source,
Socket* socket,
<< " + body_size=" << body_size;
return MakeParseError(PARSE_ERROR_TRY_OTHERS);
}
- if (body_size > FLAGS_max_body_size) {
- // We need this log to report the body_size to give users some clues
+ if (body_size > FLAGS_max_body_size ||
+ meta_size > FLAGS_max_body_size) {
+ // We need this log to report the size to give users some clues
// which is not printed in InputMessenger.
- LOG(ERROR) << "body_size=" << body_size << " from "
- << socket->remote_side() << " is too large";
+ LOG(ERROR) << "body_size=" << body_size << " meta_size=" << meta_size
Review Comment:
`FLAGS_max_body_size` is intended to cap the amount of data buffered for a
single message, but this check only limits `body_size` and `meta_size`
individually. Because the parser buffers `msg_size` bytes (`meta_size +
body_size`), a peer can still send a frame where both are <=
`FLAGS_max_body_size` but `msg_size` exceeds it (up to ~2x), defeating the
configured per-message limit.
##########
test/brpc_sofa_pbrpc_protocol_unittest.cpp:
##########
@@ -344,4 +364,26 @@ TEST_F(SofaTest, sofa_compress) {
TestSofaCompress(brpc::COMPRESS_TYPE_GZIP);
TestSofaCompress(brpc::COMPRESS_TYPE_ZLIB);
}
+
+TEST_F(SofaTest, reject_oversized_body) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_max_body_size = 1024;
+ const uint64_t body_size = 8 * 1024 * 1024;
+ butil::IOBuf buf;
+ AppendSofaTestHeader(&buf, 0, body_size, body_size);
+ brpc::ParseResult pr =
+ brpc::policy::ParseSofaMessage(&buf, _socket.get(), false, NULL);
+ ASSERT_EQ(brpc::PARSE_ERROR_TOO_BIG_DATA, pr.error());
+}
+
+TEST_F(SofaTest, reject_oversized_meta) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_max_body_size = 1024;
+ const uint32_t meta_size = 8 * 1024 * 1024;
+ butil::IOBuf buf;
+ AppendSofaTestHeader(&buf, meta_size, 0, meta_size);
+ brpc::ParseResult pr =
+ brpc::policy::ParseSofaMessage(&buf, _socket.get(), false, NULL);
+ ASSERT_EQ(brpc::PARSE_ERROR_TOO_BIG_DATA, pr.error());
+}
Review Comment:
The new oversized-body/meta tests don’t cover the case where `meta_size` and
`body_size` are each within `FLAGS_max_body_size`, but their sum (`msg_size`)
exceeds it. Adding a regression test for this helps ensure the per-message
limit is actually enforced on the bytes the parser buffers.
--
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]
