From 15e2b68214042c4a72a4f793799420fac59f7b4c Mon Sep 17 00:00:00 2001 From: Bonsai Date: Tue, 6 Apr 2021 17:09:36 +0800 Subject: [PATCH] Fix: server will crash if rdbload or rdbsave method is not provided in module (#8670) With this fix, module data type registration will fail if the load or save callbacks are not defined, or the optional aux load and save callbacks are not either both defined or both missing. --- src/module.c | 20 +++++++++++++++----- tests/modules/datatype.c | 5 +++++ tests/modules/defragtest.c | 17 +++++++++++++++++ tests/modules/testrdb.c | 9 +++++++-- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/module.c b/src/module.c index f24c0af2a..ff36a9f86 100644 --- a/src/module.c +++ b/src/module.c @@ -4374,9 +4374,9 @@ robj *moduleTypeDupOrReply(client *c, robj *fromkey, robj *tokey, robj *value) { * .defrag = myType_DefragCallback * } * - * * **rdb_load**: A callback function pointer that loads data from RDB files. - * * **rdb_save**: A callback function pointer that saves data to RDB files. - * * **aof_rewrite**: A callback function pointer that rewrites data as commands. + * * **rdb_load**: A callback function pointer that loads data from RDB files. (mandatory) + * * **rdb_save**: A callback function pointer that saves data to RDB files. (mandatory) + * * **aof_rewrite**: A callback function pointer that rewrites data as commands. (mandatory) * * **digest**: A callback function pointer that is used for `DEBUG DIGEST`. * * **free**: A callback function pointer that can free a type value. * * **aux_save**: A callback function pointer that saves out of keyspace data to RDB files. @@ -4416,8 +4416,8 @@ robj *moduleTypeDupOrReply(client *c, robj *fromkey, robj *tokey, robj *value) { * Note: the module name "AAAAAAAAA" is reserved and produces an error, it * happens to be pretty lame as well. * - * If there is already a module registering a type with the same name, - * and if the module name or encver is invalid, NULL is returned. + * If there is already a module registering a type with the same name, the name + * or encver are invalid, or a mandatory callback is missing, NULL is returned. * Otherwise the new type is registered into Redis, and a reference of * type RedisModuleType is returned: the caller of the function should store * this reference into a global variable to make future use of it in the @@ -4460,6 +4460,16 @@ moduleType *RM_CreateDataType(RedisModuleCtx *ctx, const char *name, int encver, } v3; } *tms = (struct typemethods*) typemethods_ptr; + /* Persistence functions can't be NULL */ + if (tms->rdb_load == NULL || tms->rdb_save == NULL || tms->aof_rewrite == NULL) { + return NULL; + } + + /* Function aux_load and aux_save must be either both defined or undefined. */ + if ((tms->version >= 2) && ((tms->v2.aux_load != NULL) != (tms->v2.aux_save != NULL))) { + return NULL; + } + moduleType *mt = zcalloc(sizeof(*mt)); mt->id = id; mt->module = ctx->module; diff --git a/tests/modules/datatype.c b/tests/modules/datatype.c index 0c6f95551..8c3ef2982 100644 --- a/tests/modules/datatype.c +++ b/tests/modules/datatype.c @@ -32,6 +32,10 @@ static void datatype_save(RedisModuleIO *io, void *value) { RedisModule_SaveString(io, dt->strval); } +static void datatype_rewrite(RedisModuleIO *aof, RedisModuleString *key, void *value) { + /* do-nothing callback. */ +} + static void datatype_free(void *value) { if (value) { DataType *dt = (DataType *) value; @@ -190,6 +194,7 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) .version = REDISMODULE_TYPE_METHOD_VERSION, .rdb_load = datatype_load, .rdb_save = datatype_save, + .aof_rewrite = datatype_rewrite, .free = datatype_free, .copy = datatype_copy }; diff --git a/tests/modules/defragtest.c b/tests/modules/defragtest.c index b63680c63..af11a905e 100644 --- a/tests/modules/defragtest.c +++ b/tests/modules/defragtest.c @@ -25,6 +25,20 @@ unsigned long int global_defragged = 0; int global_strings_len = 0; RedisModuleString **global_strings = NULL; +static void *fragtype_load(RedisModuleIO *io, int encver) { + /* do-nothing callback. */ + return NULL; +} + +static void fragtype_save(RedisModuleIO *io, void *value) { + /* do-nothing callback. */ +} + +static void fragtype_rewrite(RedisModuleIO *aof, RedisModuleString *key, void *value) { + /* do-nothing callback. */ +} + + static void createGlobalStrings(RedisModuleCtx *ctx, int count) { global_strings_len = count; @@ -211,6 +225,9 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) RedisModuleTypeMethods tm = { .version = REDISMODULE_TYPE_METHOD_VERSION, + .rdb_load = fragtype_load, + .rdb_save = fragtype_save, + .aof_rewrite = fragtype_rewrite, .free = FragFree, .free_effort = FragFreeEffort, .defrag = FragDefrag diff --git a/tests/modules/testrdb.c b/tests/modules/testrdb.c index 5d9382a3a..9f08f4f36 100644 --- a/tests/modules/testrdb.c +++ b/tests/modules/testrdb.c @@ -79,6 +79,11 @@ void testrdb_type_save(RedisModuleIO *rdb, void *value) { RedisModule_SaveLongDouble(rdb, 0.333333333333333333L); } + +void testrdb_type_aof_rewrite(RedisModuleIO *aof, RedisModuleString *key, void *value) { + /* do-nothing callback. */ +} + void testrdb_aux_save(RedisModuleIO *rdb, int when) { if (conf_aux_count==1) assert(when == REDISMODULE_AUX_AFTER_RDB); if (conf_aux_count==0) assert(0); @@ -240,7 +245,7 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) .version = 1, .rdb_load = testrdb_type_load, .rdb_save = testrdb_type_save, - .aof_rewrite = NULL, + .aof_rewrite = testrdb_type_aof_rewrite, .digest = NULL, .free = testrdb_type_free, }; @@ -253,7 +258,7 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) .version = REDISMODULE_TYPE_METHOD_VERSION, .rdb_load = testrdb_type_load, .rdb_save = testrdb_type_save, - .aof_rewrite = NULL, + .aof_rewrite = testrdb_type_aof_rewrite, .digest = NULL, .free = testrdb_type_free, .aux_load = testrdb_aux_load,