From e31485992a6dccdc8d7cdce1883311d98994138a Mon Sep 17 00:00:00 2001 From: quanyeyang Date: Sun, 19 Jul 2026 13:36:12 +0800 Subject: [PATCH 1/3] Add regression test Signed-off-by: quanyeyang --- tests/modules/timer.c | 50 ++++++++++++++++++++++++++++++++++ tests/unit/moduleapi/timer.tcl | 18 ++++++++++++ 2 files changed, 68 insertions(+) diff --git a/tests/modules/timer.c b/tests/modules/timer.c index f5eea192acf..44033a339c0 100644 --- a/tests/modules/timer.c +++ b/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; } diff --git a/tests/unit/moduleapi/timer.tcl b/tests/unit/moduleapi/timer.tcl index 4e9dd0f0966..40835fbbf8b 100644 --- a/tests/unit/moduleapi/timer.tcl +++ b/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 From e9554c33bad94b15410d310dd3745de42561ad50 Mon Sep 17 00:00:00 2001 From: quanyeyang Date: Sun, 19 Jul 2026 13:36:34 +0800 Subject: [PATCH 2/3] Add fix Signed-off-by: quanyeyang --- src/module.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/module.c b/src/module.c index 6afa5e15fcc..a8f1f9c0ac5 100644 --- a/src/module.c +++ b/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(¤t_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 *)¤t_id, sizeof(current_id), &live) && live == timer) { + raxRemove(Timers, (unsigned char *)¤t_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 From 367cf631e6d0c9524f02b8f2fa0517a7b64c5987 Mon Sep 17 00:00:00 2001 From: quanyeyang Date: Sun, 19 Jul 2026 21:48:05 +0800 Subject: [PATCH 3/3] Strengthen module timer self-stop test Signed-off-by: quanyeyang --- tests/modules/timer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/modules/timer.c b/tests/modules/timer.c index 44033a339c0..fe106c52db7 100644 --- a/tests/modules/timer.c +++ b/tests/modules/timer.c @@ -25,9 +25,8 @@ static void selfstop_timer_callback(ValkeyModuleCtx *ctx, void *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. */ - } + ValkeyModule_Assert(ValkeyModule_StopTimer(ctx, d->id, &timer_data) == VALKEYMODULE_OK); + ValkeyModule_Assert(timer_data == d); reply = ValkeyModule_Call(ctx, "INCR", "s", d->keyname); if (reply != NULL)