From d6f19539d2414bce1b94af9f814ce09adef6d5f2 Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 27 Nov 2023 23:26:33 +0800 Subject: [PATCH] Un-register notification and server event when RedisModule_OnLoad fails (#12809) When we register notification or server event in RedisModule_OnLoad, but RedisModule_OnLoad eventually fails, triggering notification or server event will cause the server to crash. If the loading fails on a later stage of moduleLoad, we do call moduleUnload which handles all un-registration, but when it fails on the RedisModule_OnLoad call, we only un-register several specific things and these were missing: - moduleUnsubscribeNotifications - moduleUnregisterFilters - moduleUnsubscribeAllServerEvents Refactored the code to reuse the code from moduleUnload. Fixes #12808. --- src/module.c | 33 +++++++++++------------- tests/modules/commandfilter.c | 17 +++++++++--- tests/modules/hooks.c | 16 +++++++++--- tests/modules/keyspace_events.c | 14 +++++++--- tests/unit/moduleapi/commandfilter.tcl | 14 ++++++++-- tests/unit/moduleapi/hooks.tcl | 8 ++++++ tests/unit/moduleapi/keyspace_events.tcl | 13 ++++++++++ 7 files changed, 86 insertions(+), 29 deletions(-) diff --git a/src/module.c b/src/module.c index b33192e08..96bc61e0f 100644 --- a/src/module.c +++ b/src/module.c @@ -12150,6 +12150,19 @@ int parseLoadexArguments(RedisModuleString ***module_argv, int *module_argc) { return REDISMODULE_OK; } +/* Unregister module-related things, called when moduleLoad fails or moduleUnload. */ +void moduleUnregisterCleanup(RedisModule *module) { + moduleFreeAuthenticatedClients(module); + moduleUnregisterCommands(module); + moduleUnsubscribeNotifications(module); + moduleUnregisterSharedAPI(module); + moduleUnregisterUsedAPI(module); + moduleUnregisterFilters(module); + moduleUnsubscribeAllServerEvents(module); + moduleRemoveConfigs(module); + moduleUnregisterAuthCBs(module); +} + /* Load a module and initialize it. On success C_OK is returned, otherwise * C_ERR is returned. */ int moduleLoad(const char *path, void **module_argv, int module_argc, int is_loadex) { @@ -12184,12 +12197,8 @@ int moduleLoad(const char *path, void **module_argv, int module_argc, int is_loa serverLog(LL_WARNING, "Module %s initialization failed. Module not loaded",path); if (ctx.module) { - moduleUnregisterCommands(ctx.module); - moduleUnregisterSharedAPI(ctx.module); - moduleUnregisterUsedAPI(ctx.module); + moduleUnregisterCleanup(ctx.module); moduleRemoveCateogires(ctx.module); - moduleRemoveConfigs(ctx.module); - moduleUnregisterAuthCBs(ctx.module); moduleFreeModuleStructure(ctx.module); } moduleFreeContext(&ctx); @@ -12230,8 +12239,6 @@ int moduleLoad(const char *path, void **module_argv, int module_argc, int is_loa } if (post_load_err) { - /* Unregister module auth callbacks (if any exist) that this Module registered onload. */ - moduleUnregisterAuthCBs(ctx.module); moduleUnload(ctx.module->name, NULL); moduleFreeContext(&ctx); return C_ERR; @@ -12289,17 +12296,7 @@ int moduleUnload(sds name, const char **errmsg) { } } - moduleFreeAuthenticatedClients(module); - moduleUnregisterCommands(module); - moduleUnregisterSharedAPI(module); - moduleUnregisterUsedAPI(module); - moduleUnregisterFilters(module); - moduleUnregisterAuthCBs(module); - moduleRemoveConfigs(module); - - /* Remove any notification subscribers this module might have */ - moduleUnsubscribeNotifications(module); - moduleUnsubscribeAllServerEvents(module); + moduleUnregisterCleanup(module); /* Unload the dynamic library. */ if (dlclose(module->handle) == -1) { diff --git a/tests/modules/commandfilter.c b/tests/modules/commandfilter.c index e44f6eb6e..56e517ae3 100644 --- a/tests/modules/commandfilter.c +++ b/tests/modules/commandfilter.c @@ -1,6 +1,7 @@ #include "redismodule.h" #include +#include static RedisModuleString *log_key_name; @@ -92,7 +93,7 @@ int CommandFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int return REDISMODULE_OK; } -int CommandFilter_UnfilteredClientdId(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +int CommandFilter_UnfilteredClientId(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { if (argc < 2) return RedisModule_WrongArity(ctx); @@ -192,7 +193,7 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if (RedisModule_Init(ctx,"commandfilter",1,REDISMODULE_APIVER_1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if (argc != 2) { + if (argc != 2 && argc != 3) { RedisModule_Log(ctx, "warning", "Log key name not specified"); return REDISMODULE_ERR; } @@ -219,7 +220,7 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) return REDISMODULE_ERR; if (RedisModule_CreateCommand(ctx, unfiltered_clientid_name, - CommandFilter_UnfilteredClientdId, "admin", 1,1,1) == REDISMODULE_ERR) + CommandFilter_UnfilteredClientId, "admin", 1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; if ((filter = RedisModule_RegisterCommandFilter(ctx, CommandFilter_CommandFilter, @@ -229,6 +230,16 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if ((filter1 = RedisModule_RegisterCommandFilter(ctx, CommandFilter_BlmoveSwap, 0)) == NULL) return REDISMODULE_ERR; + if (argc == 3) { + const char *ptr = RedisModule_StringPtrLen(argv[2], NULL); + if (!strcasecmp(ptr, "noload")) { + /* This is a hint that we return ERR at the last moment of OnLoad. */ + RedisModule_FreeString(ctx, log_key_name); + if (retained) RedisModule_FreeString(NULL, retained); + return REDISMODULE_ERR; + } + } + return REDISMODULE_OK; } diff --git a/tests/modules/hooks.c b/tests/modules/hooks.c index e0ff0c136..fc357d144 100644 --- a/tests/modules/hooks.c +++ b/tests/modules/hooks.c @@ -33,6 +33,7 @@ #include "redismodule.h" #include #include +#include #include /* We need to store events to be able to test and see what we got, and we can't @@ -407,9 +408,6 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) return REDISMODULE_ERR; \ } - REDISMODULE_NOT_USED(argv); - REDISMODULE_NOT_USED(argc); - if (RedisModule_Init(ctx,"testhook",1,REDISMODULE_APIVER_1) == REDISMODULE_ERR) return REDISMODULE_ERR; @@ -471,6 +469,18 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if (RedisModule_CreateCommand(ctx,"hooks.pexpireat", cmdKeyExpiry,"",0,0,0) == REDISMODULE_ERR) return REDISMODULE_ERR; + if (argc == 1) { + const char *ptr = RedisModule_StringPtrLen(argv[0], NULL); + if (!strcasecmp(ptr, "noload")) { + /* This is a hint that we return ERR at the last moment of OnLoad. */ + RedisModule_FreeDict(ctx, event_log); + RedisModule_FreeDict(ctx, removed_event_log); + RedisModule_FreeDict(ctx, removed_subevent_type); + RedisModule_FreeDict(ctx, removed_expiry_log); + return REDISMODULE_ERR; + } + } + return REDISMODULE_OK; } diff --git a/tests/modules/keyspace_events.c b/tests/modules/keyspace_events.c index 46eb688a5..1a284b50f 100644 --- a/tests/modules/keyspace_events.c +++ b/tests/modules/keyspace_events.c @@ -36,6 +36,7 @@ #include "redismodule.h" #include #include +#include #include ustime_t cached_time = 0; @@ -318,9 +319,6 @@ static int cmdGetDels(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { /* This function must be present on each Redis module. It is used in order to * register the commands into the Redis server. */ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { - REDISMODULE_NOT_USED(argv); - REDISMODULE_NOT_USED(argc); - if (RedisModule_Init(ctx,"testkeyspace",1,REDISMODULE_APIVER_1) == REDISMODULE_ERR){ return REDISMODULE_ERR; } @@ -405,6 +403,16 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) return REDISMODULE_ERR; } + if (argc == 1) { + const char *ptr = RedisModule_StringPtrLen(argv[0], NULL); + if (!strcasecmp(ptr, "noload")) { + /* This is a hint that we return ERR at the last moment of OnLoad. */ + RedisModule_FreeDict(ctx, loaded_event_log); + RedisModule_FreeDict(ctx, module_event_log); + return REDISMODULE_ERR; + } + } + return REDISMODULE_OK; } diff --git a/tests/unit/moduleapi/commandfilter.tcl b/tests/unit/moduleapi/commandfilter.tcl index 99f9a4dd9..72b16ec97 100644 --- a/tests/unit/moduleapi/commandfilter.tcl +++ b/tests/unit/moduleapi/commandfilter.tcl @@ -95,7 +95,7 @@ start_server {tags {"modules"}} { test "Unload the module - commandfilter" { assert_equal {OK} [r module unload commandfilter] } -} +} test {RM_CommandFilterArgInsert and script argv caching} { # coverage for scripts calling commands that expand the argv array @@ -162,4 +162,14 @@ test {Filtering based on client id} { $rr close } -} \ No newline at end of file +} + +start_server {} { + test {OnLoad failure will handle un-registration} { + catch {r module load $testmodule log-key 0 noload} + r set mykey @log + assert_equal [r lrange log-key 0 -1] {} + r rpush mylist elem1 @delme elem2 + assert_equal [r lrange mylist 0 -1] {elem1 @delme elem2} + } +} diff --git a/tests/unit/moduleapi/hooks.tcl b/tests/unit/moduleapi/hooks.tcl index 6f9bc3bec..94b0f6f31 100644 --- a/tests/unit/moduleapi/hooks.tcl +++ b/tests/unit/moduleapi/hooks.tcl @@ -310,4 +310,12 @@ tags "modules" { assert_equal [string match {*module-event-shutdown*} [exec tail -5 < $replica_stdout]] 1 } } + + start_server {} { + test {OnLoad failure will handle un-registration} { + catch {r module load $testmodule noload} + r flushall + r ping + } + } } diff --git a/tests/unit/moduleapi/keyspace_events.tcl b/tests/unit/moduleapi/keyspace_events.tcl index 19c712052..1323b1296 100644 --- a/tests/unit/moduleapi/keyspace_events.tcl +++ b/tests/unit/moduleapi/keyspace_events.tcl @@ -102,4 +102,17 @@ tags "modules" { assert_equal {OK} [r set x 1 EX 1] } } + + start_server {} { + test {OnLoad failure will handle un-registration} { + catch {r module load $testmodule noload} + r set x 1 + r hset y f v + r lpush z 1 2 3 + r sadd p 1 2 3 + r zadd t 1 f1 2 f2 + r xadd s * f v + r ping + } + } }