redis/hiredis
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix potential race in 'invalid timeout' tests

Browse Source 
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.
This commit is contained in:
Tom Lee 2015-11-18 00:36:29 -08:00
parent db1c46dac7
commit d4b715f0aa
2 changed files with 30 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
net.c
View File

@ -178,19 +178,15 @@ static int redisSetTcpNoDelay(redisContext *c) {
#define __MAX_MSEC (((LONG_MAX) - 999) / 1000) #define __MAX_MSEC (((LONG_MAX) - 999) / 1000)
static int redisContextWaitReady(redisContext *c, const struct timeval *timeout) { static int redisContextTimeoutMsec(redisContext *c, long *result)
struct pollfd wfd[1]; {
long msec; const struct timeval *timeout = c->timeout;
long msec = -1;
msec = -1;
wfd[0].fd = c->fd;
wfd[0].events = POLLOUT;
/* Only use timeout when not NULL. */ /* Only use timeout when not NULL. */
if (timeout != NULL) { if (timeout != NULL) {
if (timeout->tv_usec > 1000000 || timeout->tv_sec > __MAX_MSEC) { if (timeout->tv_usec > 1000000 || timeout->tv_sec > __MAX_MSEC) {
__redisSetErrorFromErrno(c, REDIS_ERR_IO, NULL); *result = msec;
redisContextCloseFd(c);
return REDIS_ERR; 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) { if (errno == EINPROGRESS) {
int res; int res;
@ -265,7 +271,9 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
int blocking = (c->flags & REDIS_BLOCK); int blocking = (c->flags & REDIS_BLOCK);
int reuseaddr = (c->flags & REDIS_REUSEADDR); int reuseaddr = (c->flags & REDIS_REUSEADDR);
int reuses = 0; int reuses = 0;
long timeout_msec = -1;
servinfo = NULL;
c->connection_type = REDIS_CONN_TCP; c->connection_type = REDIS_CONN_TCP;
c->tcp.port = port; c->tcp.port = port;
@ -296,6 +304,11 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
c->timeout = NULL; c->timeout = NULL;
} }
if (redisContextTimeoutMsec(c, &timeout_msec) != REDIS_OK) {
__redisSetError(c, REDIS_ERR_IO, "Invalid timeout specified");
goto error;
}
if (source_addr == NULL) { if (source_addr == NULL) {
free(c->tcp.source_addr); free(c->tcp.source_addr);
c->tcp.source_addr = NULL; c->tcp.source_addr = NULL;
@ -374,7 +387,7 @@ addrretry:
goto addrretry; goto addrretry;
} }
} else { } else {
if (redisContextWaitReady(c,c->timeout) != REDIS_OK) if (redisContextWaitReady(c,timeout_msec) != REDIS_OK)
goto error; 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 redisContextConnectUnix(redisContext *c, const char *path, const struct timeval *timeout) {
int blocking = (c->flags & REDIS_BLOCK); int blocking = (c->flags & REDIS_BLOCK);
struct sockaddr_un sa; struct sockaddr_un sa;
long timeout_msec = -1;
if (redisCreateSocket(c,AF_LOCAL) < 0) if (redisCreateSocket(c,AF_LOCAL) < 0)
return REDIS_ERR; return REDIS_ERR;
@ -438,13 +452,16 @@ int redisContextConnectUnix(redisContext *c, const char *path, const struct time
c->timeout = NULL; c->timeout = NULL;
} }
if (redisContextTimeoutMsec(c,&timeout_msec) != REDIS_OK)
return REDIS_ERR;
sa.sun_family = AF_LOCAL; sa.sun_family = AF_LOCAL;
strncpy(sa.sun_path,path,sizeof(sa.sun_path)-1); strncpy(sa.sun_path,path,sizeof(sa.sun_path)-1);
if (connect(c->fd, (struct sockaddr*)&sa, sizeof(sa)) == -1) { if (connect(c->fd, (struct sockaddr*)&sa, sizeof(sa)) == -1) {
if (errno == EINPROGRESS && !blocking) { if (errno == EINPROGRESS && !blocking) {
/* This is ok. */ /* This is ok. */
} else { } else {
if (redisContextWaitReady(c,c->timeout) != REDIS_OK) if (redisContextWaitReady(c,timeout_msec) != REDIS_OK)
return REDIS_ERR; return REDIS_ERR;
} }
} }

4
test.c
View File

@ -552,7 +552,7 @@ static void test_invalid_timeout_errors(struct config config) {
c = redisConnectWithTimeout(config.tcp.host, config.tcp.port, config.tcp.timeout); 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); redisFree(c);
test("Set error when an invalid timeout sec value is given to redisConnectWithTimeout: "); 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); 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); redisFree(c);
} }