From a89bf734a933e45b9dd3ae85ef4c3b62bd6891d8 Mon Sep 17 00:00:00 2001 From: "meir@redislabs.com" Date: Sun, 14 Jun 2020 10:06:00 +0300 Subject: [PATCH] Fix RM_ScanKey module api not to return int encoded strings The scan key module API provides the scan callback with the current field name and value (if it exists). Those arguments are RedisModuleString* which means it supposes to point to robj which is encoded as a string. Using createStringObjectFromLongLong function might return robj that points to an integer and so break a module that tries for example to use RedisModule_StringPtrLen on the given field/value. The PR introduces a fix that uses the createObject function and sdsfromlonglong function. Using those function promise that the field and value pass to the to the scan callback will be Strings. The PR also changes the Scan test module to use RedisModule_StringPtrLen to catch the issue. without this, the issue is hidden because RedisModule_ReplyWithString knows to handle integer encoding of the given robj (RedisModuleString). The PR also introduces a new test to verify the issue is solved. --- src/module.c | 6 +++--- tests/modules/scan.c | 20 ++++++++++++++++---- tests/unit/moduleapi/scan.tcl | 5 +++++ 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/module.c b/src/module.c index e3a338dad..226c60fd0 100644 --- a/src/module.c +++ b/src/module.c @@ -6708,7 +6708,7 @@ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleSc int pos = 0; int64_t ll; while(intsetGet(o->ptr,pos++,&ll)) { - robj *field = createStringObjectFromLongLong(ll); + robj *field = createObject(OBJ_STRING,sdsfromlonglong(ll)); fn(key, field, NULL, privdata); decrRefCount(field); } @@ -6724,12 +6724,12 @@ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleSc ziplistGet(p,&vstr,&vlen,&vll); robj *field = (vstr != NULL) ? createStringObject((char*)vstr,vlen) : - createStringObjectFromLongLong(vll); + createObject(OBJ_STRING,sdsfromlonglong(vll)); p = ziplistNext(o->ptr,p); ziplistGet(p,&vstr,&vlen,&vll); robj *value = (vstr != NULL) ? createStringObject((char*)vstr,vlen) : - createStringObjectFromLongLong(vll); + createObject(OBJ_STRING,sdsfromlonglong(vll)); fn(key, field, value, privdata); p = ziplistNext(o->ptr,p); decrRefCount(field); diff --git a/tests/modules/scan.c b/tests/modules/scan.c index afede244b..1576bae9e 100644 --- a/tests/modules/scan.c +++ b/tests/modules/scan.c @@ -55,11 +55,23 @@ void scan_key_callback(RedisModuleKey *key, RedisModuleString* field, RedisModul REDISMODULE_NOT_USED(key); scan_key_pd* pd = privdata; RedisModule_ReplyWithArray(pd->ctx, 2); - RedisModule_ReplyWithString(pd->ctx, field); - if (value) - RedisModule_ReplyWithString(pd->ctx, value); - else + size_t fieldCStrLen; + + // The implementation of RedisModuleString is robj with lots of encodings. + // We want to make sure the robj that passes to this callback in + // String encoded, this is why we use RedisModule_StringPtrLen and + // RedisModule_ReplyWithStringBuffer instead of directly use + // RedisModule_ReplyWithString. + const char* fieldCStr = RedisModule_StringPtrLen(field, &fieldCStrLen); + RedisModule_ReplyWithStringBuffer(pd->ctx, fieldCStr, fieldCStrLen); + if(value){ + size_t valueCStrLen; + const char* valueCStr = RedisModule_StringPtrLen(value, &valueCStrLen); + RedisModule_ReplyWithStringBuffer(pd->ctx, valueCStr, valueCStrLen); + } else { RedisModule_ReplyWithNull(pd->ctx); + } + pd->nreplies++; } diff --git a/tests/unit/moduleapi/scan.tcl b/tests/unit/moduleapi/scan.tcl index de1672e0a..43a0c4d8a 100644 --- a/tests/unit/moduleapi/scan.tcl +++ b/tests/unit/moduleapi/scan.tcl @@ -16,6 +16,11 @@ start_server {tags {"modules"}} { r hmset hh f1 v1 f2 v2 lsort [r scan.scan_key hh] } {{f1 v1} {f2 v2}} + + test {Module scan hash dict with int value} { + r hmset hh1 f1 1 + lsort [r scan.scan_key hh1] + } {{f1 1}} test {Module scan hash dict} { r config set hash-max-ziplist-entries 2