From 6cde9c11560cf3a14a2b5b7a5b0682ce7ab17ff1 Mon Sep 17 00:00:00 2001
From: yaxin0710 <142375608+yaxin0710@users.noreply.github.com>
Date: Mon, 13 Nov 2023 14:57:32 +0800
Subject: [PATCH] fix: binary list key randomly droped by lrem  (#2118)

* memcmp replace strcmp

* fix

* pivot covert to Slice

* add lists unit test

* no need convert to slice

* Update src/storage/src/redis_lists.cc

* fix space in  redis_lists.cc

---------

Co-authored-by: yaxin0710 <1013452617@qq.com>
Co-authored-by: Ted Mostly <wanghenshui@qq.com>
---
 src/storage/src/redis_lists.cc | 10 +++++-----
 tests/integration/list_test.go | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/src/storage/src/redis_lists.cc b/src/storage/src/redis_lists.cc
index 3e8a71433..06e182a85 100644
--- a/src/storage/src/redis_lists.cc
+++ b/src/storage/src/redis_lists.cc
@@ -251,7 +251,7 @@ Status RedisLists::LInsert(const Slice& key, const BeforeOrAfter& before_or_afte
       ListsDataKey start_data_key(key, version, current_index);
       for (iter->Seek(start_data_key.Encode()); iter->Valid() && current_index < parsed_lists_meta_value.right_index();
            iter->Next(), current_index++) {
-        if (strcmp(iter->value().ToString().data(), pivot.data()) == 0) {
+        if (iter->value() == Slice(pivot)) {
           find_pivot = true;
           pivot_index = current_index;
           break;
@@ -538,7 +538,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
         for (iter->Seek(start_data_key.Encode());
              iter->Valid() && current_index <= stop_index && ((count == 0) || rest != 0);
              iter->Next(), current_index++) {
-          if (strcmp(iter->value().ToString().data(), value.data()) == 0) {
+          if (iter->value() == value) {
             target_index.push_back(current_index);
             if (count != 0) {
               rest--;
@@ -552,7 +552,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
         for (iter->Seek(stop_data_key.Encode());
              iter->Valid() && current_index >= start_index && ((count == 0) || rest != 0);
              iter->Prev(), current_index--) {
-          if (strcmp(iter->value().ToString().data(), value.data()) == 0) {
+          if (iter->value() == value) {
             target_index.push_back(current_index);
             if (count != 0) {
               rest--;
@@ -577,7 +577,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
           rocksdb::Iterator* iter = db_->NewIterator(default_read_options_, handles_[1]);
           for (iter->Seek(sublist_right_key.Encode()); iter->Valid() && current_index >= start_index;
                iter->Prev(), current_index--) {
-            if ((strcmp(iter->value().ToString().data(), value.data()) == 0) && rest > 0) {
+            if ((iter->value() == value) && rest > 0) {
               rest--;
             } else {
               ListsDataKey lists_data_key(key, version, left--);
@@ -597,7 +597,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
           rocksdb::Iterator* iter = db_->NewIterator(default_read_options_, handles_[1]);
           for (iter->Seek(sublist_left_key.Encode()); iter->Valid() && current_index <= stop_index;
                iter->Next(), current_index++) {
-            if ((strcmp(iter->value().ToString().data(), value.data()) == 0) && rest > 0) {
+            if ((iter->value() == value) && rest > 0) {
               rest--;
             } else {
               ListsDataKey lists_data_key(key, version, right++);
diff --git a/tests/integration/list_test.go b/tests/integration/list_test.go
index 6b56dc08f..d3ddeff15 100644
--- a/tests/integration/list_test.go
+++ b/tests/integration/list_test.go
@@ -554,6 +554,29 @@ var _ = Describe("List Commands", func() {
 			Expect(lRange.Val()).To(Equal([]string{"hello", "key"}))
 		})
 
+		It("should LRem binary", func() {
+			rPush := client.RPush(ctx, "list", "\x00\xa2\x00")
+			Expect(rPush.Err()).NotTo(HaveOccurred())
+			rPush = client.RPush(ctx, "list", "\x00\x9d")
+			Expect(rPush.Err()).NotTo(HaveOccurred())
+
+			lInsert := client.LInsert(ctx, "list", "BEFORE", "\x00\x9d", "\x00\x5f")
+			Expect(lInsert.Err()).NotTo(HaveOccurred())
+			Expect(lInsert.Val()).To(Equal(int64(3)))
+
+			lRange := client.LRange(ctx, "list", 0, -1)
+			Expect(lRange.Err()).NotTo(HaveOccurred())
+			Expect(lRange.Val()).To(Equal([]string{"\x00\xa2\x00", "\x00\x5f", "\x00\x9d"}))
+
+			lRem := client.LRem(ctx, "list", -1, "\x00\x5f")
+			Expect(lRem.Err()).NotTo(HaveOccurred())
+			Expect(lRem.Val()).To(Equal(int64(1)))
+
+			lRange = client.LRange(ctx, "list", 0, -1)
+			Expect(lRange.Err()).NotTo(HaveOccurred())
+			Expect(lRange.Val()).To(Equal([]string{"\x00\xa2\x00", "\x00\x9d"}))
+		})
+
 		It("should LSet", func() {
 			rPush := client.RPush(ctx, "list", "one")
 			Expect(rPush.Err()).NotTo(HaveOccurred())