Auto merge of #379 - thomaslee:tom_test_race, r=badboy

Fix potential race in 'invalid timeout' tests

It's possible for the call to connect() to succeed on the very first try, in which case the logic for checking for invalid timeout fields is never executed. When this happens, the tests fail because they expect a REDIS_ERR_IO but no such failure has occurred.

Tests aside, this is a potential source of irritating and hard-to-find intermittent bugs.

This patch forces the validation to occur early so that we get predictable behavior whenever an invalid timeout is specified.

2 files changed, 30 insertions, 13 deletions

diff --git a/net.c b/net.c
index 60a2dc7..b658e9c 100644
--- a/net.c
+++ b/net.c
@@ -178,19 +178,15 @@ static int redisSetTcpNoDelay(redisContext *c) {
 
 #define __MAX_MSEC (((LONG_MAX) - 999) / 1000)
 
-static int redisContextWaitReady(redisContext *c, const struct timeval *timeout) {
-    struct pollfd   wfd[1];
-    long msec;
-
-    msec          = -1;
-    wfd[0].fd     = c->fd;
-    wfd[0].events = POLLOUT;
+static int redisContextTimeoutMsec(redisContext *c, long *result)
+{
+    const struct timeval *timeout = c->timeout;
+    long msec = -1;
 
     /* Only use timeout when not NULL. */
     if (timeout != NULL) {
         if (timeout->tv_usec > 1000000 || timeout->tv_sec > __MAX_MSEC) {
-            __redisSetErrorFromErrno(c, REDIS_ERR_IO, NULL);
-            redisContextCloseFd(c);
+            *result = msec;
             return REDIS_ERR;
         }
 
@@ -201,6 +197,16 @@ static int redisContextWaitReady(redisContext *c, const struct timeval *timeout)
         }
     }
 
+    *result = msec;
+    return REDIS_OK;
+}
+
+static int redisContextWaitReady(redisContext *c, long msec) {
+    struct pollfd   wfd[1];
+
+    wfd[0].fd     = c->fd;
+    wfd[0].events = POLLOUT;
+
     if (errno == EINPROGRESS) {
         int res;
 
@@ -265,7 +271,9 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
     int blocking = (c->flags & REDIS_BLOCK);
     int reuseaddr = (c->flags & REDIS_REUSEADDR);
     int reuses = 0;
+    long timeout_msec = -1;
 
+    servinfo = NULL;
     c->connection_type = REDIS_CONN_TCP;
     c->tcp.port = port;
 
@@ -296,6 +304,11 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
         c->timeout = NULL;
     }
 
+    if (redisContextTimeoutMsec(c, &timeout_msec) != REDIS_OK) {
+        __redisSetError(c, REDIS_ERR_IO, "Invalid timeout specified");
+        goto error;
+    }
+
     if (source_addr == NULL) {
         free(c->tcp.source_addr);
         c->tcp.source_addr = NULL;
@@ -374,7 +387,7 @@ addrretry:
                     goto addrretry;
                 }
             } else {
-                if (redisContextWaitReady(c,c->timeout) != REDIS_OK)
+                if (redisContextWaitReady(c,timeout_msec) != REDIS_OK)
                     goto error;
             }
         }
@@ -415,6 +428,7 @@ int redisContextConnectBindTcp(redisContext *c, const char *addr, int port,
 int redisContextConnectUnix(redisContext *c, const char *path, const struct timeval *timeout) {
     int blocking = (c->flags & REDIS_BLOCK);
     struct sockaddr_un sa;
+    long timeout_msec = -1;
 
     if (redisCreateSocket(c,AF_LOCAL) < 0)
         return REDIS_ERR;
@@ -438,13 +452,16 @@ int redisContextConnectUnix(redisContext *c, const char *path, const struct time
         c->timeout = NULL;
     }
 
+    if (redisContextTimeoutMsec(c,&timeout_msec) != REDIS_OK)
+        return REDIS_ERR;
+
     sa.sun_family = AF_LOCAL;
     strncpy(sa.sun_path,path,sizeof(sa.sun_path)-1);
     if (connect(c->fd, (struct sockaddr*)&sa, sizeof(sa)) == -1) {
         if (errno == EINPROGRESS && !blocking) {
             /* This is ok. */
         } else {
-            if (redisContextWaitReady(c,c->timeout) != REDIS_OK)
+            if (redisContextWaitReady(c,timeout_msec) != REDIS_OK)
                 return REDIS_ERR;
         }
     }
diff --git a/test.c b/test.c
index c309df8..538d376 100644
--- a/test.c
+++ b/test.c
@@ -552,7 +552,7 @@ static void test_invalid_timeout_errors(struct config config) {
 
     c = redisConnectWithTimeout(config.tcp.host, config.tcp.port, config.tcp.timeout);
 
-    test_cond(c->err == REDIS_ERR_IO);
+    test_cond(c->err == REDIS_ERR_IO && strcmp(c->errstr, "Invalid timeout specified") == 0);
     redisFree(c);
 
     test("Set error when an invalid timeout sec value is given to redisConnectWithTimeout: ");
@@ -562,7 +562,7 @@ static void test_invalid_timeout_errors(struct config config) {
 
     c = redisConnectWithTimeout(config.tcp.host, config.tcp.port, config.tcp.timeout);
 
-    test_cond(c->err == REDIS_ERR_IO);
+    test_cond(c->err == REDIS_ERR_IO && strcmp(c->errstr, "Invalid timeout specified") == 0);
     redisFree(c);
 }