add test

tests/modules/Makefile

@@ -85,6 +85,7 @@ TEST_MODULES = \
     internalsecret.so \
     configaccess.so \
     test_keymeta.so \
+    keymeta_notify.so \
     atomicslotmigration.so
 
 .PHONY: all

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 <key>  - 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 <string.h>
+#include <stdlib.h>
+
+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 <key> - 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;
+}

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}
+    }
+}