guard negative value_size in memcache PopStore#3392
Conversation
Signed-off-by: ubeddulla khan <ubed@bugqore.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the memcache binary response parsing in MemcacheResponse::PopStore by preventing a negative value_size from being implicitly converted to a huge size_t and draining pipelined data into the error string, which can desynchronize subsequent replies on the same connection.
Changes:
- Add a
value_size < 0guard inMemcacheResponse::PopStorebefore callingcutn(&_err, value_size). - Add a unit test that reproduces the malformed STORE error-response scenario and asserts the error string does not absorb bytes from a following pipelined response.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/brpc/memcache.cpp |
Adds negative value_size handling in STORE/DELETE/FLUSH response parsing. |
test/brpc_memcache_unittest.cpp |
Adds regression coverage for malformed STORE error replies causing negative value_size. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (header.status != (uint16_t)STATUS_SUCCESS) { | ||
| _buf.pop_front(sizeof(header) + header.extras_length + header.key_length); | ||
| _err.clear(); | ||
| _buf.cutn(&_err, value_size); | ||
| if (value_size < 0) { | ||
| butil::string_printf(&_err, "value_size=%d is negative", value_size); | ||
| } else { |
There was a problem hiding this comment.
Good catch, that's a real gap. The pop_front ran before my guard, so with extras_length=1 and total_body_length=0 the parser still swallowed one byte of the next pipelined reply even though cutn was no longer underflowing.
Moved the value_size < 0 check ahead of the pop_front and made the malformed path discard exactly sizeof(header) + total_body_length, which is already bounds-checked a few lines above. Extended the regression test to assert the buffer is left exactly at the start of the following response. Against the previous commit it fails with 19 bytes remaining instead of 20, so it pins the desync you described.
I left the sibling parsers alone to keep this focused on PopStore; PopCounter and PopVersion pop the same way and have the same residual skew, happy to follow up separately if you'd like that covered too.
Move the value_size < 0 check ahead of the pop_front so a malformed reply no longer pops sizeof(header) + extras_length + key_length past the declared message boundary. Discard exactly sizeof(header) + total_body_length instead, keeping the following pipelined responses aligned. Signed-off-by: ubeddulla khan <ubed@bugqore.com>
|
LGTM |
MemcacheResponse::PopStore derives value_size by subtracting the reply's extras_length and key_length from total_body_length, but unlike its siblings PopGet, PopCounter and PopVersion it never checks whether value_size went negative before handing it to cutn(&_err, value_size). A server (or man-in-the-middle) that returns a STORE/DELETE/FLUSH error reply whose extras plus key exceed the body drives value_size below zero, so the huge size_t drains the rest of the pipelined buffer into the error string and desyncs the following replies on the connection. Add the same value_size < 0 guard the sibling parsers already carry.