fix: fix crash when inserting to listpack an empty value. (#1324)

fix: fix crash when inserting to listpack empty value.

We can not pass null pointers to listpack.
Fixes #1305 and probably fixes #1290

Signed-off-by: Roman Gershman <roman@dragonflydb.io>

src/redis/listpack.c

@@ -180,7 +180,8 @@ int lpStringToInt64(const char *s, unsigned long slen, int64_t *value) {
     int negative = 0;
     uint64_t v;
 
-    if (plen == slen)
+    /* Abort if length indicates this cannot possibly be an int */
+    if (slen == 0)
         return 0;
 
     /* Special case: first and only digit is 0. */
@@ -958,7 +959,7 @@ unsigned char *lpPrependInteger(unsigned char *lp, long long lval) {
     return lpInsertInteger(lp, lval, p, LP_BEFORE, NULL);
 }
 
-/* Append the specified element 'ele' of length 'len' at the end of the
+/* Append the specified element 'ele' of length 'size' at the end of the
  * listpack. It is implemented in terms of lpInsert(), so the return value is
  * the same as lpInsert(). */
 unsigned char *lpAppend(unsigned char *lp, const unsigned char *ele, uint32_t size) {

src/server/debugcmd.cc

@@ -215,7 +215,7 @@ void DebugCmd::Load(string_view filename) {
   }
 
   absl::Cleanup rev_state = [this] {
-    sf_.service().SwitchState(GlobalState::SAVING, GlobalState::ACTIVE);
+    sf_.service().SwitchState(GlobalState::LOADING, GlobalState::ACTIVE);
   };
 
   const CommandId* cid = sf_.service().FindCmd("FLUSHALL");

src/server/rdb_load.cc

@@ -60,6 +60,7 @@ namespace {
 
 constexpr size_t kYieldPeriod = 50000;
 constexpr size_t kMaxBlobLen = 1ULL << 16;
+constexpr char kErrCat[] = "dragonfly.rdbload";
 
 inline void YieldIfNeeded(size_t i) {
   if (i % kYieldPeriod == 0) {
@@ -70,7 +71,7 @@ inline void YieldIfNeeded(size_t i) {
 class error_category : public std::error_category {
  public:
   const char* name() const noexcept final {
-    return "dragonfly.rdbload";
+    return kErrCat;
   }
 
   string message(int ev) const final;
@@ -980,7 +981,8 @@ string_view RdbLoaderBase::OpaqueObjLoader::ToSV(const RdbVariant& obj) {
 
   const base::PODArray<char>* ch_arr = get_if<base::PODArray<char>>(&obj);
   if (ch_arr) {
-    return string_view(ch_arr->data(), ch_arr->size());
+    // pass non-null pointer to avoid UB with lp API.
+    return ch_arr->empty() ? ""sv : string_view{ch_arr->data(), ch_arr->size()};
   }
 
   const LzfString* lzf = get_if<LzfString>(&obj);
@@ -990,7 +992,7 @@ string_view RdbLoaderBase::OpaqueObjLoader::ToSV(const RdbVariant& obj) {
                        lzf->uncompressed_len) == 0) {
       LOG(ERROR) << "Invalid LZF compressed string";
       ec_ = RdbError(errc::rdb_file_corrupted);
-      return string_view{};
+      return string_view{tset_blob_.data(), 0};
     }
     return string_view{tset_blob_.data(), tset_blob_.size()};
   }

src/server/rdb_test.cc

@@ -5,6 +5,7 @@
 
 extern "C" {
 #include "redis/crc64.h"
+#include "redis/listpack.h"
 #include "redis/redis_aux.h"
 #include "redis/zmalloc.h"
 }
@@ -318,7 +319,7 @@ TEST_F(RdbTest, SaveManyDbs) {
 }
 
 TEST_F(RdbTest, HMapBugs) {
-  // Force OBJ_ENCODING_HT encoding.
+  // Force kEncodingStrMap2 encoding.
   server.hash_max_listpack_value = 0;
   Run({"hset", "hmap1", "key1", "val", "key2", "val2"});
   Run({"hset", "hmap2", "key1", string(690557, 'a')});
@@ -328,6 +329,26 @@ TEST_F(RdbTest, HMapBugs) {
   EXPECT_EQ(2, CheckedInt({"hlen", "hmap1"}));
 }
 
+TEST_F(RdbTest, Issue1305) {
+  /***************
+   * The code below crashes because of the weird listpack API that assumes that lpInsert
+   * pointers are null then it should do deletion :(. See lpInsert comments for more info.
+
+     uint8_t* lp = lpNew(128);
+     lpAppend(lp, NULL, 0);
+     lpFree(lp);
+
+  */
+
+  // Force kEncodingStrMap2 encoding.
+  server.hash_max_listpack_value = 0;
+  Run({"hset", "hmap", "key1", "val", "key2", ""});
+
+  server.hash_max_listpack_value = 32;
+  Run({"debug", "reload"});
+  EXPECT_EQ(2, CheckedInt({"hlen", "hmap"}));
+}
+
 TEST_F(RdbTest, JsonTest) {
   string_view data[] = {
       R"({"a":1})"sv,                          //

src/server/stream_family.cc

@@ -118,6 +118,11 @@ inline string NoGroupError(string_view key, string_view cgroup) {
   return absl::StrCat("-NOGROUP No such consumer group '", cgroup, "' for key name '", key, "'");
 }
 
+inline const uint8_t* SafePtr(MutableSlice field) {
+  return field.empty() ? reinterpret_cast<const uint8_t*>("")
+                       : reinterpret_cast<const uint8_t*>(field.data());
+}
+
 bool ParseID(string_view strid, bool strict, uint64_t missing_seq, ParsedStreamId* dest) {
   if (strid.empty() || strid.size() > 127)
     return false;
@@ -394,7 +399,8 @@ int StreamAppendItem(stream* s, CmdArgList fields, streamID* added_id, streamID*
     lp = lpAppendInteger(lp, numfields);
     for (int64_t i = 0; i < numfields; i++) {
       MutableSlice field = fields[i * 2];
-      lp = lpAppend(lp, reinterpret_cast<const uint8_t*>(field.data()), field.size());
+
+      lp = lpAppend(lp, SafePtr(field), field.size());
     }
     lp = lpAppendInteger(lp, 0); /* Master entry zero terminator. */
     raxInsert(s->rax_tree, (unsigned char*)&rax_key, sizeof(rax_key), lp, NULL);
@@ -468,8 +474,8 @@ int StreamAppendItem(stream* s, CmdArgList fields, streamID* added_id, streamID*
   for (int64_t i = 0; i < numfields; i++) {
     MutableSlice field = fields[i * 2], value = fields[i * 2 + 1];
     if (!(flags & STREAM_ITEM_FLAG_SAMEFIELDS))
-      lp = lpAppend(lp, reinterpret_cast<const uint8_t*>(field.data()), field.size());
-    lp = lpAppend(lp, reinterpret_cast<const uint8_t*>(value.data()), value.size());
+      lp = lpAppend(lp, SafePtr(field), field.size());
+    lp = lpAppend(lp, SafePtr(value), value.size());
   }
   /* Compute and store the lp-count field. */
   int64_t lp_count = numfields;