ubeddulla commented on code in PR #3392:
URL: https://github.com/apache/brpc/pull/3392#discussion_r3619820264
##########
src/brpc/memcache.cpp:
##########
@@ -503,8 +503,12 @@ bool MemcacheResponse::PopStore(uint8_t command, uint64_t*
cas_value) {
- (int)header.key_length;
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 {
Review 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.
--
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]
