Copilot commented on code in PR #3363:
URL: https://github.com/apache/brpc/pull/3363#discussion_r3487531108
##########
src/brpc/redis_command.cpp:
##########
@@ -425,7 +442,27 @@ RedisCommandConsumeState
RedisCommandParser::ConsumeImpl(butil::IOBuf& buf,
return CONSUME_STATE_ERROR;
}
}
- const size_t buf_size = buf.size();
+ const size_t max_inline_size =
+ FLAGS_redis_max_allocation_size > 0 ?
+ static_cast<size_t>(FLAGS_redis_max_allocation_size) : 0;
+ const size_t crlf_pos = FindCRLF(buf, max_inline_size + 2);
+ if (crlf_pos == CRLF_NOT_FOUND) {
+ if (buf.size() <= max_inline_size) {
+ *err = PARSE_ERROR_NOT_ENOUGH_DATA;
+ return CONSUME_STATE_ERROR;
+ }
+ LOG(ERROR) << "inline command exceeds max allocation size! max="
+ << FLAGS_redis_max_allocation_size << ", actually=" <<
buf.size();
+ *err = PARSE_ERROR_ABSOLUTELY_WRONG;
+ return CONSUME_STATE_ERROR;
+ }
Review Comment:
When the inline command length is exactly redis_max_allocation_size and the
incoming data ends with a lone '\r' (with '\n' arriving in the next read),
FindCRLF() returns not-found and buf.size() becomes max_inline_size+1, which
currently triggers PARSE_ERROR_ABSOLUTELY_WRONG. This rejects a valid command
whose CRLF is simply split across reads; it should return
PARSE_ERROR_NOT_ENOUGH_DATA in that boundary case.
##########
test/brpc_redis_unittest.cpp:
##########
@@ -1512,6 +1512,35 @@ TEST_F(RedisTest, memory_allocation_limits) {
ASSERT_EQ(brpc::PARSE_ERROR_ABSOLUTELY_WRONG, err);
}
+ {
+ // Test inline command exceeding limit before CRLF arrives.
+ brpc::RedisCommandParser parser;
+ butil::IOBuf buf;
+ std::string large_inline_cmd = "get ";
+ large_inline_cmd.append(brpc::FLAGS_redis_max_allocation_size + 1,
'k');
+ buf.append(large_inline_cmd);
+
+ std::vector<butil::StringPiece> args;
+ brpc::ParseError err = parser.Consume(buf, &args, &arena);
+ ASSERT_EQ(brpc::PARSE_ERROR_ABSOLUTELY_WRONG, err);
+ }
+
Review Comment:
The new inline-size limit logic has a subtle boundary case when CRLF is
split across reads at the exact size limit (command length ==
redis_max_allocation_size, then a lone '\r'). Adding a regression test for that
split-CRLF scenario would prevent reintroducing this bug later.
--
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]
