From 7f1c4f8cc3db0c46d018ad04cc86cab89327e19d Mon Sep 17 00:00:00 2001 From: ubeddulla khan Date: Fri, 17 Jul 2026 13:23:37 +0530 Subject: [PATCH 1/2] guard negative value_size in memcache PopStore Signed-off-by: ubeddulla khan --- src/brpc/memcache.cpp | 8 ++++++-- test/brpc_memcache_unittest.cpp | 27 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/brpc/memcache.cpp b/src/brpc/memcache.cpp index c7d6f836b1..fbe18a6ce8 100644 --- a/src/brpc/memcache.cpp +++ b/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 { + _err.clear(); + _buf.cutn(&_err, value_size); + } return false; } LOG_IF(ERROR, value_size != 0) << "STORE response must not have value, actually=" diff --git a/test/brpc_memcache_unittest.cpp b/test/brpc_memcache_unittest.cpp index b1da958449..8a9b9ffacf 100644 --- a/test/brpc_memcache_unittest.cpp +++ b/test/brpc_memcache_unittest.cpp @@ -62,6 +62,33 @@ TEST(MemcacheParserTest, RejectOversizedResponseBeforeBufferingBody) { } +TEST(MemcacheParserTest, PopStoreRejectsNegativeValueSize) { + // A STORE error response whose extras_length + key_length exceeds + // total_body_length makes value_size negative. PopStore used to pass that + // straight to cutn(&_err, value_size); the negative value became a huge + // size_t and drained the rest of the (pipelined) buffer into the error + // string. The sibling parsers PopGet/PopCounter/PopVersion already guard + // value_size < 0; PopStore must do the same. + brpc::policy::MemcacheResponseHeader header = {}; + header.magic = brpc::policy::MC_MAGIC_RESPONSE; + header.command = brpc::policy::MC_BINARY_SET; + header.status = 1; // non-zero: error response + header.extras_length = 1; // claims extras... + header.key_length = 0; + header.total_body_length = 0; // ...but body carries none -> value_size = -1 + + brpc::MemcacheResponse response; + response.raw_buffer().append(&header, sizeof(header)); + // Bytes of a following pipelined response that must not leak into _err. + const char next_response[] = "SECRET-NEXT-RESPONSE"; + response.raw_buffer().append(next_response, sizeof(next_response) - 1); + + uint64_t cas_value = 0; + ASSERT_FALSE(response.PopSet(&cas_value)); + ASSERT_EQ("value_size=-1 is negative", response.LastError()); + ASSERT_EQ(std::string::npos, response.LastError().find("RESPONSE")); +} + static pthread_once_t download_memcached_once = PTHREAD_ONCE_INIT; static pid_t g_mc_pid = -1; From 8019b52f17969a1f6bb39eb9d592640e15a30149 Mon Sep 17 00:00:00 2001 From: ubeddulla khan Date: Tue, 21 Jul 2026 11:35:30 +0530 Subject: [PATCH 2/2] memcache: drop only the declared message when value_size is negative 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 --- src/brpc/memcache.cpp | 15 +++++++++------ test/brpc_memcache_unittest.cpp | 4 ++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/brpc/memcache.cpp b/src/brpc/memcache.cpp index fbe18a6ce8..8d01e7bfee 100644 --- a/src/brpc/memcache.cpp +++ b/src/brpc/memcache.cpp @@ -501,14 +501,17 @@ bool MemcacheResponse::PopStore(uint8_t command, uint64_t* cas_value) { LOG_IF(ERROR, header.key_length != 0) << "STORE response must not have key"; int value_size = (int)header.total_body_length - (int)header.extras_length - (int)header.key_length; + if (value_size < 0) { + // extras_length + key_length overrun the declared body. Drop exactly the + // declared message so that following pipelined responses stay aligned. + _buf.pop_front(sizeof(header) + header.total_body_length); + butil::string_printf(&_err, "value_size=%d is negative", value_size); + return false; + } if (header.status != (uint16_t)STATUS_SUCCESS) { _buf.pop_front(sizeof(header) + header.extras_length + header.key_length); - if (value_size < 0) { - butil::string_printf(&_err, "value_size=%d is negative", value_size); - } else { - _err.clear(); - _buf.cutn(&_err, value_size); - } + _err.clear(); + _buf.cutn(&_err, value_size); return false; } LOG_IF(ERROR, value_size != 0) << "STORE response must not have value, actually=" diff --git a/test/brpc_memcache_unittest.cpp b/test/brpc_memcache_unittest.cpp index 8a9b9ffacf..3b7e676bdd 100644 --- a/test/brpc_memcache_unittest.cpp +++ b/test/brpc_memcache_unittest.cpp @@ -87,6 +87,10 @@ TEST(MemcacheParserTest, PopStoreRejectsNegativeValueSize) { ASSERT_FALSE(response.PopSet(&cas_value)); ASSERT_EQ("value_size=-1 is negative", response.LastError()); ASSERT_EQ(std::string::npos, response.LastError().find("RESPONSE")); + // Only the declared message (header + total_body_length) is dropped, so the + // following pipelined response is still intact at the head of the buffer. + ASSERT_EQ(sizeof(next_response) - 1, response.raw_buffer().size()); + ASSERT_EQ(next_response, response.raw_buffer().to_string()); } static pthread_once_t download_memcached_once = PTHREAD_ONCE_INIT;