Fix GETRANGE and ascii packing bugs

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);

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) {

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;
 

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