Merge pull request #2229 from advance512/spopWithCount

Memory leak fixes (+ code style fixes)

src/intset.c

@@ -261,22 +261,19 @@ int64_t intsetRandom(intset *is) {
     return _intsetGet(is,rand()%intrev32ifbe(is->length));
 }
 
-/* How many times bigger should the set length be compared to the requested 
+/* How many times bigger should the set length be compared to the requested
  * count of members for us to use the Floyd algorithm instead of
-   the Knuth algorithm */
+ * the Knuth algorithm */
 #define RANDOMMEMBERS_ALGORITHM_SELECTION_RATIO (2)
 
-/* Copies 'count' random members from the set into the 'values' array. 
+/* Copies 'count' random members from the set into the 'values' array.
-   'values' must be an array of int64_t values, of length 'count'.
+ * 'values' must be an array of int64_t values, of length 'count'.
-   Returns the amount of items returned. If this amount is less than 'count',
+ * Returns the amount of items returned. If this amount is less than 'count',
-   then the remaining 'values' are left uninitialized. */
+ * then the remaining 'values' are left uninitialized. */
 int intsetRandomMembers(intset *is, int64_t* values, int count) {
 
     /* We don't check that is and values are non-NULL - the caller must
-       play nice. */
+     * play nice. */
-    /* redisAssert(is != NULL);
-       redisAssert(values != NULL);
-       redisAssert(count > 0); */
 
     int length = intsetLen(is);
 
@@ -286,19 +283,17 @@ int intsetRandomMembers(intset *is, int64_t* values, int count) {
     }
 
     /* Choose between the Knuth shuffle algorithm, O(1) space, O(length) time,
-       and the Floyd algorithm, O(length) space, O(count) time. */
+     * and the Floyd algorithm, O(length) space, O(count) time. */
     if ((RANDOMMEMBERS_ALGORITHM_SELECTION_RATIO * count) > length) {
 
         /* If the count of members requested is almost the length of the set,
-           use the Knuth shuffle algorithm, O(1) space, O(length) time. */
+         * use the Knuth shuffle algorithm, O(1) space, O(length) time. */
 
         /* First, fill the values array with unique random indexes inside
-           the set. */
+         * the set. */
         int in, im, rn, rm;
         im = 0;
-        for (in = 0; 
+        for (in = 0; in < length && im < count; in++) {
-             in < length && im < count; 
-             in++) {
 
             rn = length - in;
             rm = count - im;
@@ -309,10 +304,10 @@ int intsetRandomMembers(intset *is, int64_t* values, int count) {
 
     } else {
 
-        /* If the length is considerably more than the count of members 
+        /* If the length is considerably more than the count of members
-           requested, use Robert Floyd's algorithm, O(length) space, 
+         * requested, use Robert Floyd's algorithm, O(length) space,
-           O(count) time.
+         * O(count) time.
-           Based on Jon Bentley's Programming Pearls */
+         * Based on Jon Bentley's Programming Pearls */
 
         int64_t *is_used = zcalloc(sizeof(int64_t) * length);
         int in, im, r;
@@ -320,18 +315,15 @@ int intsetRandomMembers(intset *is, int64_t* values, int count) {
         r = 0;
         im = 0;
 
-        for (in = length - count; 
+        for (in = length - count; in < length && im < count; in++) {
-             in < length && im < count; 
-             in++) {
 
             /* Generate a random number r */
-            r = rand() % (in + 1); 
+            r = rand() % (in + 1);
 
             /* Do we already have the value in r? */
             if (is_used[r]) {
-            
                 /* Use in instead of the generated number */
-                r = in; 
+                r = in;
             }
 
             values[im++] = r ;
@@ -345,10 +337,8 @@ int intsetRandomMembers(intset *is, int64_t* values, int count) {
 
     /* Replace each random index with the value stored there in the intset */
     uint8_t encoding = intrev32ifbe(is->encoding);
-    for (int currentValue = 0;
+    for (int currentValue = 0; currentValue < count; currentValue++) {
-         currentValue < count;
+        values[currentValue] =
-         currentValue++) {
-        values[currentValue] = 
             _intsetGetEncoded(is, values[currentValue], encoding);
     }
 
@@ -457,7 +447,7 @@ int main(int argc, char **argv) {
         assert(_intsetValueEncoding(+2147483647) == INTSET_ENC_INT32);
         assert(_intsetValueEncoding(-2147483649) == INTSET_ENC_INT64);
         assert(_intsetValueEncoding(+2147483648) == INTSET_ENC_INT64);
-        assert(_intsetValueEncoding(-9223372036854775808ull) == 
+        assert(_intsetValueEncoding(-9223372036854775808ull) ==
                     INTSET_ENC_INT64);
         assert(_intsetValueEncoding(+9223372036854775807ull) ==
                     INTSET_ENC_INT64);

src/rdb.c

@@ -1491,7 +1491,9 @@ int rdbSaveToSlavesSockets(void) {
             {
                 retval = REDIS_ERR;
             }
+            zfree(msg);
         }
+        zfree(clientids);
         exitFromChild((retval == REDIS_OK) ? 0 : 1);
     } else {
         /* Parent */

src/t_set.c

@@ -33,7 +33,7 @@
  * Set Commands
  *----------------------------------------------------------------------------*/
 
-void sunionDiffGenericCommand(redisClient *c, robj **setkeys, int setnum, 
+void sunionDiffGenericCommand(redisClient *c, robj **setkeys, int setnum,
                               robj *dstkey, int op);
 
 /* Factory method to return a set that *can* hold "value". When the object has
@@ -208,7 +208,7 @@ int setTypeRandomElement(robj *setobj, robj **objele, int64_t *llele) {
 }
 
 /* Return a number of random elements from a non empty set.
- * 
+ *
  * This is a version of setTypeRandomElement() that is modified in order to
  * return multiple entries, using dictGetRandomKeys() and intsetRandomMembers().
  *
@@ -223,72 +223,78 @@ int setTypeRandomElement(robj *setobj, robj **objele, int64_t *llele) {
  * of continuous elements to run some kind of algorithm or to produce
  * statistics. However the function is much faster than setTypeRandomElement()
  * at producing N elements, and the elements are guaranteed to be non
- * repeating. 
+ * repeating.
  */
-unsigned long setTypeRandomElements(robj *set, unsigned long count, robj *aux_set) {
+unsigned long setTypeRandomElements(robj *set, unsigned long count,
+                                    robj *aux_set) {
     unsigned long set_size;
     unsigned long elements_to_return = count;
     unsigned long elements_copied = 0;
     unsigned long current_element = 0;
 
-    /* Like all setType* functions, we assume good behavior on part of the caller,
+    /* Like all setType* functions, we assume good behavior on part of the
-     * so no extra parameter checks are made. */
+     * caller, so no extra parameter checks are made. */
 
-    /* If the number of elements in the the set is less than the count 
+    /* If the number of elements in the the set is less than the count
-       requested, just return all of them. */
+     * requested, just return all of them. */
     set_size = setTypeSize(set);
     if (set_size < count) {
         elements_to_return = set_size;
     }
 
-    /* TODO: It is definitely faster adding items to the set by directly handling the Dict
+    /* TODO: It is definitely faster adding items to the set by directly
-       or intset inside it, avoiding the constant encoding checks inside setTypeAdd(). 
+     * handling the Dict or intset inside it, avoiding the constant encoding
-       However, We don't want to touch the set internals in non setType* functions.
+     * checks inside setTypeAdd(). However, We don't want to touch the set
-       So, we just call setTypeAdd() multiple times, but this isn't an optimial
+     * internals in non setType* functions. So, we just call setTypeAdd()
-       solution. 
+     * multiple times, but this isn't an optimal solution.
-       Another option would be to create a bulk-add function: setTypeAddBulk(). */
+     * Another option would be to create a bulk-add function:
+     * setTypeAddBulk(). */
     if (set->encoding == REDIS_ENCODING_HT) {
         /* Allocate result array */
-        dictEntry **random_elements = 
+        dictEntry **random_elements =
             zmalloc(sizeof(dictEntry*) * elements_to_return);
 
         /* Get the random elements */
-        elements_copied = 
+        elements_copied =
             dictGetRandomKeys(set->ptr, random_elements, elements_to_return);
         redisAssert(elements_copied == elements_to_return);
 
         /* Put them into the set */
-        for (current_element = 0;
+        for (current_element = 0; current_element < elements_copied;
-             current_element < elements_copied;
              current_element++) {
 
             /* We get the key and duplicate it, as we know it is a string */
-            setTypeAdd(aux_set, 
+            setTypeAdd(aux_set,
-                dupStringObject(dictGetKey(random_elements[current_element])));
+                dictGetKey(random_elements[current_element]));
         }
 
         zfree(random_elements);
 
     } else if (set->encoding == REDIS_ENCODING_INTSET) {
         /* Allocate result array */
-        int64_t *random_elements = zmalloc(sizeof(int64_t) * elements_to_return);
+        int64_t *random_elements =
+            zmalloc(sizeof(int64_t) * elements_to_return);
+        robj* element_as_str = NULL;
 
-        elements_copied = 
+        elements_copied =
-            intsetRandomMembers((intset*) set->ptr, 
+            intsetRandomMembers((intset*) set->ptr,
-                                random_elements, 
+                                random_elements,
                                 elements_to_return);
 
         redisAssert(elements_copied == elements_to_return);
 
         /* Put them into the set */
-        for (current_element = 0;
+        for (current_element = 0; current_element < elements_copied;
-             current_element < elements_copied;
              current_element++) {
 
+            element_as_str = createStringObjectFromLongLong(
+                    random_elements[current_element]);
+
             /* Put the values in the set */
-            setTypeAdd(aux_set, 
+            setTypeAdd(aux_set,
-                       createStringObjectFromLongLong(
+                       element_as_str);
-                            random_elements[current_element]));
+
+            decrRefCount(element_as_str);
         }
 
         zfree(random_elements);
@@ -296,7 +302,7 @@ unsigned long setTypeRandomElements(robj *set, unsigned long count, robj *aux_se
         redisPanic("Unknown set encoding");
     }
 
-    /* We have a set with random elements. Return the actual elements in 
+    /* We have a set with random elements. Return the actual elements in
        the aux_set. */
     return elements_copied;
 }
@@ -493,31 +499,31 @@ void spopWithCountCommand(redisClient *c) {
         return;
     }
 
-    /* Make sure a key with the name inputted exists, and that it's type is 
+    /* Make sure a key with the name inputted exists, and that it's type is
-       indeed a set. Otherwise, return nil */
+     * indeed a set. Otherwise, return nil */
     if ((set = lookupKeyReadOrReply(c,c->argv[1],shared.emptymultibulk))
         == NULL || checkType(c,set,REDIS_SET)) return;
 
-    /* If count is zero, serve an empty multibulk ASAP to avoid special 
+    /* If count is zero, serve an empty multibulk ASAP to avoid special
-       cases later. */
+     * cases later. */
     if (count == 0) {
         addReply(c,shared.emptymultibulk);
         return;
     }
 
     /* Get the size of the set. It is always > 0, as empty sets get
-       deleted.  */
+     * deleted.  */
     size = setTypeSize(set);
 
     /* Generate an SPOP keyspace notification */
     notifyKeyspaceEvent(REDIS_NOTIFY_SET,"spop",c->argv[1],c->db->id);
 
     /* CASE 1:
-     * The number of requested elements is greater than or equal to 
+     * The number of requested elements is greater than or equal to
      * the number of elements inside the set: simply return the whole set. */
     if (count >= size) {
 
-        /* We just return the entire set */ 
+        /* We just return the entire set */
         sunionDiffGenericCommand(c,c->argv+1,1,NULL,REDIS_OP_UNION);
 
         /* Delete the set as it is now empty */
@@ -536,14 +542,14 @@ void spopWithCountCommand(redisClient *c) {
      * The number of requested elements is less than the number
      * of elements inside the set. */
 
-    /* We need an auxiliary set. Optimistically, we create a set using an 
+    /* We need an auxiliary set. Optimistically, we create a set using an
-       Intset internally. */
+     * Intset internally. */
     aux = createStringObjectFromLongLong(0);
     aux_set = setTypeCreate(aux);
     decrRefCount(aux);
 
-    /* Get the count requested of random elements from the set into our 
+    /* Get the count requested of random elements from the set into our
-       auxiliary set. */
+     * auxiliary set. */
     elements_returned = setTypeRandomElements(set, count, aux_set);
     redisAssert(elements_returned == count);
 
@@ -568,8 +574,8 @@ void spopWithCountCommand(redisClient *c) {
                 setTypeRemove(set, objele);
             }
             else if (element_encoding == REDIS_ENCODING_INTSET) {
-                /* TODO: setTypeRemove() forces us to convert all of the ints 
+                /* TODO: setTypeRemove() forces us to convert all of the ints
-                         to string... isn't there a nicer way to do this? */
+                 *       to string... isn't there a nicer way to do this? */
                 objele = createStringObjectFromLongLong(llele);
                 addReplyBulk(c, objele);
 
@@ -590,7 +596,7 @@ void spopWithCountCommand(redisClient *c) {
     }
 
     /* Free the auxiliary set - we need it no more. */
-    freeSetObject(aux_set);
+    decrRefCount(aux_set);
 }
 
 void spopCommand(redisClient *c) {
@@ -606,8 +612,8 @@ void spopCommand(redisClient *c) {
         return;
     }
 
-    /* Make sure a key with the name inputted exists, and that it's type is 
+    /* Make sure a key with the name inputted exists, and that it's type is
-       indeed a set */
+     * indeed a set */
     if ((set = lookupKeyWriteOrReply(c,c->argv[1],shared.nullbulk)) == NULL ||
         checkType(c,set,REDIS_SET)) return;
 
@@ -822,7 +828,7 @@ int qsortCompareSetsByRevCardinality(const void *s1, const void *s2) {
     return  (o2 ? setTypeSize(o2) : 0) - (o1 ? setTypeSize(o1) : 0);
 }
 
-void sinterGenericCommand(redisClient *c, robj **setkeys, 
+void sinterGenericCommand(redisClient *c, robj **setkeys,
                           unsigned long setnum, robj *dstkey) {
     robj **sets = zmalloc(sizeof(robj*)*setnum);
     setTypeIterator *si;
@@ -970,7 +976,7 @@ void sinterstoreCommand(redisClient *c) {
 #define REDIS_OP_DIFF 1
 #define REDIS_OP_INTER 2
 
-void sunionDiffGenericCommand(redisClient *c, robj **setkeys, int setnum, 
+void sunionDiffGenericCommand(redisClient *c, robj **setkeys, int setnum,
                               robj *dstkey, int op) {
     robj **sets = zmalloc(sizeof(robj*)*setnum);
     setTypeIterator *si;