Merge commit '2ad427f862084e0e18fbd6c313af35ccdb6c68f7' into redis_6_merge

Former-commit-id: d71c4c221580221c2c31b5e3b5d23bab50a544dc

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

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;
+            if (!string2ld(szFromObj(o), sdslen(szFromObj(o)), &value))
-            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))
                 return C_ERR;
         } else if (o->encoding == OBJ_ENCODING_INT) {
             value = (long)szFromObj(o);

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

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

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 <key> <gt> <timeout> - Block clients until list has an element greater than <gt>.
@@ -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]);
     }

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);

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]

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 *] {