Skip to content

Commit

Permalink
Un-register notification and server event when RedisModule_OnLoad fails
Browse files Browse the repository at this point in the history
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 two are missing.

Fixes redis#12808.
  • Loading branch information
enjoy-binbin committed Nov 26, 2023
1 parent c9aa586 commit 7fa030d
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 6 deletions.
2 changes: 2 additions & 0 deletions src/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -12190,6 +12190,8 @@ int moduleLoad(const char *path, void **module_argv, int module_argc, int is_loa
moduleRemoveCateogires(ctx.module);
moduleRemoveConfigs(ctx.module);
moduleUnregisterAuthCBs(ctx.module);
moduleUnsubscribeNotifications(ctx.module);
moduleUnsubscribeAllServerEvents(ctx.module);
moduleFreeModuleStructure(ctx.module);
}
moduleFreeContext(&ctx);
Expand Down
15 changes: 12 additions & 3 deletions tests/modules/hooks.c
Original file line number Diff line number Diff line change
Expand Up @@ -407,9 +407,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;

Expand Down Expand Up @@ -471,6 +468,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;
}

Expand Down
13 changes: 10 additions & 3 deletions tests/modules/keyspace_events.c
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,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;
}
Expand Down Expand Up @@ -405,6 +402,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;
}

Expand Down
8 changes: 8 additions & 0 deletions tests/unit/moduleapi/hooks.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
13 changes: 13 additions & 0 deletions tests/unit/moduleapi/keyspace_events.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}

0 comments on commit 7fa030d

Please sign in to comment.