From eb6accad40577cd11846c4f5391c6351eb46d7b6 Mon Sep 17 00:00:00 2001 From: "Meir Shpilraien (Spielrein)" Date: Wed, 12 Oct 2022 13:09:51 +0300 Subject: [PATCH] Fix crash on RM_Call inside module load (#11346) PR #9320 introduces initialization order changes. Now cluster is initialized after modules. This changes causes a crash if the module uses RM_Call inside the load function on cluster mode (the code will try to access `server.cluster` which at this point is NULL). To solve it, separate cluster initialization into 2 phases: 1. Structure initialization that happened before the modules initialization 2. Listener initialization that happened after. Test was added to verify the fix. --- src/cluster.c | 45 +++++++++++++++++--------------- src/cluster.h | 1 + src/server.c | 5 +++- tests/modules/basics.c | 22 ++++++++++++++++ tests/unit/moduleapi/basics.tcl | 2 +- tests/unit/moduleapi/cluster.tcl | 14 ++++++++++ 6 files changed, 66 insertions(+), 23 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index b1b4e645d..f7603611f 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -694,27 +694,6 @@ void clusterInit(void) { exit(1); } - if (connectionIndexByType(connTypeOfCluster()->get_type(NULL)) < 0) { - serverLog(LL_WARNING, "Missing connection type %s, but it is required for the Cluster bus.", connTypeOfCluster()->get_type(NULL)); - exit(1); - } - - connListener *listener = &server.clistener; - listener->count = 0; - listener->bindaddr = server.bindaddr; - listener->bindaddr_count = server.bindaddr_count; - listener->port = server.cluster_port ? server.cluster_port : port + CLUSTER_PORT_INCR; - listener->ct = connTypeOfCluster(); - if (connListen(listener) == C_ERR ) { - /* Note: the following log text is matched by the test suite. */ - serverLog(LL_WARNING, "Failed listening on port %u (cluster), aborting.", listener->port); - exit(1); - } - - if (createSocketAcceptHandler(&server.clistener, clusterAcceptHandler) != C_OK) { - serverPanic("Unrecoverable error creating Redis Cluster socket accept handler."); - } - /* Initialize data for the Slot to key API. */ slotToKeyInit(server.db); @@ -733,6 +712,30 @@ void clusterInit(void) { clusterUpdateMyselfHostname(); } +void clusterInitListeners(void) { + if (connectionIndexByType(connTypeOfCluster()->get_type(NULL)) < 0) { + serverLog(LL_WARNING, "Missing connection type %s, but it is required for the Cluster bus.", connTypeOfCluster()->get_type(NULL)); + exit(1); + } + + int port = server.tls_cluster ? server.tls_port : server.port; + connListener *listener = &server.clistener; + listener->count = 0; + listener->bindaddr = server.bindaddr; + listener->bindaddr_count = server.bindaddr_count; + listener->port = server.cluster_port ? server.cluster_port : port + CLUSTER_PORT_INCR; + listener->ct = connTypeOfCluster(); + if (connListen(listener) == C_ERR ) { + /* Note: the following log text is matched by the test suite. */ + serverLog(LL_WARNING, "Failed listening on port %u (cluster), aborting.", listener->port); + exit(1); + } + + if (createSocketAcceptHandler(&server.clistener, clusterAcceptHandler) != C_OK) { + serverPanic("Unrecoverable error creating Redis Cluster socket accept handler."); + } +} + /* Reset a node performing a soft or hard reset: * * 1) All other nodes are forgotten. diff --git a/src/cluster.h b/src/cluster.h index ba1b4edb4..75516716f 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -383,6 +383,7 @@ static_assert(offsetof(clusterMsg, data) == 2256, "unexpected field offset"); /* ---------------------- API exported outside cluster.c -------------------- */ void clusterInit(void); +void clusterInitListeners(void); void clusterCron(void); void clusterBeforeSleep(void); clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, int argc, int *hashslot, int *ask); diff --git a/src/server.c b/src/server.c index cb0faf888..2c65aac66 100644 --- a/src/server.c +++ b/src/server.c @@ -7053,6 +7053,9 @@ int main(int argc, char **argv) { if (server.set_proc_title) redisSetProcTitle(NULL); redisAsciiArt(); checkTcpBacklogSettings(); + if (server.cluster_enabled) { + clusterInit(); + } if (!server.sentinel_mode) { moduleInitModulesSystemLast(); moduleLoadFromQueue(); @@ -7060,7 +7063,7 @@ int main(int argc, char **argv) { ACLLoadUsersAtStartup(); initListeners(); if (server.cluster_enabled) { - clusterInit(); + clusterInitListeners(); } InitServerLast(); diff --git a/tests/modules/basics.c b/tests/modules/basics.c index 55b36fa5e..b6f9766c6 100644 --- a/tests/modules/basics.c +++ b/tests/modules/basics.c @@ -893,6 +893,28 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if (RedisModule_Init(ctx,"test",1,REDISMODULE_APIVER_1) == REDISMODULE_ERR) return REDISMODULE_ERR; + /* Perform RM_Call inside the RedisModule_OnLoad + * to verify that it works as expected without crashing. + * The tests will verify it on different configurations + * options (cluster/no cluster). A simple ping command + * is enough for this test. */ + RedisModuleCallReply *reply = RedisModule_Call(ctx, "ping", ""); + if (RedisModule_CallReplyType(reply) != REDISMODULE_REPLY_STRING) { + RedisModule_FreeCallReply(reply); + return REDISMODULE_ERR; + } + size_t len; + const char *reply_str = RedisModule_CallReplyStringPtr(reply, &len); + if (len != 4) { + RedisModule_FreeCallReply(reply); + return REDISMODULE_ERR; + } + if (memcmp(reply_str, "PONG", 4) != 0) { + RedisModule_FreeCallReply(reply); + return REDISMODULE_ERR; + } + RedisModule_FreeCallReply(reply); + if (RedisModule_CreateCommand(ctx,"test.call", TestCall,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; diff --git a/tests/unit/moduleapi/basics.tcl b/tests/unit/moduleapi/basics.tcl index 040d9eb3c..be606ced0 100644 --- a/tests/unit/moduleapi/basics.tcl +++ b/tests/unit/moduleapi/basics.tcl @@ -38,4 +38,4 @@ start_server {tags {"modules external:skip"} overrides {enable-module-command no test {module command disabled} { assert_error "ERR *MODULE command not allowed*" {r module load $testmodule} } -} \ No newline at end of file +} diff --git a/tests/unit/moduleapi/cluster.tcl b/tests/unit/moduleapi/cluster.tcl index f1bbc8920..c7ec45a71 100644 --- a/tests/unit/moduleapi/cluster.tcl +++ b/tests/unit/moduleapi/cluster.tcl @@ -163,3 +163,17 @@ start_cluster 3 0 [list config_lines $modules] { $node2_rd close } } + +set testmodule [file normalize tests/modules/basics.so] +set modules [list loadmodule $testmodule] +start_cluster 3 0 [list config_lines $modules] { + set node1 [srv 0 client] + set node2 [srv -1 client] + set node3 [srv -2 client] + + test "Verify RM_Call inside module load function on cluster mode" { + assert_equal {PONG} [$node1 PING] + assert_equal {PONG} [$node2 PING] + assert_equal {PONG} [$node3 PING] + } +} \ No newline at end of file