From b2f4ca8c64c98a5af7225cbbcb7dcecf16159d80 Mon Sep 17 00:00:00 2001
From: sundb <sundbcn@gmail.com>
Date: Mon, 9 Aug 2021 22:13:46 +0800
Subject: [PATCH] Sanitize dump payload: handle remaining empty key when RDB
 loading and restore command (#9349)

This commit mainly fixes empty keys due to RDB loading and restore command,
which was omitted in #9297.

1) When loading quicklsit, if all the ziplists in the quicklist are empty, NULL will be returned.
    If only some of the ziplists are empty, then we will skip the empty ziplists silently.
2) When loading hash zipmap, if zipmap is empty, sanitization check will fail.
3) When loading hash ziplist, if ziplist is empty, NULL will be returned.
4) Add RDB loading test with sanitize.

(cherry picked from commit cbda492909cd2fff25263913cd2e1f00bc48a541)
---
 src/rdb.c                           |  22 +++++++++++-
 src/ziplist.c                       |   2 +-
 src/zipmap.c                        |   3 ++
 tests/assets/corrupt_empty_keys.rdb | Bin 187 -> 261 bytes
 tests/integration/corrupt-dump.tcl  |  50 ++++++++++++++++++++--------
 5 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/src/rdb.c b/src/rdb.c
index 051a0d87e..3cc84f64f 100644
--- a/src/rdb.c
+++ b/src/rdb.c
@@ -1842,7 +1842,19 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int *error) {
                 zfree(zl);
                 return NULL;
             }
-            quicklistAppendZiplist(o->ptr, zl);
+
+            /* Silently skip empty ziplists, if we'll end up with empty quicklist we'll fail later. */
+            if (ziplistLen(zl) == 0) {
+                zfree(zl);
+                continue;
+            } else {
+                quicklistAppendZiplist(o->ptr, zl);
+            }
+        }
+
+        if (quicklistCount(o->ptr) == 0) {
+            decrRefCount(o);
+            goto emptykey;
         }
     } else if (rdbtype == RDB_TYPE_HASH_ZIPMAP  ||
                rdbtype == RDB_TYPE_LIST_ZIPLIST ||
@@ -1927,6 +1939,14 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int *error) {
                     decrRefCount(o);
                     return NULL;
                 }
+
+                if (ziplistLen(encoded) == 0) {
+                    zfree(encoded);
+                    o->ptr = NULL;
+                    decrRefCount(o);
+                    goto emptykey;
+                }
+
                 o->type = OBJ_LIST;
                 o->encoding = OBJ_ENCODING_ZIPLIST;
                 listTypeConvert(o,OBJ_ENCODING_QUICKLIST);
diff --git a/src/ziplist.c b/src/ziplist.c
index 74c033ce2..b3b27ce36 100644
--- a/src/ziplist.c
+++ b/src/ziplist.c
@@ -1542,7 +1542,7 @@ int ziplistValidateIntegrity(unsigned char *zl, size_t size, int deep,
         return 0;
 
     /* Make sure the <zltail> entry really do point to the start of the last entry. */
-    if (prev != ZIPLIST_ENTRY_TAIL(zl))
+    if (prev != NULL && prev != ZIPLIST_ENTRY_TAIL(zl))
         return 0;
 
     /* Check that the count in the header is correct */
diff --git a/src/zipmap.c b/src/zipmap.c
index c24e81355..21a608843 100644
--- a/src/zipmap.c
+++ b/src/zipmap.c
@@ -430,6 +430,9 @@ int zipmapValidateIntegrity(unsigned char *zm, size_t size, int deep) {
             return 0;
     }
 
+    /* check that the zipmap is not empty. */
+    if (count == 0) return 0;
+
     /* check that the count in the header is correct */
     if (zm[0] != ZIPMAP_BIGLEN && zm[0] != count)
         return 0;
diff --git a/tests/assets/corrupt_empty_keys.rdb b/tests/assets/corrupt_empty_keys.rdb
index bc2b4f202b75fd17b02b73a879c91e0656a8443c..8f260d49333fbe04e749d0d4ea2f9a7349994cc1 100644
GIT binary patch
delta 148
zcmdnZ*vd4)%vfGmF!2{hX>n?bZf<Jsp#{Hq7=H02=BMcv6s0ESCgr3aVED)In}dOc
zB_pvogMo>;ICWy4880hKRdH$w0~b$DW^u{H2_mk1d>~eQVQFS^Hi*T*Cxa>wpPE}x
hQW;;BSpX7e<mP5zVBi8`28RFty&HFa51M_S1pxL4Gj9L@

delta 90
zcmZo=+RZq@%;MDz?!;dlrNyZ!y1A*jhd9`I8Gi93=BMcv6s0ESCgr3aVED)In~i~q
txj40ifseDWG&4CnC$o5BrkOYkOGaXG1_LWg6-e&?iK^(WXIVb12LR4rAuRv^

diff --git a/tests/integration/corrupt-dump.tcl b/tests/integration/corrupt-dump.tcl
index 896571c2f..75bdf0f7f 100644
--- a/tests/integration/corrupt-dump.tcl
+++ b/tests/integration/corrupt-dump.tcl
@@ -118,6 +118,16 @@ test {corrupt payload: #3080 - quicklist} {
     }
 }
 
+test {corrupt payload: quicklist with empty ziplist} {
+    start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
+        r config set sanitize-dump-payload no
+        r debug set-skip-checksum-validation 1
+        catch {r restore key 0 "\x0E\x01\x0B\x0B\x00\x00\x00\x0A\x00\x00\x00\x00\x00\xFF\x09\x00\xC2\x69\x37\x83\x3C\x7F\xFE\x6F" replace} err
+        assert_match "*Bad data format*" $err
+        r ping
+    }
+}
+
 test {corrupt payload: #3080 - ziplist} {
     start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
         # shallow sanitization is enough for restore to safely reject the payload with wrong size
@@ -148,20 +158,24 @@ test {corrupt payload: load corrupted rdb with no CRC - #3505} {
     kill_server $srv ;# let valgrind look for issues
 }
 
-test {corrupt payload: load corrupted rdb with empty keys} {
-    set server_path [tmpdir "server.rdb-corruption-empty-keys-test"]
-    exec cp tests/assets/corrupt_empty_keys.rdb $server_path
-    start_server [list overrides [list "dir" $server_path "dbfilename" "corrupt_empty_keys.rdb"]] {
-        r select 0
-        assert_equal [r dbsize] 0
+foreach sanitize_dump {no yes} {
+    test {corrupt payload: load corrupted rdb with empty keys} {
+        set server_path [tmpdir "server.rdb-corruption-empty-keys-test"]
+        exec cp tests/assets/corrupt_empty_keys.rdb $server_path
+        start_server [list overrides [list "dir" $server_path "dbfilename" "corrupt_empty_keys.rdb" "sanitize-dump-payload" $sanitize_dump]] {
+            r select 0
+            assert_equal [r dbsize] 0
 
-        verify_log_message 0 "*skipping empty key: set*" 0
-        verify_log_message 0 "*skipping empty key: quicklist*" 0
-        verify_log_message 0 "*skipping empty key: hash*" 0
-        verify_log_message 0 "*skipping empty key: hash_ziplist*" 0
-        verify_log_message 0 "*skipping empty key: zset*" 0
-        verify_log_message 0 "*skipping empty key: zset_ziplist*" 0
-        verify_log_message 0 "*empty keys skipped: 6*" 0
+            verify_log_message 0 "*skipping empty key: set*" 0
+            verify_log_message 0 "*skipping empty key: list_quicklist*" 0
+            verify_log_message 0 "*skipping empty key: list_quicklist_empty_ziplist*" 0
+            verify_log_message 0 "*skipping empty key: list_ziplist*" 0
+            verify_log_message 0 "*skipping empty key: hash*" 0
+            verify_log_message 0 "*skipping empty key: hash_ziplist*" 0
+            verify_log_message 0 "*skipping empty key: zset*" 0
+            verify_log_message 0 "*skipping empty key: zset_ziplist*" 0
+            verify_log_message 0 "*empty keys skipped: 8*" 0
+        }
     }
 }
 
@@ -241,6 +255,16 @@ test {corrupt payload: hash dupliacte records} {
     }
 }
 
+test {corrupt payload: hash empty zipmap} {
+    start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
+        r config set sanitize-dump-payload no
+        r debug set-skip-checksum-validation 1
+        catch { r RESTORE _hash 0 "\x09\x02\x00\xFF\x09\x00\xC0\xF1\xB8\x67\x4C\x16\xAC\xE3" } err
+        assert_match "*Bad data format*" $err
+        verify_log_message 0 "*Zipmap integrity check failed*" 0
+    }
+}
+
 test {corrupt payload: fuzzer findings - NPD in streamIteratorGetID} {
     start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
         r config set sanitize-dump-payload no