Skip to content

Commit 9d10abf

Browse files
zuiderkwastzuiderkwast
authored
Relaxed RDB check for foreign RDB formats (#2543)
In #1604, we attempt to read future Valkey RDB formats, but we rejected foreign RDB formats, because of the risk that the opcodes and types added by other projects collide with the new types and opcodes added in recent Valkey versions. This change accepts foreign RDB versions but limits the types and opcodes to the ones that we can understand, to prevent misinterpretation of types/opcodes which could lead to undefined behavior. If unsupported RDB types or opcodes are seen, we error out. Additional changes: * Improve error reporting when encountering unknown RDB types in relaxed version check mode. * Tests for loading various RDB files. * Improvement to valkey-check-rdb to accept future and foreign RDB versions, including tests. --------- Signed-off-by: Viktor Söderqvist <[email protected]>
1 parent 811a644 commit 9d10abf

File tree

9 files changed

+197
-25
lines changed

9 files changed

+197
-25
lines changed

‎src/cluster.c‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,7 @@ int verifyDumpPayload(unsigned char *p, size_t len, uint16_t *rdbver_ptr) {
166166
if (rdbver_ptr) {
167167
*rdbver_ptr = rdbver;
168168
}
169-
if ((rdbver >= RDB_FOREIGN_VERSION_MIN && rdbver <= RDB_FOREIGN_VERSION_MAX) ||
170-
(rdbver > RDB_VERSION && server.rdb_version_check == RDB_VERSION_CHECK_STRICT)) {
171-
return C_ERR;
172-
}
173-
169+
if (!rdbIsVersionAccepted(rdbver, false, false)) return C_ERR;
174170
if (server.skip_checksum_validation) return C_OK;
175171

176172
/* Verify CRC64 */
@@ -203,6 +199,7 @@ void dumpCommand(client *c) {
203199
/* RESTORE key ttl serialized-value [REPLACE] [ABSTTL] [IDLETIME seconds] [FREQ frequency] */
204200
void restoreCommand(client *c) {
205201
long long ttl, lfu_freq = -1, lru_idle = -1, lru_clock = -1;
202+
uint16_t rdbver = 0;
206203
rio payload;
207204
int j, type, replace = 0, absttl = 0;
208205
robj *obj;
@@ -251,14 +248,27 @@ void restoreCommand(client *c) {
251248
}
252249

253250
/* Verify RDB version and data checksum. */
254-
if (verifyDumpPayload(c->argv[3]->ptr, sdslen(c->argv[3]->ptr), NULL) == C_ERR) {
251+
if (verifyDumpPayload(c->argv[3]->ptr, sdslen(c->argv[3]->ptr), &rdbver) == C_ERR) {
255252
addReplyError(c, "DUMP payload version or checksum are wrong");
256253
return;
257254
}
258255

259256
rioInitWithBuffer(&payload, c->argv[3]->ptr);
260-
if (((type = rdbLoadObjectType(&payload)) == -1) ||
261-
((obj = rdbLoadObject(type, &payload, key->ptr, c->db->id, NULL)) == NULL)) {
257+
type = rdbLoadObjectType(&payload);
258+
if (type == -1) {
259+
addReplyError(c, "Bad data format");
260+
return;
261+
}
262+
263+
/* If it's a foreign RDB format, only accept old data types that we know
264+
* existed in the past and that don't clash with new types added later. */
265+
if (rdbIsForeignVersion(rdbver) && type >= RDB_FOREIGN_TYPE_MIN) {
266+
addReplyErrorFormat(c, "Unsupported foreign data type: %d", type);
267+
return;
268+
}
269+
270+
obj = rdbLoadObject(type, &payload, key->ptr, c->db->id, NULL);
271+
if (obj == NULL) {
262272
addReplyError(c, "Bad data format");
263273
return;
264274
}

‎src/rdb.c‎

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,22 @@ void rdbCheckError(const char *fmt, ...);
7676
void rdbCheckSetError(const char *fmt, ...);
7777
int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadingCtx *rdb_loading_ctx);
7878

79+
/* Returns true if the RDB version is valid and accepted, false otherwise. This
80+
* function takes configuration into account. The parameter `is_valkey_magic`
81+
* indicates that an RDB file with the VALKEY magic string was parsed.
82+
* `is_redis_magic` indicates a legacy RDB file with the REDIS magic string.
83+
* When there is no magic string such as in DUMP/RESTORE, set both to false. */
84+
bool rdbIsVersionAccepted(int rdbver, bool is_valkey_magic, bool is_redis_magic) {
85+
if (rdbver < 1) return false;
86+
if (is_valkey_magic && rdbver <= RDB_FOREIGN_VERSION_MAX) return false;
87+
if (is_redis_magic && rdbver > RDB_FOREIGN_VERSION_MAX) return false;
88+
if (server.rdb_version_check == RDB_VERSION_CHECK_STRICT) {
89+
if (rdbver > RDB_VERSION) return false; /* future version */
90+
if (rdbIsForeignVersion(rdbver)) return false;
91+
}
92+
return true;
93+
}
94+
7995
#ifdef __GNUC__
8096
void rdbReportError(int corruption_error, int linenum, char *reason, ...) __attribute__((format(printf, 3, 4)));
8197
#endif
@@ -2877,6 +2893,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
28772893
return NULL;
28782894
}
28792895
o = createModuleObject(mt, ptr);
2896+
} else if (server.rdb_version_check == RDB_VERSION_CHECK_RELAXED) {
2897+
/* Future or foreign type. Don't report it as an internal error. */
2898+
if (error) *error = RDB_LOAD_ERR_UNKNOWN_TYPE;
2899+
return NULL;
28802900
} else {
28812901
rdbReportReadError("Unknown RDB encoding type %d", rdbtype);
28822902
return NULL;
@@ -3086,10 +3106,7 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
30863106
return C_ERR;
30873107
}
30883108
rdbver = atoi(buf + 6);
3089-
if (rdbver < 1 || (rdbver < RDB_FOREIGN_VERSION_MIN && !is_redis_magic) ||
3090-
(rdbver >= RDB_FOREIGN_VERSION_MIN && rdbver <= RDB_FOREIGN_VERSION_MAX) ||
3091-
(rdbver > RDB_FOREIGN_VERSION_MAX && !is_valkey_magic) ||
3092-
(rdbver > RDB_VERSION && server.rdb_version_check == RDB_VERSION_CHECK_STRICT)) {
3109+
if (!rdbIsVersionAccepted(rdbver, is_valkey_magic, is_redis_magic)) {
30933110
serverLog(LL_WARNING, "Can't handle RDB format version %d", rdbver);
30943111
return C_ERR;
30953112
}
@@ -3105,6 +3122,13 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
31053122
/* Read type. */
31063123
if ((type = rdbLoadType(rdb)) == -1) goto eoferr;
31073124

3125+
/* Safeguard for unknown foreign opcode interpretations. */
3126+
if (is_redis_magic && type >= RDB_FOREIGN_TYPE_MIN && type <= RDB_FOREIGN_TYPE_MAX) {
3127+
serverLog(LL_WARNING, "Can't handle foreign type or opcode %d in RDB with version %d",
3128+
type, rdbver);
3129+
return C_ERR;
3130+
}
3131+
31083132
/* Handle special types. */
31093133
if (type == RDB_OPCODE_EXPIRETIME) {
31103134
/* EXPIRETIME: load an expire associated with the next key
@@ -3157,6 +3181,19 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
31573181
if ((expires_size = rdbLoadLen(rdb, NULL)) == RDB_LENERR) goto eoferr;
31583182
should_expand_db = 1;
31593183
continue; /* Read next opcode. */
3184+
} else if (type == RDB_OPCODE_SLOT_INFO) {
3185+
/* RDB slot info size annotations used in pre-8.0 and foreign RDB.
3186+
* See the aux field "slot-info". */
3187+
uint64_t slot_id, slot_size, expires_slot_size;
3188+
if ((slot_id = rdbLoadLen(rdb, NULL)) == RDB_LENERR) goto eoferr;
3189+
if ((slot_size = rdbLoadLen(rdb, NULL)) == RDB_LENERR) goto eoferr;
3190+
if ((expires_slot_size = rdbLoadLen(rdb, NULL)) == RDB_LENERR) goto eoferr;
3191+
if (server.cluster_enabled && slot_id < CLUSTER_SLOTS) {
3192+
if (slot_size) kvstoreHashtableExpand(db->keys, slot_id, slot_size);
3193+
if (expires_slot_size) kvstoreHashtableExpand(db->expires, slot_id, expires_slot_size);
3194+
should_expand_db = 0;
3195+
}
3196+
continue; /* Read next opcode. */
31603197
} else if (type == RDB_OPCODE_AUX) {
31613198
/* AUX: generic string-string fields. Use to add state to RDB
31623199
* which is backward compatible. Implementations of RDB loading
@@ -3225,7 +3262,7 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
32253262
goto eoferr;
32263263
}
32273264

3228-
if (server.cluster_enabled) {
3265+
if (server.cluster_enabled && slot_id >= 0 && slot_id < CLUSTER_SLOTS) {
32293266
/* In cluster mode we resize individual slot specific dictionaries based on the number of keys that
32303267
* slot holds. */
32313268
if (slot_size) kvstoreHashtableExpand(db->keys, slot_id, slot_size);
@@ -3367,6 +3404,10 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
33673404
if (error == RDB_LOAD_ERR_EMPTY_KEY) {
33683405
if (empty_keys_skipped++ < 10) serverLog(LL_NOTICE, "rdbLoadObject skipping empty key: %s", key);
33693406
sdsfree(key);
3407+
} else if (error == RDB_LOAD_ERR_UNKNOWN_TYPE) {
3408+
sdsfree(key);
3409+
serverLog(LL_WARNING, "Unknown type or opcode when loading DB. Unrecoverable error, aborting now.");
3410+
return C_ERR;
33703411
} else {
33713412
sdsfree(key);
33723413
goto eoferr;

‎src/rdb.h‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@
5757
static_assert(RDB_VERSION < RDB_FOREIGN_VERSION_MIN || RDB_VERSION > RDB_FOREIGN_VERSION_MAX,
5858
"RDB version in foreign version range");
5959

60+
static inline bool rdbIsForeignVersion(int rdbver) {
61+
return rdbver >= RDB_FOREIGN_VERSION_MIN && rdbver <= RDB_FOREIGN_VERSION_MAX;
62+
}
63+
6064
/* Defines related to the dump file format. To store 32 bits lengths for short
6165
* keys requires a lot of space, so we check the most significant 2 bits of
6266
* the first byte to interpreter the length:
@@ -112,11 +116,16 @@ enum RdbType {
112116
RDB_TYPE_STREAM_LISTPACKS_2 = 19,
113117
RDB_TYPE_SET_LISTPACK = 20,
114118
RDB_TYPE_STREAM_LISTPACKS_3 = 21,
115-
RDB_TYPE_HASH_2 = 22,
119+
RDB_TYPE_HASH_2 = 22, /* Hash with field-level expiration (Valkey 9.0) */
116120
RDB_TYPE_LAST
117121
};
118122
/* NOTE: WHEN ADDING NEW RDB TYPE, UPDATE rdb_type_string[] */
119123

124+
/* When our RDB format diverges, we need to reject types/opcodes for which we
125+
* may have assigned a different meaning compared to other implementations. */
126+
#define RDB_FOREIGN_TYPE_MIN 22
127+
#define RDB_FOREIGN_TYPE_MAX 243
128+
120129
typedef int (*ChildSnapshotFunc)(int req, rio *rdb, void *privdata);
121130
typedef struct rdbSnapshotOptions {
122131
int connsnum; /* Number of connections. */
@@ -131,7 +140,9 @@ typedef struct rdbSnapshotOptions {
131140
/* Test if a type is an object type. */
132141
#define rdbIsObjectType(t) (((t) >= 0 && (t) <= 7) || ((t) >= 9 && (t) < RDB_TYPE_LAST))
133142

134-
/* Special RDB opcodes (saved/loaded with rdbSaveType/rdbLoadType). */
143+
/* Special RDB opcodes (saved/loaded with rdbSaveType/rdbLoadType).
144+
* These are special RDB types, but they start from 255 and grow down. */
145+
#define RDB_OPCODE_SLOT_INFO 244 /* Foreign slot info, safe to ignore. */
135146
#define RDB_OPCODE_FUNCTION2 245 /* function library data */
136147
#define RDB_OPCODE_FUNCTION_PRE_GA 246 /* old function library data for 7.0 rc1 and rc2 */
137148
#define RDB_OPCODE_MODULE_AUX 247 /* Module auxiliary data. */
@@ -168,9 +179,11 @@ typedef struct rdbSnapshotOptions {
168179

169180
/* When rdbLoadObject() returns NULL, the err flag is
170181
* set to hold the type of error that occurred */
171-
#define RDB_LOAD_ERR_EMPTY_KEY 1 /* Error of empty key */
172-
#define RDB_LOAD_ERR_OTHER 2 /* Any other errors */
182+
#define RDB_LOAD_ERR_EMPTY_KEY 1 /* Error of empty key */
183+
#define RDB_LOAD_ERR_UNKNOWN_TYPE 2 /* Unknown type in file */
184+
#define RDB_LOAD_ERR_OTHER 3 /* Any other errors */
173185

186+
bool rdbIsVersionAccepted(int rdbver, bool is_valkey_magic, bool is_redis_magic);
174187
ssize_t rdbWriteRaw(rio *rdb, void *p, size_t len);
175188
int rdbSaveType(rio *rdb, unsigned char type);
176189
int rdbLoadType(rio *rdb);

‎src/valkey-check-rdb.c‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ struct {
7676
rio *rio;
7777
robj *key; /* Current key we are reading. */
7878
int key_type; /* Current key type if != -1. */
79+
int rdbver; /* RDB version. */
7980
unsigned long keys; /* Number of keys processed. */
8081
unsigned long expires; /* Number of keys with an expire. */
8182
unsigned long already_expired; /* Number of keys already expired. */
@@ -625,13 +626,17 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) {
625626
goto err;
626627
}
627628
rdbver = atoi(buf + 6);
628-
if (rdbver < 1 || rdbver > RDB_VERSION ||
629+
if (rdbver < 1 ||
629630
(rdbver < RDB_FOREIGN_VERSION_MIN && !is_redis_magic) ||
630-
(rdbver >= RDB_FOREIGN_VERSION_MIN && rdbver <= RDB_FOREIGN_VERSION_MAX) ||
631631
(rdbver > RDB_FOREIGN_VERSION_MAX && !is_valkey_magic)) {
632632
rdbCheckError("Can't handle RDB format version %d", rdbver);
633633
goto err;
634+
} else if (rdbver > RDB_VERSION) {
635+
rdbCheckInfo("Future RDB version %d detected", rdbver);
636+
} else if (rdbIsForeignVersion(rdbver)) {
637+
rdbCheckInfo("Foreign RDB version %d detected", rdbver);
634638
}
639+
rdbstate.rdbver = rdbver;
635640

636641
expiretime = -1;
637642
while (1) {
@@ -689,6 +694,12 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) {
689694
if ((db_size = rdbLoadLen(&rdb, NULL)) == RDB_LENERR) goto eoferr;
690695
if ((expires_size = rdbLoadLen(&rdb, NULL)) == RDB_LENERR) goto eoferr;
691696
continue; /* Read type again. */
697+
} else if (type == RDB_OPCODE_SLOT_INFO) {
698+
/* Hint used in foreign RDB versions. */
699+
if (rdbLoadLen(&rdb, NULL) == RDB_LENERR) goto eoferr;
700+
if (rdbLoadLen(&rdb, NULL) == RDB_LENERR) goto eoferr;
701+
if (rdbLoadLen(&rdb, NULL) == RDB_LENERR) goto eoferr;
702+
continue; /* Read type again. */
692703
} else if (type == RDB_OPCODE_AUX) {
693704
/* AUX: generic string-string fields. Use to add state to RDB
694705
* which is backward compatible. Implementations of RDB loading
@@ -742,11 +753,19 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) {
742753
}
743754
rdbstate.functions_num++;
744755
continue;
745-
} else {
746-
if (!rdbIsObjectType(type)) {
756+
} else if (rdbIsForeignVersion(rdbver) &&
757+
type >= RDB_FOREIGN_TYPE_MIN &&
758+
type <= RDB_FOREIGN_TYPE_MAX) {
759+
rdbCheckError("Unknown object type %d in RDB file with foreign version %d", type, rdbver);
760+
goto err;
761+
} else if (!rdbIsObjectType(type)) {
762+
if (rdbver > RDB_VERSION) {
763+
rdbCheckError("Unknown object type %d in RDB file with future version %d", type, rdbver);
764+
} else {
747765
rdbCheckError("Invalid object type: %d", type);
748-
goto err;
749766
}
767+
goto err;
768+
} else {
750769
rdbstate.key_type = type;
751770
}
752771

@@ -894,8 +913,13 @@ int redis_check_rdb_main(int argc, char **argv, FILE *fp) {
894913
rdbCheckInfo("Checking RDB file %s", argv[1]);
895914
rdbCheckSetupSignals();
896915
int retval = redis_check_rdb(argv[1], fp);
916+
rdbCheckInfo("Check RDB returned error code %d (0 means success)", retval);
897917
if (retval == 0) {
898-
rdbCheckInfo("\\o/ RDB looks OK! \\o/");
918+
if (rdbIsForeignVersion(rdbstate.rdbver) || rdbstate.rdbver > RDB_VERSION) {
919+
rdbCheckInfo("\\o/ RDB looks OK, but loading requires config 'rdb-version-check relaxed'");
920+
} else {
921+
rdbCheckInfo("\\o/ RDB looks OK! \\o/");
922+
}
899923
rdbShowGenericInfo();
900924
}
901925
if (fp) return (retval == 0) ? C_OK : C_ERR;

‎tests/assets/encodings-rdb12.rdb‎

109 Bytes
Binary file not shown.

‎tests/assets/encodings-rdb75-unknown-types.rdb‎

109 Bytes
Binary file not shown.

‎tests/assets/encodings-rdb987-unknown-types.rdb‎

109 Bytes
Binary file not shown.

‎tests/integration/rdb.tcl‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ set server_path [tmpdir "server.rdb-encoding-test"]
1515

1616
# Copy RDB with different encodings in server path
1717
exec cp tests/assets/encodings.rdb $server_path
18+
exec cp tests/assets/encodings-rdb12.rdb $server_path
19+
exec cp tests/assets/encodings-rdb75-unknown-types.rdb $server_path
1820
exec cp tests/assets/encodings-rdb987.rdb $server_path
21+
exec cp tests/assets/encodings-rdb987-unknown-types.rdb $server_path
1922
exec cp tests/assets/list-quicklist.rdb $server_path
2023

2124
start_server [list overrides [list "dir" $server_path "dbfilename" "list-quicklist.rdb" save ""]] {
@@ -55,7 +58,7 @@ start_server_and_kill_it [list "dir" $server_path "dbfilename" "encodings-rdb987
5558
[string match {*Fatal error loading*} \
5659
[exec tail -1 < [dict get $srv stdout]]]
5760
} else {
58-
fail "Server started even if RDB version check failed"
61+
fail "Server started even though RDB version is unsupported"
5962
}
6063
}
6164
}
@@ -69,6 +72,42 @@ start_server [list overrides [list "dir" $server_path \
6972
} $csv_dump
7073
}
7174

75+
start_server_and_kill_it [list dir $server_path \
76+
dbfilename "encodings-rdb987-unknown-types.rdb" \
77+
rdb-version-check relaxed] {
78+
test "RDB future version loading with unknown types, relaxed version check" {
79+
wait_for_condition 50 100 {
80+
[string match {*Unknown type or opcode when loading DB. Unrecoverable error, aborting now.*} \
81+
[exec tail -2 < [dict get $srv stdout]]]
82+
} else {
83+
fail "Server started even though RDB contains unknown types"
84+
}
85+
}
86+
}
87+
88+
start_server [list overrides [list dir $server_path \
89+
dbfilename "encodings-rdb12.rdb" \
90+
rdb-version-check relaxed]] {
91+
test "RDB foreign version loading, relaxed version check" {
92+
r select 0
93+
assert_equal foo [r keys *]
94+
assert_equal bar [r get foo]
95+
}
96+
}
97+
98+
start_server_and_kill_it [list dir $server_path \
99+
dbfilename "encodings-rdb75-unknown-types.rdb" \
100+
rdb-version-check relaxed] {
101+
test "RDB foreign version loading with unknown types, relaxed version check" {
102+
wait_for_condition 50 100 {
103+
[string match {*Can't handle foreign type or opcode 150 in RDB with version 75*} \
104+
[exec tail -2 < [dict get $srv stdout]]]
105+
} else {
106+
fail "Server started even though RDB contains unknown types"
107+
}
108+
}
109+
}
110+
72111
set server_path [tmpdir "server.rdb-startup-test"]
73112

74113
start_server [list overrides [list "dir" $server_path] keep_persistence true] {

0 commit comments

Comments
 (0)