From 74a6e48a3d2a86402cd653eb9e6626fdb78fedd8 Mon Sep 17 00:00:00 2001
From: Binbin <binloveplay1314@qq.com>
Date: Wed, 31 Jan 2024 15:28:50 +0800
Subject: [PATCH] Fix module unblock crash due to no timeout_callback (#13017)

The block timeout is passed in the test case, but we do not pass
in the timeout_callback, and it will crash when unlocking. In this
case, in moduleBlockedClientTimedOut we will check timeout_callback.
There is the stack:
```
beforeSleep
blockedBeforeSleep
handleBlockedClientsTimeout
checkBlockedClientTimeout
unblockClientOnTimeout
replyToBlockedClientTimedOut
moduleBlockedClientTimedOut
-- timeout_callback is NULL, invalidFunctionWasCalled
bc->timeout_callback(&ctx,(void**)c->argv,c->argc);
```
---
 src/module.c                           |  7 ++++++-
 tests/modules/blockedclient.c          | 11 +++++++++--
 tests/unit/moduleapi/blockedclient.tcl | 12 +++++++++++-
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/module.c b/src/module.c
index cd90a76aa..b18377156 100644
--- a/src/module.c
+++ b/src/module.c
@@ -8438,7 +8438,12 @@ void moduleBlockedClientTimedOut(client *c, int from_module) {
     if (!from_module)
         prev_error_replies = server.stat_total_error_replies;
 
-    bc->timeout_callback(&ctx,(void**)c->argv,c->argc);
+    if (bc->timeout_callback) {
+        /* In theory, the user should always pass the timeout handler as an
+         * argument, but better to be safe than sorry. */
+        bc->timeout_callback(&ctx,(void**)c->argv,c->argc);
+    }
+
     moduleFreeContext(&ctx);
 
     if (!from_module)
diff --git a/tests/modules/blockedclient.c b/tests/modules/blockedclient.c
index 23030cef4..4a59623fd 100644
--- a/tests/modules/blockedclient.c
+++ b/tests/modules/blockedclient.c
@@ -629,16 +629,23 @@ static void timer_callback(RedisModuleCtx *ctx, void *data)
     RedisModule_FreeThreadSafeContext(reply_ctx);
 }
 
+/* unblock_by_timer <period_ms> <timeout_ms>
+ * period_ms is the period of the timer.
+ * timeout_ms is the blocking timeout. */
 int unblock_by_timer(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
 {
-    if (argc != 2)
+    if (argc != 3)
         return RedisModule_WrongArity(ctx);
 
     long long period;
+    long long timeout;
     if (RedisModule_StringToLongLong(argv[1],&period) != REDISMODULE_OK)
         return RedisModule_ReplyWithError(ctx,"ERR invalid period");
+    if (RedisModule_StringToLongLong(argv[2],&timeout) != REDISMODULE_OK) {
+        return RedisModule_ReplyWithError(ctx,"ERR invalid timeout");
+    }
 
-    RedisModuleBlockedClient *bc = RedisModule_BlockClient(ctx, NULL, NULL, NULL, 0);
+    RedisModuleBlockedClient *bc = RedisModule_BlockClient(ctx, NULL, NULL, NULL, timeout);
     RedisModule_CreateTimer(ctx, period, timer_callback, bc);
     return REDISMODULE_OK;
 }
diff --git a/tests/unit/moduleapi/blockedclient.tcl b/tests/unit/moduleapi/blockedclient.tcl
index 9d475ebc0..fdacb49b9 100644
--- a/tests/unit/moduleapi/blockedclient.tcl
+++ b/tests/unit/moduleapi/blockedclient.tcl
@@ -278,7 +278,17 @@ foreach call_type {nested normal} {
     }
 
     test {Unblock by timer} {
-        assert_match "OK" [r unblock_by_timer 100]
+        # When the client is unlock, we will get the OK reply.
+        assert_match "OK" [r unblock_by_timer 100 0]
+    }
+
+    test {block time is shorter than timer period} {
+        # This command does not have the reply.
+        set rd [redis_deferring_client]
+        $rd unblock_by_timer 100 10
+        # Wait for the client to unlock.
+        after 120
+        $rd close
     }
     
     test "Unload the module - blockedclient" {