Prevent double-free of the module timer when the callback stops it

src/module.c

@@ -9919,14 +9919,26 @@ long long moduleTimerHandler(struct aeEventLoop *eventLoop, long long id, void *
         memcpy(&expiretime, ri.key, sizeof(expiretime));
         expiretime = ntohu64(expiretime);
         if (now >= expiretime) {
+            /* Preserve the timer ID before invoking the callback. The callback
+             * may call ValkeyModule_StopTimer() on the currently firing timer,
+             * which removes the radix-tree entry and frees the timer object.
+             * Also, other tree mutations in the callback can invalidate ri.key. */
+            ValkeyModuleTimerID current_id;
+            memcpy(&current_id, ri.key, sizeof(current_id));
+
             ValkeyModuleTimer *timer = ri.data;
             ValkeyModuleCtx ctx;
             moduleCreateContext(&ctx, timer->module, VALKEYMODULE_CTX_TEMP_CLIENT);
             selectDb(ctx.client, timer->dbid);
             timer->callback(&ctx, timer->data);
             moduleFreeContext(&ctx);
-            raxRemove(Timers, (unsigned char *)ri.key, ri.key_len, NULL);
-            zfree(timer);
+
+            /* Skip cleanup if the callback already stopped this timer. */
+            void *live = NULL;
+            if (raxFind(Timers, (unsigned char *)&current_id, sizeof(current_id), &live) && live == timer) {
+                raxRemove(Timers, (unsigned char *)&current_id, sizeof(current_id), NULL);
+                zfree(timer);
+            }
         } else {
             /* We call ustime() again instead of using the cached 'now' so that
              * 'next_period' isn't affected by the time it took to execute

tests/modules/timer.c

@@ -1,6 +1,11 @@
 
 #include "valkeymodule.h"
 
+typedef struct {
+    ValkeyModuleTimerID id;
+    ValkeyModuleString *keyname;
+} selfstopTimerData;
+
 static void timer_callback(ValkeyModuleCtx *ctx, void *data)
 {
     ValkeyModuleString *keyname = data;
@@ -12,6 +17,25 @@ static void timer_callback(ValkeyModuleCtx *ctx, void *data)
     ValkeyModule_FreeString(ctx, keyname);
 }
 
+/* Stops the currently firing timer from inside its own callback.
+ * This used to double-free the ValkeyModuleTimer in moduleTimerHandler(). */
+static void selfstop_timer_callback(ValkeyModuleCtx *ctx, void *data)
+{
+    selfstopTimerData *d = data;
+    ValkeyModuleCallReply *reply;
+    void *timer_data = NULL;
+
+    if (ValkeyModule_StopTimer(ctx, d->id, &timer_data) == VALKEYMODULE_OK) {
+        /* timer_data should be our private data; ownership stays with us. */
+    }
+
+    reply = ValkeyModule_Call(ctx, "INCR", "s", d->keyname);
+    if (reply != NULL)
+        ValkeyModule_FreeCallReply(reply);
+    ValkeyModule_FreeString(ctx, d->keyname);
+    ValkeyModule_Free(d);
+}
+
 int test_createtimer(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc)
 {
     if (argc != 3) {
@@ -84,6 +108,30 @@ int test_stoptimer(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc)
     return VALKEYMODULE_OK;
 }
 
+int test_selfstoptimer(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc)
+{
+    if (argc != 3) {
+        ValkeyModule_WrongArity(ctx);
+        return VALKEYMODULE_OK;
+    }
+
+    long long period;
+    if (ValkeyModule_StringToLongLong(argv[1], &period) == VALKEYMODULE_ERR) {
+        ValkeyModule_ReplyWithError(ctx, "Invalid time specified.");
+        return VALKEYMODULE_OK;
+    }
+
+    ValkeyModuleString *keyname = argv[2];
+    ValkeyModule_RetainString(ctx, keyname);
+
+    selfstopTimerData *d = ValkeyModule_Alloc(sizeof(*d));
+    d->keyname = keyname;
+    d->id = ValkeyModule_CreateTimer(ctx, period, selfstop_timer_callback, d);
+    ValkeyModule_ReplyWithLongLong(ctx, d->id);
+
+    return VALKEYMODULE_OK;
+}
+
 
 int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
     VALKEYMODULE_NOT_USED(argv);
@@ -97,6 +145,8 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg
         return VALKEYMODULE_ERR;
     if (ValkeyModule_CreateCommand(ctx,"test.stoptimer", test_stoptimer,"",0,0,0) == VALKEYMODULE_ERR)
         return VALKEYMODULE_ERR;
+    if (ValkeyModule_CreateCommand(ctx,"test.selfstoptimer", test_selfstoptimer,"",0,0,0) == VALKEYMODULE_ERR)
+        return VALKEYMODULE_ERR;
 
     return VALKEYMODULE_OK;
 }

tests/unit/moduleapi/timer.tcl

@@ -56,6 +56,24 @@ start_server {tags {"modules"}} {
         assert_equal {} [r test.gettimer $id]
     }
 
+    test {RM_StopTimer: stopping the currently firing timer is safe} {
+        # Regression for double-free when a timer callback calls
+        # ValkeyModule_StopTimer() on its own timer ID.
+        r set "timer-selfstop-key" 0
+        set id [r test.selfstoptimer 10 timer-selfstop-key]
+
+        wait_for_condition 50 100 {
+            [r get timer-selfstop-key] == 1
+        } else {
+            fail "Self-stopping timer callback did not run"
+        }
+
+        # Timer was freed by StopTimer inside the callback; id must be gone
+        # and the server must still be responsive (no double free crash).
+        assert_equal {} [r test.gettimer $id]
+        assert_equal {PONG} [r ping]
+    }
+
     test "Module can be unloaded when timer was finished" {
         r set "timer-incr-key" 0
         r test.createtimer 500 timer-incr-key