diff --git a/tests/modules/Makefile b/tests/modules/Makefile index f14123478..0659497cf 100644 --- a/tests/modules/Makefile +++ b/tests/modules/Makefile @@ -85,6 +85,7 @@ TEST_MODULES = \ internalsecret.so \ configaccess.so \ test_keymeta.so \ + keymeta_notify.so \ atomicslotmigration.so .PHONY: all diff --git a/tests/modules/keymeta_notify.c b/tests/modules/keymeta_notify.c new file mode 100644 index 000000000..c2d051baa --- /dev/null +++ b/tests/modules/keymeta_notify.c @@ -0,0 +1,147 @@ +/* Test module: SetKeyMeta during keyspace notification callback. + * + * This module reproduces a bug where hsetnxCommand (and potentially other + * hash commands) would access a stale kvobj pointer after firing a keyspace + * notification, if a module's notification callback called SetKeyMeta which + * internally reallocates the kvobj via keyMetaSetMetadata. + * + * The fix ensures notifyKeyspaceEvent is called AFTER all kvobj accesses. + * + * Commands: + * KEYMETANOTIFY.GET - Get the metadata value attached to a key + * KEYMETANOTIFY.CHECK - Returns "OK" (health check) + * + * Copyright (c) 2006-Present, Redis Ltd. + * All rights reserved. + * + * Licensed under your choice of (a) the Redis Source Available License 2.0 + * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the + * GNU Affero General Public License v3 (AGPLv3). + */ + +#include "redismodule.h" +#include +#include + +static RedisModuleKeyMetaClassId meta_class_id = -1; + +/* Counter incremented each time we successfully set metadata in a notification */ +static long long meta_set_count = 0; + +/* Notification callback: sets metadata on the key during hash notifications. + * This triggers the bug if the notification fires before all kvobj accesses + * in the command implementation (e.g., hsetnxCommand). */ +static int HashNotifyCallback(RedisModuleCtx *ctx, int type, const char *event, + RedisModuleString *key) { + REDISMODULE_NOT_USED(type); + REDISMODULE_NOT_USED(event); + + if (meta_class_id < 0) return REDISMODULE_OK; + + RedisModuleKey *k = RedisModule_OpenKey(ctx, key, REDISMODULE_WRITE); + if (!k) return REDISMODULE_OK; + + if (RedisModule_KeyType(k) == REDISMODULE_KEYTYPE_EMPTY) { + RedisModule_CloseKey(k); + return REDISMODULE_OK; + } + + /* Free existing metadata if any */ + uint64_t existing = 0; + if (RedisModule_GetKeyMeta(meta_class_id, k, &existing) == REDISMODULE_OK) { + if (existing != 0) { + free((char *)existing); + } + } + + /* Set new metadata - a simple string "notified" */ + char *new_str = strdup("notified"); + if (RedisModule_SetKeyMeta(meta_class_id, k, (uint64_t)new_str) == REDISMODULE_OK) { + meta_set_count++; + } else { + free(new_str); + } + + RedisModule_CloseKey(k); + return REDISMODULE_OK; +} + +/* Free callback for metadata */ +static void MetaFreeCallback(const char *keyname, uint64_t meta) { + REDISMODULE_NOT_USED(keyname); + if (meta != 0) { + free((char *)meta); + } +} + +/* KEYMETANOTIFY.GET - Get the metadata string attached to a key */ +static int GetMetaCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + if (argc != 2) return RedisModule_WrongArity(ctx); + + RedisModuleKey *k = RedisModule_OpenKey(ctx, argv[1], REDISMODULE_READ); + if (!k || RedisModule_KeyType(k) == REDISMODULE_KEYTYPE_EMPTY) { + if (k) RedisModule_CloseKey(k); + RedisModule_ReplyWithNull(ctx); + return REDISMODULE_OK; + } + + uint64_t meta = 0; + if (RedisModule_GetKeyMeta(meta_class_id, k, &meta) == REDISMODULE_OK && meta != 0) { + RedisModule_ReplyWithCString(ctx, (const char *)meta); + } else { + RedisModule_ReplyWithNull(ctx); + } + RedisModule_CloseKey(k); + return REDISMODULE_OK; +} + +/* KEYMETANOTIFY.SETCOUNT - Get how many times we successfully set metadata in notifications */ +static int SetCountCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + REDISMODULE_NOT_USED(argv); + REDISMODULE_NOT_USED(argc); + RedisModule_ReplyWithLongLong(ctx, meta_set_count); + return REDISMODULE_OK; +} + +int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + REDISMODULE_NOT_USED(argv); + REDISMODULE_NOT_USED(argc); + + if (RedisModule_Init(ctx, "keymetanotify", 1, REDISMODULE_APIVER_1) == REDISMODULE_ERR) + return REDISMODULE_ERR; + + /* Register a metadata class */ + RedisModuleKeyMetaClassConfig config = {0}; + config.version = REDISMODULE_KEY_META_VERSION; + config.flags = (1 << REDISMODULE_META_ALLOW_IGNORE); + config.reset_value = (uint64_t)NULL; + config.free = MetaFreeCallback; + config.rdb_load = NULL; + config.rdb_save = NULL; + config.aof_rewrite = NULL; + config.copy = NULL; + config.rename = NULL; + config.move = NULL; + config.defrag = NULL; + config.unlink = NULL; + config.mem_usage = NULL; + config.free_effort = NULL; + + meta_class_id = RedisModule_CreateKeyMetaClass(ctx, "kmno", 1, &config); + if (meta_class_id < 0) return REDISMODULE_ERR; + + /* Subscribe to hash keyspace events */ + if (RedisModule_SubscribeToKeyspaceEvents(ctx, REDISMODULE_NOTIFY_HASH, + HashNotifyCallback) != REDISMODULE_OK) + return REDISMODULE_ERR; + + if (RedisModule_CreateCommand(ctx, "keymetanotify.get", GetMetaCommand, + "readonly", 1, 1, 1) == REDISMODULE_ERR) + return REDISMODULE_ERR; + + if (RedisModule_CreateCommand(ctx, "keymetanotify.setcount", SetCountCommand, + "readonly", 0, 0, 0) == REDISMODULE_ERR) + return REDISMODULE_ERR; + + return REDISMODULE_OK; +} diff --git a/tests/unit/moduleapi/keymeta-notify.tcl b/tests/unit/moduleapi/keymeta-notify.tcl new file mode 100644 index 000000000..c284a7c16 --- /dev/null +++ b/tests/unit/moduleapi/keymeta-notify.tcl @@ -0,0 +1,97 @@ +# Regression test for SetKeyMeta during keyspace notification. +# +# Bug: hsetnxCommand fired notifyKeyspaceEvent BEFORE accessing the kvobj +# pointer for hashTypeLength/updateKeysizesHist/updateSlotAllocSize. +# If a module's notification callback called SetKeyMeta (which requires +# REDISMODULE_WRITE and triggers keyMetaSetMetadata), the kvobj could be +# reallocated, leaving hsetnxCommand with a stale pointer. This caused +# "Guru Meditation: Unknown hash encoding" crash. +# +# Fix: Move notifyKeyspaceEvent to AFTER all kvobj accesses in hsetnxCommand, +# matching the safe pattern already used by hsetCommand. + +set testmodule [file normalize tests/modules/keymeta_notify.so] + +start_server {tags {"modules" "external:skip"}} { + r module load $testmodule + + test {HSETNX with SetKeyMeta in notification does not crash} { + # This is the primary regression test. + # Before the fix, this would crash with: + # "Guru Meditation: Unknown hash encoding #t_hash.c:1335" + r HSETNX mykey field1 value1 + + # Verify the hash is valid and accessible + assert_equal [r HGET mykey field1] "value1" + + # Verify metadata was set by the notification callback + assert_equal [r keymetanotify.get mykey] "notified" + + # Second HSETNX on same field (no-op, field exists) + r HSETNX mykey field1 value2 + assert_equal [r HGET mykey field1] "value1" + + # HSETNX on a new field in the same hash + r HSETNX mykey field2 value2 + assert_equal [r HGET mykey field2] "value2" + assert_equal [r HLEN mykey] 2 + + # Verify the hash is still fully functional + assert_equal [r keymetanotify.get mykey] "notified" + } + + test {HSET with SetKeyMeta in notification works correctly} { + r DEL mykey2 + r HSET mykey2 f1 v1 + assert_equal [r HGET mykey2 f1] "v1" + assert_equal [r keymetanotify.get mykey2] "notified" + + # Multiple fields + r HSET mykey2 f2 v2 f3 v3 + assert_equal [r HLEN mykey2] 3 + assert_equal [r keymetanotify.get mykey2] "notified" + } + + test {HMSET with SetKeyMeta in notification works correctly} { + r DEL mykey3 + r HMSET mykey3 f1 v1 f2 v2 + assert_equal [r HGET mykey3 f1] "v1" + assert_equal [r HGET mykey3 f2] "v2" + assert_equal [r keymetanotify.get mykey3] "notified" + } + + test {HINCRBY with SetKeyMeta in notification works correctly} { + r DEL mykey4 + r HSET mykey4 counter 10 + r HINCRBY mykey4 counter 5 + assert_equal [r HGET mykey4 counter] "15" + assert_equal [r keymetanotify.get mykey4] "notified" + } + + test {HINCRBYFLOAT with SetKeyMeta in notification works correctly} { + r DEL mykey5 + r HSET mykey5 value 10.5 + r HINCRBYFLOAT mykey5 value 1.5 + assert_equal [r HGET mykey5 value] "12" + assert_equal [r keymetanotify.get mykey5] "notified" + } + + test {Multiple HSETNX on new keys with SetKeyMeta does not crash} { + # Stress test: create many keys via HSETNX + for {set i 0} {$i < 100} {incr i} { + r HSETNX "stresskey:$i" field "value$i" + } + + # Verify all keys are valid + for {set i 0} {$i < 100} {incr i} { + assert_equal [r HGET "stresskey:$i" field] "value$i" + assert_equal [r keymetanotify.get "stresskey:$i"] "notified" + } + } + + test {SetKeyMeta notification count is tracked} { + # The setcount should be > 0 since we've been setting metadata + set count [r keymetanotify.setcount] + assert {$count > 0} + } +}