More variadic MIGRATE fixes.

Another leak was fixed in the case of syntax error by restructuring the
allocation strategy for the two dynamic vectors.

We also make sure to always close the cached socket on I/O errors so that
all the I/O errors are handled the same, even if we had a previously
queued error of a different kind from the destination server.

Thanks to Kevin McGehee. Related to issue #3016.

1 files changed, 9 insertions, 8 deletions

diff --git a/src/cluster.c b/src/cluster.c
index 89467757f..e0ac92c2f 100644
--- a/src/cluster.c
+++ b/src/cluster.c
@@ -4606,9 +4606,9 @@ void migrateCommand(client *c) {
     long timeout;
     long dbid;
     long long ttl, expireat;
-    robj **ov = zmalloc(sizeof(robj*)); /* Objects to migrate. */
-    robj **kv = zmalloc(sizeof(robj*)); /* Key names. */
-    robj **newargv = NULL;
+    robj **ov = NULL; /* Objects to migrate. */
+    robj **kv = NULL; /* Key names. */
+    robj **newargv = NULL; /* Used to rewrite the command as DEL ... keys ... */
     rio cmd, payload;
     int may_retry = 1;
     int write_error = 0;
@@ -4633,13 +4633,10 @@ void migrateCommand(client *c) {
                 addReplyError(c,
                     "When using MIGRATE KEYS option, the key argument"
                     " must be set to the empty string");
-                zfree(ov); zfree(kv);
                 return;
             }
             first_key = j+1;
             num_keys = c->argc - j - 1;
-            ov = zrealloc(ov,sizeof(robj*)*num_keys);
-            kv = zrealloc(kv,sizeof(robj*)*num_keys);
             break; /* All the remaining args are keys. */
         } else {
             addReply(c,shared.syntaxerr);
@@ -4651,7 +4648,6 @@ void migrateCommand(client *c) {
     if (getLongFromObjectOrReply(c,c->argv[5],&timeout,NULL) != C_OK ||
         getLongFromObjectOrReply(c,c->argv[4],&dbid,NULL) != C_OK)
     {
-        zfree(ov); zfree(kv);
         return;
     }
     if (timeout <= 0) timeout = 1000;
@@ -4661,7 +4657,10 @@ void migrateCommand(client *c) {
      * the caller there was nothing to migrate. We don't return an error in
      * this case, since often this is due to a normal condition like the key
      * expiring in the meantime. */
+    ov = zrealloc(ov,sizeof(robj*)*num_keys);
+    kv = zrealloc(kv,sizeof(robj*)*num_keys);
     int oi = 0;
+
     for (j = 0; j < num_keys; j++) {
         if ((ov[oi] = lookupKeyRead(c->db,c->argv[first_key+j])) != NULL) {
             kv[oi] = c->argv[first_key+j];
@@ -4811,7 +4810,8 @@ try_again:
     }
 
     /* If we are here and a socket error happened, we don't want to retry.
-     * Just signal the problem to the client. */
+     * Just signal the problem to the client, but only do it if we don't
+     * already queued a different error reported by the destination server. */
     if (!error_from_target && socket_error) {
         may_retry = 0;
         goto socket_err;
@@ -4828,6 +4828,7 @@ try_again:
 
     sdsfree(cmd.io.buffer.ptr);
     zfree(ov); zfree(kv); zfree(newargv);
+    if (socket_error) migrateCloseSocket(c->argv[1],c->argv[2]);
     return;
 
 /* On socket errors we try to close the cached socket and try again.