From 037755edd4a8400a6ab1baf6af5227ecdf11aaf0 Mon Sep 17 00:00:00 2001 From: Vasyl Melnychuk Date: Fri, 10 Jan 2020 23:34:15 +0200 Subject: [PATCH 1/5] Make error when submitting command in incorrect context more explicit So error message `ERR only (P)SUBSCRIBE / (P)UNSUBSCRIBE / PING / QUIT allowed in this context` will become `ERR 'get' command submitted, but only (P)SUBSCRIBE / (P)UNSUBSCRIBE / PING / QUIT allowed in this context` --- src/server.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/server.c b/src/server.c index 5845a5485..3f64aaec9 100644 --- a/src/server.c +++ b/src/server.c @@ -3498,7 +3498,10 @@ int processCommand(client *c) { c->cmd->proc != unsubscribeCommand && c->cmd->proc != psubscribeCommand && c->cmd->proc != punsubscribeCommand) { - addReplyError(c,"only (P)SUBSCRIBE / (P)UNSUBSCRIBE / PING / QUIT allowed in this context"); + addReplyErrorFormat(c, + "'%s' command submitted, but only (P)SUBSCRIBE / " + "(P)UNSUBSCRIBE / PING / QUIT allowed in this context", + c->cmd->name); return C_OK; } From 4048cf612532fdec69514c246ab55198c492ec56 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 15 Jan 2020 17:55:24 +0100 Subject: [PATCH 2/5] Change error message for #6775. --- src/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index 3f64aaec9..2b226f568 100644 --- a/src/server.c +++ b/src/server.c @@ -3499,8 +3499,8 @@ int processCommand(client *c) { c->cmd->proc != psubscribeCommand && c->cmd->proc != punsubscribeCommand) { addReplyErrorFormat(c, - "'%s' command submitted, but only (P)SUBSCRIBE / " - "(P)UNSUBSCRIBE / PING / QUIT allowed in this context", + "Can't execute '%s': only (P)SUBSCRIBE / " + "(P)UNSUBSCRIBE / PING / QUIT are allowed in this context", c->cmd->name); return C_OK; } From d9f508d52702e70d32fc5ea1b70cc86bb981974a Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Tue, 24 Dec 2019 17:14:23 +0530 Subject: [PATCH 3/5] Modules: Fix blocked-client-related memory leak If a blocked module client times-out (or disconnects, unblocked by CLIENT command, etc.) we need to call moduleUnblockClient in order to free memory allocated by the module sub-system and blocked-client private data Other changes: Made blockedonkeys.tcl tests a bit more aggressive in order to smoke-out potential memory leaks --- src/module.c | 9 +++++++ tests/modules/blockonkeys.c | 13 ++++++----- tests/unit/moduleapi/blockonkeys.tcl | 35 ++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/module.c b/src/module.c index 965bb4460..d2b267be2 100644 --- a/src/module.c +++ b/src/module.c @@ -4277,6 +4277,15 @@ void unblockClientFromModule(client *c) { moduleFreeContext(&ctx); } + /* If we made it here and client is still blocked it means that the command + * timed-out, client was killed or disconnected and disconnect_callback was + * not implemented (or it was, but RM_UnblockClient was not called from + * within it, as it should). + * We must call moduleUnblockClient in order to free privdata and + * RedisModuleBlockedClient */ + if (!bc->unblocked) + moduleUnblockClient(c); + bc->client = NULL; /* Reset the client for a new query since, for blocking commands implemented * into modules, we do not it immediately after the command returns (and diff --git a/tests/modules/blockonkeys.c b/tests/modules/blockonkeys.c index 959918b1c..10dc65b1a 100644 --- a/tests/modules/blockonkeys.c +++ b/tests/modules/blockonkeys.c @@ -172,13 +172,13 @@ int bpopgt_reply_callback(RedisModuleCtx *ctx, RedisModuleString **argv, int arg REDISMODULE_NOT_USED(argv); REDISMODULE_NOT_USED(argc); RedisModuleString *keyname = RedisModule_GetBlockedClientReadyKey(ctx); - long long gt = (long long)RedisModule_GetBlockedClientPrivateData(ctx); + long long *pgt = RedisModule_GetBlockedClientPrivateData(ctx); fsl_t *fsl; if (!get_fsl(ctx, keyname, REDISMODULE_READ, 0, &fsl, 0)) return REDISMODULE_ERR; - if (!fsl || fsl->list[fsl->length-1] <= gt) + if (!fsl || fsl->list[fsl->length-1] <= *pgt) return REDISMODULE_ERR; RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]); @@ -192,10 +192,8 @@ int bpopgt_timeout_callback(RedisModuleCtx *ctx, RedisModuleString **argv, int a } void bpopgt_free_privdata(RedisModuleCtx *ctx, void *privdata) { - /* Nothing to do because privdata is actually a 'long long', - * not a pointer to the heap */ REDISMODULE_NOT_USED(ctx); - REDISMODULE_NOT_USED(privdata); + RedisModule_Free(privdata); } /* FSL.BPOPGT - Block clients until list has an element greater than . @@ -217,9 +215,12 @@ int fsl_bpopgt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { return REDISMODULE_OK; if (!fsl || fsl->list[fsl->length-1] <= gt) { + /* We use malloc so the tests in blockedonkeys.tcl can check for memory leaks */ + long long *pgt = RedisModule_Alloc(sizeof(long long)); + *pgt = gt; /* Key is empty or has <2 elements, we must block */ RedisModule_BlockClientOnKeys(ctx, bpopgt_reply_callback, bpopgt_timeout_callback, - bpopgt_free_privdata, timeout, &argv[1], 1, (void*)gt); + bpopgt_free_privdata, timeout, &argv[1], 1, pgt); } else { RedisModule_ReplyWithLongLong(ctx, fsl->list[--fsl->length]); } diff --git a/tests/unit/moduleapi/blockonkeys.tcl b/tests/unit/moduleapi/blockonkeys.tcl index cb99ab1c9..b380227e0 100644 --- a/tests/unit/moduleapi/blockonkeys.tcl +++ b/tests/unit/moduleapi/blockonkeys.tcl @@ -45,18 +45,24 @@ start_server {tags {"modules"}} { test {Module client blocked on keys (with metadata): Timeout} { r del k set rd [redis_deferring_client] + $rd client id + set cid [$rd read] r fsl.push k 33 $rd fsl.bpopgt k 35 1 assert_equal {Request timedout} [$rd read] + r client kill id $cid ;# try to smoke-out client-related memory leak } test {Module client blocked on keys (with metadata): Blocked, case 1} { r del k set rd [redis_deferring_client] + $rd client id + set cid [$rd read] r fsl.push k 33 $rd fsl.bpopgt k 33 0 r fsl.push k 34 assert_equal {34} [$rd read] + r client kill id $cid ;# try to smoke-out client-related memory leak } test {Module client blocked on keys (with metadata): Blocked, case 2} { @@ -70,6 +76,35 @@ start_server {tags {"modules"}} { assert_equal {36} [$rd read] } + test {Module client blocked on keys (with metadata): Blocked, CLIENT KILL} { + r del k + set rd [redis_deferring_client] + $rd client id + set cid [$rd read] + $rd fsl.bpopgt k 35 0 + r client kill id $cid ;# try to smoke-out client-related memory leak + } + + test {Module client blocked on keys (with metadata): Blocked, CLIENT UNBLOCK TIMEOUT} { + r del k + set rd [redis_deferring_client] + $rd client id + set cid [$rd read] + $rd fsl.bpopgt k 35 0 + r client unblock $cid timeout ;# try to smoke-out client-related memory leak + assert_equal {Request timedout} [$rd read] + } + + test {Module client blocked on keys (with metadata): Blocked, CLIENT UNBLOCK ERROR} { + r del k + set rd [redis_deferring_client] + $rd client id + set cid [$rd read] + $rd fsl.bpopgt k 35 0 + r client unblock $cid error ;# try to smoke-out client-related memory leak + assert_error "*unblocked*" {$rd read} + } + test {Module client blocked on keys does not wake up on wrong type} { r del k set rd [redis_deferring_client] From 229229eb55123021e5fe45c431dcde9f81681c0c Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 29 Jan 2020 12:47:50 +0100 Subject: [PATCH 4/5] Add more info in the unblockClientFromModule() function. --- src/module.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/module.c b/src/module.c index d2b267be2..7fdab1b34 100644 --- a/src/module.c +++ b/src/module.c @@ -4282,7 +4282,13 @@ void unblockClientFromModule(client *c) { * not implemented (or it was, but RM_UnblockClient was not called from * within it, as it should). * We must call moduleUnblockClient in order to free privdata and - * RedisModuleBlockedClient */ + * RedisModuleBlockedClient. + * + * Note that clients implementing threads and working with private data, + * should make sure to stop the threads or protect the private data + * in some other way in the disconnection and timeout callback, because + * here we are going to free the private data associated with the + * blocked client. */ if (!bc->unblocked) moduleUnblockClient(c); From 2ad427f862084e0e18fbd6c313af35ccdb6c68f7 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Thu, 30 Jan 2020 18:14:45 +0530 Subject: [PATCH 5/5] ld2string should fail if string contains \0 in the middle This bug affected RM_StringToLongDouble and HINCRBYFLOAT. I added tests for both cases. Main changes: 1. Fixed string2ld to fail if string contains \0 in the middle 2. Use string2ld in getLongDoubleFromObject - No point of having duplicated code here The two changes above broke RM_SaveLongDouble/RM_LoadLongDouble because the long double string was saved with length+1 (An innocent mistake, but it's actually a bug - The length passed to RM_SaveLongDouble should not include the last \0). --- src/module.c | 2 +- src/object.c | 10 +--------- src/util.c | 3 ++- tests/modules/misc.c | 9 +++++++++ tests/unit/type/hash.tcl | 7 +++++++ 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/module.c b/src/module.c index 7fdab1b34..d922c5c20 100644 --- a/src/module.c +++ b/src/module.c @@ -3901,7 +3901,7 @@ void RM_SaveLongDouble(RedisModuleIO *io, long double value) { /* Long double has different number of bits in different platforms, so we * save it as a string type. */ size_t len = ld2string(buf,sizeof(buf),value,LD_STR_HEX); - RM_SaveStringBuffer(io,buf,len+1); /* len+1 for '\0' */ + RM_SaveStringBuffer(io,buf,len); } /* In the context of the rdb_save method of a module data type, loads back the diff --git a/src/object.c b/src/object.c index 2201a317a..52007b474 100644 --- a/src/object.c +++ b/src/object.c @@ -640,21 +640,13 @@ int getDoubleFromObjectOrReply(client *c, robj *o, double *target, const char *m int getLongDoubleFromObject(robj *o, long double *target) { long double value; - char *eptr; if (o == NULL) { value = 0; } else { serverAssertWithInfo(NULL,o,o->type == OBJ_STRING); if (sdsEncodedObject(o)) { - errno = 0; - value = strtold(o->ptr, &eptr); - if (sdslen(o->ptr) == 0 || - isspace(((const char*)o->ptr)[0]) || - (size_t)(eptr-(char*)o->ptr) != sdslen(o->ptr) || - (errno == ERANGE && - (value == HUGE_VAL || value == -HUGE_VAL || value == 0)) || - isnan(value)) + if (!string2ld(o->ptr, sdslen(o->ptr), &value)) return C_ERR; } else if (o->encoding == OBJ_ENCODING_INT) { value = (long)o->ptr; diff --git a/src/util.c b/src/util.c index 20471b539..2be42a0df 100644 --- a/src/util.c +++ b/src/util.c @@ -471,13 +471,14 @@ int string2ld(const char *s, size_t slen, long double *dp) { long double value; char *eptr; - if (slen >= sizeof(buf)) return 0; + if (slen == 0 || slen >= sizeof(buf)) return 0; memcpy(buf,s,slen); buf[slen] = '\0'; errno = 0; value = strtold(buf, &eptr); if (isspace(buf[0]) || eptr[0] != '\0' || + (size_t)(eptr-buf) != slen || (errno == ERANGE && (value == HUGE_VAL || value == -HUGE_VAL || value == 0)) || errno == EINVAL || diff --git a/tests/modules/misc.c b/tests/modules/misc.c index b5a032f60..41bec06ed 100644 --- a/tests/modules/misc.c +++ b/tests/modules/misc.c @@ -74,6 +74,15 @@ int test_ld_conv(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { RedisModule_ReplyWithError(ctx, err); goto final; } + /* Make sure we can't convert a string that has \0 in it */ + char buf[4] = "123"; + buf[1] = '\0'; + RedisModuleString *s3 = RedisModule_CreateString(ctx, buf, 3); + long double ld3; + if (RedisModule_StringToLongDouble(s3, &ld3) == REDISMODULE_OK) { + RedisModule_ReplyWithError(ctx, "Invalid string successfully converted to long double"); + goto final; + } RedisModule_ReplyWithLongDouble(ctx, ld2); final: RedisModule_FreeString(ctx, s1); diff --git a/tests/unit/type/hash.tcl b/tests/unit/type/hash.tcl index d2c679d32..9f8a21b1c 100644 --- a/tests/unit/type/hash.tcl +++ b/tests/unit/type/hash.tcl @@ -390,6 +390,13 @@ start_server {tags {"hash"}} { lappend rv [string match "ERR*not*float*" $bigerr] } {1 1} + test {HINCRBYFLOAT fails against hash value that contains a null-terminator in the middle} { + r hset h f "1\x002" + catch {r hincrbyfloat h f 1} err + set rv {} + lappend rv [string match "ERR*not*float*" $err] + } {1} + test {HSTRLEN against the small hash} { set err {} foreach k [array names smallhash *] {