cap simple string length in RedisReply::ConsumePartialIOBuf - #3404
cap simple string length in RedisReply::ConsumePartialIOBuf#3404ubeddulla wants to merge 3 commits into
Conversation
Signed-off-by: ubeddulla khan <ubed@bugqore.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens Redis reply parsing in brpc by enforcing the existing redis_max_allocation_size limit for Redis simple-string (+) and error (-) replies, preventing oversized payloads from overflowing internal length bookkeeping and leading to out-of-bounds reads.
Changes:
- Add a max-allocation-size cap for simple-string and error replies in
RedisReply::ConsumePartialIOBuf. - Add regression tests covering oversized simple-string and error replies alongside existing allocation-limit tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/brpc/redis_reply.cpp |
Adds allocation-size enforcement for +/- reply parsing. |
test/brpc_redis_unittest.cpp |
Adds new test cases to ensure oversized +/- replies are rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (len > (size_t)FLAGS_redis_max_allocation_size) { | ||
| LOG(ERROR) << "simple string exceeds max allocation size! max=" | ||
| << FLAGS_redis_max_allocation_size << ", actually=" << len; | ||
| return PARSE_ERROR_ABSOLUTELY_WRONG; | ||
| } |
There was a problem hiding this comment.
Good point. Added an explicit negative check so a negative flag rejects the length here too, matching how the signed comparison in the bulk-string branch behaves.
Signed-off-by: ubeddulla khan <ubed@bugqore.com>
|
LGTM |
| if (FLAGS_redis_max_allocation_size < 0 || | ||
| len > (size_t)FLAGS_redis_max_allocation_size) { | ||
| LOG(ERROR) << "simple string exceeds max allocation size! max=" | ||
| << FLAGS_redis_max_allocation_size << ", actually=" << len; | ||
| return PARSE_ERROR_ABSOLUTELY_WRONG; |
There was a problem hiding this comment.
Good point. I now enforce the cap in the waiting-for-CRLF path too, so a peer that never sends the terminator can't grow buf past the limit (only up to the 2^32 hard bound before). Kept a one-byte '\r' allowance so a boundary CRLF split isn't rejected, matching how RedisCommandParser handles inline commands. Added an over-limit-before-CRLF test and a boundary-split test alongside the existing ones.
Signed-off-by: ubeddulla khan <ubed@bugqore.com>
The simple-string ('+') and error ('-') branch of RedisReply::ConsumePartialIOBuf never bounds the payload length against redis_max_allocation_size, unlike the bulk-string ('$') and array ('*') branches in the same function which both cap it. A server returning a simple string of 2^31 bytes or more truncates the length into the signed int _length, and c_str()/data()/Print()/SerializeTo() then take the small-string path for the negative value and read past the 16-byte inline buffer. Cap it the same way the sibling branches already do, and add a regression test next to the existing bulk-string and array limit checks.