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

@@ -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) {

tests/modules/commandfilter.c

@@ -1,6 +1,7 @@
 #include "redismodule.h"
 
 #include <string.h>
+#include <strings.h>
 
 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;
 }
 

tests/modules/hooks.c

@@ -33,6 +33,7 @@
 #include "redismodule.h"
 #include <stdio.h>
 #include <string.h>
+#include <strings.h>
 #include <assert.h>
 
 /* 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;
 }
 

tests/modules/keyspace_events.c

@@ -36,6 +36,7 @@
 #include "redismodule.h"
 #include <stdio.h>
 #include <string.h>
+#include <strings.h>
 #include <unistd.h>
 
 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;
 }
 

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
     }
-}
+}
+
+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}
+    }
+}

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
+        }
+    }
 }

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
+        }
+    }
 }