diff --git a/src/module.cpp b/src/module.cpp index b93ad36ad..57d0b9da7 100644 --- a/src/module.cpp +++ b/src/module.cpp @@ -3981,7 +3981,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 @@ -4357,6 +4357,21 @@ 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. + * + * 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); + 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/src/object.cpp b/src/object.cpp index d6c352873..cd35a50e2 100644 --- a/src/object.cpp +++ b/src/object.cpp @@ -660,21 +660,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(szFromObj(o), &eptr); - if (sdslen(szFromObj(o)) == 0 || - isspace(((const char*)szFromObj(o))[0]) || - (size_t)(eptr-(char*)szFromObj(o)) != sdslen(szFromObj(o)) || - (errno == ERANGE && - (value == HUGE_VAL || value == -HUGE_VAL || value == 0)) || - std::isnan(value)) + if (!string2ld(szFromObj(o), sdslen(szFromObj(o)), &value)) return C_ERR; } else if (o->encoding == OBJ_ENCODING_INT) { value = (long)szFromObj(o); diff --git a/src/server.cpp b/src/server.cpp index 000bdd76f..e64950735 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -3729,7 +3729,10 @@ int processCommand(client *c, int callFlags) { 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, + "Can't execute '%s': only (P)SUBSCRIBE / " + "(P)UNSUBSCRIBE / PING / QUIT are allowed in this context", + c->cmd->name); return C_OK; } 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/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/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/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] 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 *] {