You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "ubeddulla (via GitHub)" <gi...@apache.org> on 2026/07/17 07:54:00 UTC
[PR] guard negative value_size in memcache PopStore (brpc)
ubeddulla opened a new pull request, #3392:
URL: https://github.com/apache/brpc/pull/3392
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.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] guard negative value_size in memcache PopStore (brpc)
Posted by "ubeddulla (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] guard negative value_size in memcache PopStore (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3392:
URL: https://github.com/apache/brpc/pull/3392
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] guard negative value_size in memcache PopStore (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3392:
URL: https://github.com/apache/brpc/pull/3392#discussion_r3618835421
##########
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:
When `value_size < 0` (i.e., `extras_length + key_length > total_body_length`), this code has already executed `_buf.pop_front(sizeof(header) + header.extras_length + header.key_length)`, which pops past the declared message boundary and consumes bytes from the following pipelined response. This can still desync the connection even though the `cutn()` underflow is avoided. Consider checking `value_size < 0` before popping `extras_length + key_length`, and in the malformed case discard only `sizeof(header) + header.total_body_length` (the declared message size).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] guard negative value_size in memcache PopStore (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3392:
URL: https://github.com/apache/brpc/pull/3392#issuecomment-5033621066
LGTM
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org