Redis 2.2.7

This reverts commit 1b905277bb.
Moving all the new stuff in 2.4 branch.

00-RELEASENOTES

@@ -12,6 +12,27 @@ for 2.0.
 CHANGELOG
 ---------
 
+What's new in Redis 2.2.7
+=========================
+
+* Fixed bug #543-2 (the issue was reopened with a completely different report)
+  that caused Redis to randomly crash on list push performed against lists
+  with other clients blocked with BLPOP (or variants).
+
+What's new in Redis 2.2.6
+=========================
+
+* Fixed bug #543. If you saw Redis instances crashing on List operations
+  (only happening with a non-default max entry size ziplist setting in
+  redis.conf) it was almost certainly this problem.
+* Fixed a bug with replication where SLAVEOF NO ONE caused a slave to close the
+  connection with all its slaves.
+
+What's new in Redis 2.2.5
+=========================
+
+* Fixed a crash occurring when loading an AOF containing the SPOP command.
+
 What's new in Redis 2.2.4
 =========================
 

src/aof.c

@@ -285,9 +285,11 @@ int loadAppendOnlyFile(char *filename) {
         /* The fake client should not have a reply */
         redisAssert(fakeClient->bufpos == 0 && listLength(fakeClient->reply) == 0);
 
-        /* Clean up, ready for the next command */
-        for (j = 0; j < argc; j++) decrRefCount(argv[j]);
-        zfree(argv);
+        /* Clean up. Command code may have changed argv/argc so we use the
+         * argv/argc of the client instead of the local variables. */
+        for (j = 0; j < fakeClient->argc; j++)
+            decrRefCount(fakeClient->argv[j]);
+        zfree(fakeClient->argv);
 
         /* Handle swapping while loading big datasets when VM is on */
         force_swapout = 0;

src/debug.c

@@ -80,7 +80,7 @@ void computeDatasetDigest(unsigned char *final) {
         redisDb *db = server.db+j;
 
         if (dictSize(db->dict) == 0) continue;
-        di = dictGetIterator(db->dict);
+        di = dictGetSafeIterator(db->dict);
 
         /* hash the DB id, so the same dataset moved in a different
          * DB will lead to a different digest */

src/dict.c

@@ -244,9 +244,9 @@ int dictRehashMilliseconds(dict *d, int ms) {
 }
 
 /* This function performs just a step of rehashing, and only if there are
- * not iterators bound to our hash table. When we have iterators in the middle
- * of a rehashing we can't mess with the two hash tables otherwise some element
- * can be missed or duplicated.
+ * no safe iterators bound to our hash table. When we have iterators in the
+ * middle of a rehashing we can't mess with the two hash tables otherwise
+ * some element can be missed or duplicated.
  *
  * This function is called by common lookup or update operations in the
  * dictionary so that the hash table automatically migrates from H1 to H2
@@ -423,17 +423,26 @@ dictIterator *dictGetIterator(dict *d)
     iter->d = d;
     iter->table = 0;
     iter->index = -1;
+    iter->safe = 0;
     iter->entry = NULL;
     iter->nextEntry = NULL;
     return iter;
 }
 
+dictIterator *dictGetSafeIterator(dict *d) {
+    dictIterator *i = dictGetIterator(d);
+
+    i->safe = 1;
+    return i;
+}
+
 dictEntry *dictNext(dictIterator *iter)
 {
     while (1) {
         if (iter->entry == NULL) {
             dictht *ht = &iter->d->ht[iter->table];
-            if (iter->index == -1 && iter->table == 0) iter->d->iterators++;
+            if (iter->safe && iter->index == -1 && iter->table == 0)
+                iter->d->iterators++;
             iter->index++;
             if (iter->index >= (signed) ht->size) {
                 if (dictIsRehashing(iter->d) && iter->table == 0) {
@@ -460,7 +469,8 @@ dictEntry *dictNext(dictIterator *iter)
 
 void dictReleaseIterator(dictIterator *iter)
 {
-    if (!(iter->index == -1 && iter->table == 0)) iter->d->iterators--;
+    if (iter->safe && !(iter->index == -1 && iter->table == 0))
+        iter->d->iterators--;
     zfree(iter);
 }
 

src/dict.h

@@ -74,10 +74,13 @@ typedef struct dict {
     int iterators; /* number of iterators currently running */
 } dict;
 
+/* If safe is set to 1 this is a safe iteartor, that means, you can call
+ * dictAdd, dictFind, and other functions against the dictionary even while
+ * iterating. Otherwise it is a non safe iterator, and only dictNext()
+ * should be called while iterating. */
 typedef struct dictIterator {
     dict *d;
-    int table;
-    int index;
+    int table, index, safe;
     dictEntry *entry, *nextEntry;
 } dictIterator;
 
@@ -132,6 +135,7 @@ dictEntry * dictFind(dict *d, const void *key);
 void *dictFetchValue(dict *d, const void *key);
 int dictResize(dict *d);
 dictIterator *dictGetIterator(dict *d);
+dictIterator *dictGetSafeIterator(dict *d);
 dictEntry *dictNext(dictIterator *iter);
 void dictReleaseIterator(dictIterator *iter);
 dictEntry *dictGetRandomKey(dict *d);

src/networking.c

@@ -416,7 +416,7 @@ void acceptTcpHandler(aeEventLoop *el, int fd, void *privdata, int mask) {
 
     cfd = anetTcpAccept(server.neterr, fd, cip, &cport);
     if (cfd == AE_ERR) {
-        redisLog(REDIS_VERBOSE,"Accepting client connection: %s", server.neterr);
+        redisLog(REDIS_WARNING,"Accepting client connection: %s", server.neterr);
         return;
     }
     redisLog(REDIS_VERBOSE,"Accepted %s:%d", cip, cport);
@@ -431,7 +431,7 @@ void acceptUnixHandler(aeEventLoop *el, int fd, void *privdata, int mask) {
 
     cfd = anetUnixAccept(server.neterr, fd);
     if (cfd == AE_ERR) {
-        redisLog(REDIS_VERBOSE,"Accepting client connection: %s", server.neterr);
+        redisLog(REDIS_WARNING,"Accepting client connection: %s", server.neterr);
         return;
     }
     redisLog(REDIS_VERBOSE,"Accepted connection to %s", server.unixsocket);
@@ -523,10 +523,16 @@ void freeClient(redisClient *c) {
          * close the connection with all our slaves if we have any, so
          * when we'll resync with the master the other slaves will sync again
          * with us as well. Note that also when the slave is not connected
-         * to the master it will keep refusing connections by other slaves. */
-        while (listLength(server.slaves)) {
-            ln = listFirst(server.slaves);
-            freeClient((redisClient*)ln->value);
+         * to the master it will keep refusing connections by other slaves.
+         *
+         * We do this only if server.masterhost != NULL. If it is NULL this
+         * means the user called SLAVEOF NO ONE and we are freeing our
+         * link with the master, so no need to close link with slaves. */
+        if (server.masterhost != NULL) {
+            while (listLength(server.slaves)) {
+                ln = listFirst(server.slaves);
+                freeClient((redisClient*)ln->value);
+            }
         }
     }
     /* Release memory */

src/redis.c

@@ -85,8 +85,8 @@ struct redisCommand readonlyCommandTable[] = {
     {"incr",incrCommand,2,REDIS_CMD_DENYOOM,NULL,1,1,1},
     {"decr",decrCommand,2,REDIS_CMD_DENYOOM,NULL,1,1,1},
     {"mget",mgetCommand,-2,0,NULL,1,-1,1},
-    {"rpush",rpushCommand,-3,REDIS_CMD_DENYOOM,NULL,1,1,1},
-    {"lpush",lpushCommand,-3,REDIS_CMD_DENYOOM,NULL,1,1,1},
+    {"rpush",rpushCommand,3,REDIS_CMD_DENYOOM,NULL,1,1,1},
+    {"lpush",lpushCommand,3,REDIS_CMD_DENYOOM,NULL,1,1,1},
     {"rpushx",rpushxCommand,3,REDIS_CMD_DENYOOM,NULL,1,1,1},
     {"lpushx",lpushxCommand,3,REDIS_CMD_DENYOOM,NULL,1,1,1},
     {"linsert",linsertCommand,5,REDIS_CMD_DENYOOM,NULL,1,1,1},

src/t_list.c

@@ -259,35 +259,30 @@ void listTypeConvert(robj *subject, int enc) {
  *----------------------------------------------------------------------------*/
 
 void pushGenericCommand(redisClient *c, int where) {
-    int j, addlen = 0, pushed = 0;
     robj *lobj = lookupKeyWrite(c->db,c->argv[1]);
-    int may_have_waiting_clients = (lobj == NULL);
-
-    if (lobj && lobj->type != REDIS_LIST) {
-        addReply(c,shared.wrongtypeerr);
-        return;
-    }
-
-    for (j = 2; j < c->argc; j++) {
-        c->argv[j] = tryObjectEncoding(c->argv[j]);
-        if (may_have_waiting_clients) {
-            if (handleClientsWaitingListPush(c,c->argv[1],c->argv[j])) {
-                addlen++;
-                continue;
-            } else {
-                may_have_waiting_clients = 0;
-            }
+    c->argv[2] = tryObjectEncoding(c->argv[2]);
+    if (lobj == NULL) {
+        if (handleClientsWaitingListPush(c,c->argv[1],c->argv[2])) {
+            addReply(c,shared.cone);
+            return;
         }
-        if (!lobj) {
-            lobj = createZiplistObject();
-            dbAdd(c->db,c->argv[1],lobj);
+        lobj = createZiplistObject();
+        dbAdd(c->db,c->argv[1],lobj);
+    } else {
+        if (lobj->type != REDIS_LIST) {
+            addReply(c,shared.wrongtypeerr);
+            return;
+        }
+        if (handleClientsWaitingListPush(c,c->argv[1],c->argv[2])) {
+            touchWatchedKey(c->db,c->argv[1]);
+            addReply(c,shared.cone);
+            return;
         }
-        listTypePush(lobj,c->argv[j],where);
-        pushed++;
     }
-    addReplyLongLong(c,addlen + (lobj ? listTypeLength(lobj) : 0));
-    if (pushed) touchWatchedKey(c->db,c->argv[1]);
-    server.dirty += pushed;
+    listTypePush(lobj,c->argv[2],where);
+    addReplyLongLong(c,listTypeLength(lobj));
+    touchWatchedKey(c->db,c->argv[1]);
+    server.dirty++;
 }
 
 void lpushCommand(redisClient *c) {
@@ -822,7 +817,6 @@ int handleClientsWaitingListPush(redisClient *c, robj *key, robj *ele) {
 
         /* This should remove the first element of the "clients" list. */
         unblockClientWaitingData(receiver);
-        redisAssert(ln != listFirst(clients));
 
         if (dstkey == NULL) {
             /* BRPOP/BLPOP */

src/version.h

@@ -1 +1 @@
-#define REDIS_VERSION "2.2.4"
+#define REDIS_VERSION "2.2.7"

src/ziplist.c

@@ -398,12 +398,17 @@ static unsigned char *__ziplistCascadeUpdate(unsigned char *zl, unsigned char *p
             offset = p-zl;
             extra = rawlensize-next.prevrawlensize;
             zl = ziplistResize(zl,curlen+extra);
-            ZIPLIST_TAIL_OFFSET(zl) += extra;
             p = zl+offset;
 
-            /* Move the tail to the back. */
+            /* Current pointer and offset for next element. */
             np = p+rawlen;
             noffset = np-zl;
+
+            /* Update tail offset when next element is not the tail element. */
+            if ((zl+ZIPLIST_TAIL_OFFSET(zl)) != np)
+                ZIPLIST_TAIL_OFFSET(zl) += extra;
+
+            /* Move the tail to the back. */
             memmove(np+rawlensize,
                 np+next.prevrawlensize,
                 curlen-noffset-next.prevrawlensize-1);
@@ -877,7 +882,7 @@ void pop(unsigned char *zl, int where) {
     }
 }
 
-void randstring(char *target, unsigned int min, unsigned int max) {
+int randstring(char *target, unsigned int min, unsigned int max) {
     int p, len = min+rand()%(max-min+1);
     int minval, maxval;
     switch(rand() % 3) {
@@ -899,10 +904,9 @@ void randstring(char *target, unsigned int min, unsigned int max) {
 
     while(p < len)
         target[p++] = minval+rand()%(maxval-minval+1);
-    return;
+    return len;
 }
 
-
 int main(int argc, char **argv) {
     unsigned char *zl, *p;
     unsigned char *entry;
@@ -1235,6 +1239,7 @@ int main(int argc, char **argv) {
         int i,j,len,where;
         unsigned char *p;
         char buf[1024];
+        int buflen;
         list *ref;
         listNode *refnode;
 
@@ -1243,10 +1248,6 @@ int main(int argc, char **argv) {
         unsigned int slen;
         long long sval;
 
-        /* In the regression for the cascade bug, it was triggered
-         * with a random seed of 2. */
-        srand(2);
-
         for (i = 0; i < 20000; i++) {
             zl = ziplistNew();
             ref = listCreate();
@@ -1256,31 +1257,32 @@ int main(int argc, char **argv) {
             /* Create lists */
             for (j = 0; j < len; j++) {
                 where = (rand() & 1) ? ZIPLIST_HEAD : ZIPLIST_TAIL;
-                switch(rand() % 4) {
-                case 0:
-                    sprintf(buf,"%lld",(0LL + rand()) >> 20);
-                    break;
-                case 1:
-                    sprintf(buf,"%lld",(0LL + rand()));
-                    break;
-                case 2:
-                    sprintf(buf,"%lld",(0LL + rand()) << 20);
-                    break;
-                case 3:
-                    randstring(buf,0,256);
-                break;
-                default:
-                    assert(NULL);
+                if (rand() % 2) {
+                    buflen = randstring(buf,1,sizeof(buf)-1);
+                } else {
+                    switch(rand() % 3) {
+                    case 0:
+                        buflen = sprintf(buf,"%lld",(0LL + rand()) >> 20);
+                        break;
+                    case 1:
+                        buflen = sprintf(buf,"%lld",(0LL + rand()));
+                        break;
+                    case 2:
+                        buflen = sprintf(buf,"%lld",(0LL + rand()) << 20);
+                        break;
+                    default:
+                        assert(NULL);
+                    }
                 }
 
                 /* Add to ziplist */
-                zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), where);
+                zl = ziplistPush(zl, (unsigned char*)buf, buflen, where);
 
                 /* Add to reference list */
                 if (where == ZIPLIST_HEAD) {
-                    listAddNodeHead(ref,sdsnew(buf));
+                    listAddNodeHead(ref,sdsnewlen(buf, buflen));
                 } else if (where == ZIPLIST_TAIL) {
-                    listAddNodeTail(ref,sdsnew(buf));
+                    listAddNodeTail(ref,sdsnewlen(buf, buflen));
                 } else {
                     assert(NULL);
                 }
@@ -1295,12 +1297,13 @@ int main(int argc, char **argv) {
 
                 assert(ziplistGet(p,&sstr,&slen,&sval));
                 if (sstr == NULL) {
-                    sprintf(buf,"%lld",sval);
+                    buflen = sprintf(buf,"%lld",sval);
                 } else {
-                    memcpy(buf,sstr,slen);
-                    buf[slen] = '\0';
+                    buflen = slen;
+                    memcpy(buf,sstr,buflen);
+                    buf[buflen] = '\0';
                 }
-                assert(strcmp(buf,listNodeValue(refnode)) == 0);
+                assert(memcmp(buf,listNodeValue(refnode),buflen) == 0);
             }
             zfree(zl);
             listRelease(ref);

tests/integration/aof.tcl

@@ -31,13 +31,14 @@ tags {"aof"} {
     }
 
     start_server_aof [list dir $server_path] {
-        test {Unfinished MULTI: Server should not have been started} {
-            is_alive $srv
-        } {0}
+        test "Unfinished MULTI: Server should not have been started" {
+            assert_equal 0 [is_alive $srv]
+        }
 
-        test {Unfinished MULTI: Server should have logged an error} {
-            exec cat [dict get $srv stdout] | tail -n1
-        } {*Unexpected end of file reading the append only file*}
+        test "Unfinished MULTI: Server should have logged an error" {
+            set result [exec cat [dict get $srv stdout] | tail -n1]
+            assert_match "*Unexpected end of file reading the append only file*" $result
+        }
     }
 
     ## Test that the server exits when the AOF contains a short read
@@ -47,36 +48,57 @@ tags {"aof"} {
     }
 
     start_server_aof [list dir $server_path] {
-        test {Short read: Server should not have been started} {
-            is_alive $srv
-        } {0}
+        test "Short read: Server should not have been started" {
+            assert_equal 0 [is_alive $srv]
+        }
 
-        test {Short read: Server should have logged an error} {
-            exec cat [dict get $srv stdout] | tail -n1
-        } {*Bad file format reading the append only file*}
+        test "Short read: Server should have logged an error" {
+            set result [exec cat [dict get $srv stdout] | tail -n1]
+            assert_match "*Bad file format reading the append only file*" $result
+        }
     }
 
     ## Test that redis-check-aof indeed sees this AOF is not valid
-    test {Short read: Utility should confirm the AOF is not valid} {
+    test "Short read: Utility should confirm the AOF is not valid" {
         catch {
             exec src/redis-check-aof $aof_path
-        } str
-        set _ $str
-    } {*not valid*}
+        } result
+        assert_match "*not valid*" $result
+    }
 
-    test {Short read: Utility should be able to fix the AOF} {
-        exec echo y | src/redis-check-aof --fix $aof_path
-    } {*Successfully truncated AOF*}
+    test "Short read: Utility should be able to fix the AOF" {
+        set result [exec echo y | src/redis-check-aof --fix $aof_path]
+        assert_match "*Successfully truncated AOF*" $result
+    }
 
     ## Test that the server can be started using the truncated AOF
     start_server_aof [list dir $server_path] {
-        test {Fixed AOF: Server should have been started} {
-            is_alive $srv
-        } {1}
+        test "Fixed AOF: Server should have been started" {
+            assert_equal 1 [is_alive $srv]
+        }
 
-        test {Fixed AOF: Keyspace should contain values that were parsable} {
+        test "Fixed AOF: Keyspace should contain values that were parsable" {
             set client [redis [dict get $srv host] [dict get $srv port]]
-            list [$client get foo] [$client get bar]
-        } {hello {}}
+            assert_equal "hello" [$client get foo]
+            assert_equal "" [$client get bar]
+        }
+    }
+
+    ## Test that SPOP (that modifies the client its argc/argv) is correctly free'd
+    create_aof {
+        append_to_aof [formatCommand sadd set foo]
+        append_to_aof [formatCommand sadd set bar]
+        append_to_aof [formatCommand spop set]
+    }
+
+    start_server_aof [list dir $server_path] {
+        test "AOF+SPOP: Server should have been started" {
+            assert_equal 1 [is_alive $srv]
+        }
+
+        test "AOF+SPOP: Set should have 1 member" {
+            set client [redis [dict get $srv host] [dict get $srv port]]
+            assert_equal 1 [$client scard set]
+        }
     }
 }

tests/unit/type/list.tcl

@@ -55,13 +55,6 @@ start_server {
         assert_equal $largevalue(linkedlist) [r lindex mylist2 2]
     }
 
-    test {Variadic RPUSH/LPUSH} {
-        r del mylist
-        assert_equal 4 [r lpush mylist a b c d]
-        assert_equal 8 [r rpush mylist 0 1 2 3]
-        assert_equal {d c b a 0 1 2 3} [r lrange mylist 0 -1]
-    }
-
     test {DEL a list - ziplist} {
         assert_equal 1 [r del myziplist2]
         assert_equal 0 [r exists myziplist2]
@@ -149,15 +142,6 @@ start_server {
         }
     }
 
-    test "BLPOP with variadic LPUSH" {
-        set rd [redis_deferring_client]
-        r del blist target
-        $rd blpop blist 0
-        assert_equal 2 [r lpush blist foo bar]
-        assert_equal {blist foo} [$rd read]
-        assert_equal bar [lindex [r lrange blist 0 -1] 0]
-    }
-
     test "BRPOPLPUSH with zero timeout should block indefinitely" {
         set rd [redis_deferring_client]
         r del blist target