From 948fbce4bdf6e59aa1df7a0f798f24eb046650ef Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Tue, 5 Apr 2022 14:46:26 +0300 Subject: [PATCH] Fix GETRANGE and ascii packing bugs --- src/core/compact_object.cc | 47 +++++++++++++++++--------------- src/core/compact_object_test.cc | 6 +++- src/server/string_family.cc | 12 +++++--- src/server/string_family_test.cc | 5 ++++ 4 files changed, 43 insertions(+), 27 deletions(-) diff --git a/src/core/compact_object.cc b/src/core/compact_object.cc index 3464fe6ff..f56d10549 100644 --- a/src/core/compact_object.cc +++ b/src/core/compact_object.cc @@ -383,18 +383,20 @@ quicklist* RobjWrapper::GetQL() const { // len must be at least 16 void ascii_pack(const char* ascii, size_t len, uint8_t* bin) { + const char* end = ascii + len; + unsigned i = 0; - while (len >= 8) { + while (ascii + 8 <= end) { for (i = 0; i < 7; ++i) { *bin++ = (ascii[0] >> i) | (ascii[1] << (7 - i)); ++ascii; } ++ascii; - len -= 8; } - for (i = 0; i < len; ++i) { - *bin++ = (ascii[i] >> i) | (ascii[i + 1] << (7 - i)); + // epilog - we do not pack since we have less than 8 bytes. + while (ascii < end) { + *bin++ = *ascii++; } } @@ -420,22 +422,19 @@ void ascii_unpack(const uint8_t* bin, size_t ascii_len, char* ascii) { *ascii++ = p >> 1; } + DCHECK_LT(ascii_len, 8u); for (i = 0; i < ascii_len; ++i) { - uint8_t src = *bin; - *ascii++ = (p >> (8 - i)) | ((src << i) & kM); - p = src; - ++bin; + *ascii++ = *bin++; } } -#pragma GCC pop_options - // compares packed and unpacked strings. packed must be of length = binpacked_len(ascii_len). bool compare_packed(const uint8_t* packed, const char* ascii, size_t ascii_len) { unsigned i = 0; bool res = true; + const char* end = ascii + ascii_len; - while (ascii_len >= 8) { + while (ascii + 8 <= end) { for (i = 0; i < 7; ++i) { uint8_t conv = (ascii[0] >> i) | (ascii[1] << (7 - i)); res &= (conv == *packed); @@ -445,19 +444,21 @@ bool compare_packed(const uint8_t* packed, const char* ascii, size_t ascii_len) if (!res) return false; + ++ascii; - ascii_len -= 8; } - for (i = 0; i < ascii_len; ++i) { - uint8_t b = (ascii[i] >> i) | (ascii[i + 1] << (7 - i)); - res &= (b == *packed); - ++packed; + while (ascii < end) { + if (*ascii++ != *packed++) { + return false; + } } - return res; + return true; } +#pragma GCC pop_options + } // namespace detail using namespace std; @@ -513,7 +514,7 @@ size_t CompactObj::Size() const { raw_size = u_.r_obj.Size(); break; default: - LOG(DFATAL) << "Should not reach " << int(taglen_); + LOG(DFATAL) << "Should not reach " << int(taglen_); } } uint8_t encoded = (mask_ & kEncMask); @@ -703,12 +704,14 @@ void CompactObj::SetString(std::string_view str) { if (is_ascii) { size_t encode_len = binpacked_len(str.size()); size_t rev_len = ascii_len(encode_len); - CHECK_GE(rev_len, str.size() - 1) << "Bad ascii encoding for len " << str.size(); - if (rev_len == str.size() - 1) { - mask |= ASCII1_ENC_BIT; + if (rev_len == str.size()) { + mask |= ASCII2_ENC_BIT; // str hits its highest bound. } else { - mask |= ASCII2_ENC_BIT; + CHECK_EQ(str.size(), rev_len - 1) + << "Bad ascii encoding for len " << str.size(); + + mask |= ASCII1_ENC_BIT; } tl.tmp_buf.resize(encode_len); diff --git a/src/core/compact_object_test.cc b/src/core/compact_object_test.cc index 42f2bfa4d..df708ad7a 100644 --- a/src/core/compact_object_test.cc +++ b/src/core/compact_object_test.cc @@ -126,7 +126,7 @@ TEST_F(CompactObjectTest, Int) { } TEST_F(CompactObjectTest, MediumString) { - string tmp(512, 'b'); + string tmp(511, 'b'); cobj_.SetString(tmp); EXPECT_EQ(tmp.size(), cobj_.Size()); @@ -134,6 +134,10 @@ TEST_F(CompactObjectTest, MediumString) { cobj_.SetString(tmp); EXPECT_EQ(tmp.size(), cobj_.Size()); cobj_.Reset(); + + tmp.assign(27463, 'c'); + cobj_.SetString(tmp); + EXPECT_EQ(27463, cobj_.Size()); } TEST_F(CompactObjectTest, AsciiUtil) { diff --git a/src/server/string_family.cc b/src/server/string_family.cc index a507cba09..57c6f5a34 100644 --- a/src/server/string_family.cc +++ b/src/server/string_family.cc @@ -264,6 +264,10 @@ void StringFamily::DecrBy(CmdArgList args, ConnectionContext* cntx) { if (!absl::SimpleAtoi(sval, &val)) { return (*cntx)->SendError(kInvalidIntErr); } + if (val == INT64_MIN) { + return (*cntx)->SendError(kIncrOverflow); + } + return IncrByGeneric(key, -val, cntx); } @@ -522,15 +526,15 @@ void StringFamily::GetRange(CmdArgList args, ConnectionContext* cntx) { if (end < 0) end = strlen + end; - if (strlen == 0 || start > end || size_t(start) >= strlen) { - return OpStatus::OK; - } - if (start < 0) start = 0; if (end < 0) end = 0; + if (strlen == 0 || start > end || size_t(start) >= strlen) { + return OpStatus::OK; + } + if (size_t(end) >= strlen) end = strlen - 1; diff --git a/src/server/string_family_test.cc b/src/server/string_family_test.cc index db4108a6c..6a5be9834 100644 --- a/src/server/string_family_test.cc +++ b/src/server/string_family_test.cc @@ -51,6 +51,7 @@ TEST_F(StringFamilyTest, Incr) { ASSERT_THAT(Run({"incrby", "key1", "1"}), ElementsAre(ErrArg("ERR value is not an integer"))); ASSERT_THAT(Run({"incrby", "ne", "0"}), ElementsAre(IntArg(0))); + ASSERT_THAT(Run({"decrby", "a", "-9223372036854775808"}), ElementsAre(ErrArg("overflow"))); } TEST_F(StringFamilyTest, Append) { @@ -285,6 +286,10 @@ TEST_F(StringFamilyTest, Range) { EXPECT_THAT(Run({"getrange", "key3", "2", "3"}), RespEq("3")); EXPECT_THAT(Run({"getrange", "key3", "3", "3"}), RespEq("")); EXPECT_THAT(Run({"getrange", "key3", "4", "5"}), RespEq("")); + + Run({"SET", "num", "1234"}); + EXPECT_THAT(Run({"getrange","num", "3", "5000"}), RespEq("4")); + EXPECT_THAT(Run({"getrange","num", "-5000", "10000"}), RespEq("1234")); } } // namespace dfly