From 49974c9359ad6b58cea15106cf6511bdb31d31a9 Mon Sep 17 00:00:00 2001 From: Mark Nunberg Date: Sun, 4 Mar 2018 18:17:16 +0200 Subject: Call connect(2) again for non-blocking connect This retrieves the actual error which occurred, as getsockopt is not always reliable in this regard. --- async.c | 24 ++++++++++++------------ async.h | 4 ++++ hiredis.c | 2 ++ hiredis.h | 3 +++ net.c | 35 ++++++++++++++++++++++++++++++++++- net.h | 1 + 6 files changed, 56 insertions(+), 13 deletions(-) diff --git a/async.c b/async.c index cb5b841..7309344 100644 --- a/async.c +++ b/async.c @@ -506,22 +506,22 @@ void redisProcessCallbacks(redisAsyncContext *ac) { * write event fires. When connecting was not successful, the connect callback * is called with a REDIS_ERR status and the context is free'd. */ static int __redisAsyncHandleConnect(redisAsyncContext *ac) { + int completed = 0; redisContext *c = &(ac->c); - - if (redisCheckSocketError(c) == REDIS_ERR) { - /* Try again later when connect(2) is still in progress. */ - if (errno == EINPROGRESS) - return REDIS_OK; - - if (ac->onConnect) ac->onConnect(ac,REDIS_ERR); + if (redisFinishAsyncConnect(c, &completed) == REDIS_ERR) { + /* Error! */ + redisCheckSocketError(c); + if (ac->onConnect) ac->onConnect(ac, REDIS_ERR); __redisAsyncDisconnect(ac); return REDIS_ERR; + } else if (completed == 1) { + /* connected! */ + if (ac->onConnect) ac->onConnect(ac, REDIS_OK); + c->flags |= REDIS_CONNECTED; + return REDIS_OK; + } else { + return REDIS_OK; } - - /* Mark context as connected. */ - c->flags |= REDIS_CONNECTED; - if (ac->onConnect) ac->onConnect(ac,REDIS_OK); - return REDIS_OK; } /* This function should be called when the socket is readable. diff --git a/async.h b/async.h index e69d840..740555c 100644 --- a/async.h +++ b/async.h @@ -93,6 +93,10 @@ typedef struct redisAsyncContext { /* Regular command callbacks */ redisCallbackList replies; + /* Address used for connect() */ + struct sockaddr *saddr; + size_t addrlen; + /* Subscription callbacks */ struct { redisCallbackList invalid; diff --git a/hiredis.c b/hiredis.c index 98f43c9..351dfbc 100644 --- a/hiredis.c +++ b/hiredis.c @@ -606,12 +606,14 @@ void redisFree(redisContext *c) { return; if (c->fd > 0) close(c->fd); + sdsfree(c->obuf); redisReaderFree(c->reader); free(c->tcp.host); free(c->tcp.source_addr); free(c->unix_sock.path); free(c->timeout); + free(c->saddr); free(c); } diff --git a/hiredis.h b/hiredis.h index bd53e7e..1b0d5e6 100644 --- a/hiredis.h +++ b/hiredis.h @@ -134,6 +134,9 @@ typedef struct redisContext { char *path; } unix_sock; + /* For non-blocking connect */ + struct sockadr *saddr; + size_t addrlen; } redisContext; redisContext *redisConnect(const char *ip, int port); diff --git a/net.c b/net.c index 5d50f7c..62c6ca0 100644 --- a/net.c +++ b/net.c @@ -232,8 +232,28 @@ static int redisContextWaitReady(redisContext *c, long msec) { return REDIS_ERR; } +int redisFinishAsyncConnect(redisContext *c, int *completed) { + int rc = connect(c->fd, (const struct sockaddr *)c->saddr, c->addrlen); + if (rc == 0) { + *completed = 1; + return REDIS_OK; + } + switch (errno) { + case EISCONN: + *completed = 1; + return REDIS_OK; + case EALREADY: + case EINPROGRESS: + case EWOULDBLOCK: + *completed = 0; + return REDIS_OK; + default: + return REDIS_ERR; + } +} + int redisCheckSocketError(redisContext *c) { - int err = 0; + int err = 0, errno_saved = errno; socklen_t errlen = sizeof(err); if (getsockopt(c->fd, SOL_SOCKET, SO_ERROR, &err, &errlen) == -1) { @@ -241,6 +261,10 @@ int redisCheckSocketError(redisContext *c) { return REDIS_ERR; } + if (err == 0) { + err = errno_saved; + } + if (err) { errno = err; __redisSetErrorFromErrno(c,REDIS_ERR_IO,NULL); @@ -373,6 +397,15 @@ addrretry: goto error; } } + + /* For repeat connection */ + if (c->saddr) { + free(c->saddr); + } + c->saddr = malloc(sizeof(*p->ai_addr)); + memcpy(c->saddr, p->ai_addr, p->ai_addrlen); + c->addrlen = p->ai_addrlen; + if (connect(s,p->ai_addr,p->ai_addrlen) == -1) { if (errno == EHOSTUNREACH) { redisContextCloseFd(c); diff --git a/net.h b/net.h index d9dc362..c35facc 100644 --- a/net.h +++ b/net.h @@ -45,5 +45,6 @@ int redisContextConnectBindTcp(redisContext *c, const char *addr, int port, const char *source_addr); int redisContextConnectUnix(redisContext *c, const char *path, const struct timeval *timeout); int redisKeepAlive(redisContext *c, int interval); +int redisFinishAsyncConnect(redisContext *c, int *completed); #endif -- cgit v1.2.3 From 5e6bbf8c6075ce6406d424e7ed031c7a6c676fb2 Mon Sep 17 00:00:00 2001 From: Mark Nunberg Date: Mon, 5 Mar 2018 11:57:22 +0200 Subject: saddr should be addrlen bytes Not sizeof saddr. --- net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net.c b/net.c index 62c6ca0..7d5588e 100644 --- a/net.c +++ b/net.c @@ -402,7 +402,7 @@ addrretry: if (c->saddr) { free(c->saddr); } - c->saddr = malloc(sizeof(*p->ai_addr)); + c->saddr = malloc(p->ai_addrlen); memcpy(c->saddr, p->ai_addr, p->ai_addrlen); c->addrlen = p->ai_addrlen; -- cgit v1.2.3 From cbe4ae63aecee40674509a2bf23bc5db067e7f3d Mon Sep 17 00:00:00 2001 From: Mark Nunberg Date: Mon, 5 Mar 2018 12:17:49 +0200 Subject: Handle connection errors better in blocking mode as well --- async.c | 2 +- net.c | 17 +++++++++++++---- net.h | 2 +- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/async.c b/async.c index 7309344..0cecd30 100644 --- a/async.c +++ b/async.c @@ -508,7 +508,7 @@ void redisProcessCallbacks(redisAsyncContext *ac) { static int __redisAsyncHandleConnect(redisAsyncContext *ac) { int completed = 0; redisContext *c = &(ac->c); - if (redisFinishAsyncConnect(c, &completed) == REDIS_ERR) { + if (redisCheckConnectDone(c, &completed) == REDIS_ERR) { /* Error! */ redisCheckSocketError(c); if (ac->onConnect) ac->onConnect(ac, REDIS_ERR); diff --git a/net.c b/net.c index 7d5588e..a4b3abc 100644 --- a/net.c +++ b/net.c @@ -221,8 +221,10 @@ static int redisContextWaitReady(redisContext *c, long msec) { return REDIS_ERR; } - if (redisCheckSocketError(c) != REDIS_OK) + if (redisCheckConnectDone(c, &res) != REDIS_OK || res == 0) { + redisCheckSocketError(c); return REDIS_ERR; + } return REDIS_OK; } @@ -232,7 +234,7 @@ static int redisContextWaitReady(redisContext *c, long msec) { return REDIS_ERR; } -int redisFinishAsyncConnect(redisContext *c, int *completed) { +int redisCheckConnectDone(redisContext *c, int *completed) { int rc = connect(c->fd, (const struct sockaddr *)c->saddr, c->addrlen); if (rc == 0) { *completed = 1; @@ -410,8 +412,14 @@ addrretry: if (errno == EHOSTUNREACH) { redisContextCloseFd(c); continue; - } else if (errno == EINPROGRESS && !blocking) { - /* This is ok. */ + } else if (errno == EINPROGRESS) { + if (blocking) { + goto wait_for_ready; + } + /* This is ok. + * Note that even when it's in blocking mode, we unset blocking + * for `connect()` + */ } else if (errno == EADDRNOTAVAIL && reuseaddr) { if (++reuses >= REDIS_CONNECT_RETRIES) { goto error; @@ -420,6 +428,7 @@ addrretry: goto addrretry; } } else { + wait_for_ready: if (redisContextWaitReady(c,timeout_msec) != REDIS_OK) goto error; } diff --git a/net.h b/net.h index c35facc..a11594e 100644 --- a/net.h +++ b/net.h @@ -45,6 +45,6 @@ int redisContextConnectBindTcp(redisContext *c, const char *addr, int port, const char *source_addr); int redisContextConnectUnix(redisContext *c, const char *path, const struct timeval *timeout); int redisKeepAlive(redisContext *c, int interval); -int redisFinishAsyncConnect(redisContext *c, int *completed); +int redisCheckConnectDone(redisContext *c, int *completed); #endif -- cgit v1.2.3 From 3cb4fb2395919ff91d29c7ab66634f5211eaca89 Mon Sep 17 00:00:00 2001 From: Mark Nunberg Date: Tue, 25 Sep 2018 20:50:02 -0400 Subject: Skip NXDOMAIN test when using evil ISPs Some ISPs like to inject their own "Suggestions" page whenever you hit NXDOMAIN. This confuses Redis as well as addrinfo (black-holing the route). --- test.c | 55 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/test.c b/test.c index 0f5bfe5..a5eb9c7 100644 --- a/test.c +++ b/test.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -91,7 +92,7 @@ static int disconnect(redisContext *c, int keep_fd) { return -1; } -static redisContext *connect(struct config config) { +static redisContext *do_connect(struct config config) { redisContext *c = NULL; if (config.type == CONN_TCP) { @@ -248,7 +249,7 @@ static void test_append_formatted_commands(struct config config) { char *cmd; int len; - c = connect(config); + c = do_connect(config); test("Append format command: "); @@ -434,18 +435,32 @@ static void test_free_null(void) { static void test_blocking_connection_errors(void) { redisContext *c; - - test("Returns error when host cannot be resolved: "); - c = redisConnect((char*)"idontexist.test", 6379); - test_cond(c->err == REDIS_ERR_OTHER && - (strcmp(c->errstr,"Name or service not known") == 0 || - strcmp(c->errstr,"Can't resolve: idontexist.test") == 0 || - strcmp(c->errstr,"nodename nor servname provided, or not known") == 0 || - strcmp(c->errstr,"No address associated with hostname") == 0 || - strcmp(c->errstr,"Temporary failure in name resolution") == 0 || - strcmp(c->errstr,"hostname nor servname provided, or not known") == 0 || - strcmp(c->errstr,"no address associated with name") == 0)); - redisFree(c); + struct addrinfo hints = {.ai_family = AF_INET}; + struct addrinfo *ai_tmp = NULL; + const char *bad_domain = "idontexist.com"; + + int rv = getaddrinfo(bad_domain, "6379", &hints, &ai_tmp); + if (rv != 0) { + // Address does *not* exist + test("Returns error when host cannot be resolved: "); + // First see if this domain name *actually* resolves to NXDOMAIN + c = redisConnect("dontexist.com", 6379); + test_cond( + c->err == REDIS_ERR_OTHER && + (strcmp(c->errstr, "Name or service not known") == 0 || + strcmp(c->errstr, "Can't resolve: sadkfjaskfjsa.com") == 0 || + strcmp(c->errstr, + "nodename nor servname provided, or not known") == 0 || + strcmp(c->errstr, "No address associated with hostname") == 0 || + strcmp(c->errstr, "Temporary failure in name resolution") == 0 || + strcmp(c->errstr, + "hostname nor servname provided, or not known") == 0 || + strcmp(c->errstr, "no address associated with name") == 0)); + redisFree(c); + } else { + printf("Skipping NXDOMAIN test. Found evil ISP!\n"); + freeaddrinfo(ai_tmp); + } test("Returns error when the port is not open: "); c = redisConnect((char*)"localhost", 1); @@ -463,7 +478,7 @@ static void test_blocking_connection(struct config config) { redisContext *c; redisReply *reply; - c = connect(config); + c = do_connect(config); test("Is able to deliver commands: "); reply = redisCommand(c,"PING"); @@ -544,7 +559,7 @@ static void test_blocking_connection_timeouts(struct config config) { const char *cmd = "DEBUG SLEEP 3\r\n"; struct timeval tv; - c = connect(config); + c = do_connect(config); test("Successfully completes a command when the timeout is not exceeded: "); reply = redisCommand(c,"SET foo fast"); freeReplyObject(reply); @@ -556,7 +571,7 @@ static void test_blocking_connection_timeouts(struct config config) { freeReplyObject(reply); disconnect(c, 0); - c = connect(config); + c = do_connect(config); test("Does not return a reply when the command times out: "); s = write(c->fd, cmd, strlen(cmd)); tv.tv_sec = 0; @@ -590,7 +605,7 @@ static void test_blocking_io_errors(struct config config) { int major, minor; /* Connect to target given by config. */ - c = connect(config); + c = do_connect(config); { /* Find out Redis version to determine the path for the next test */ const char *field = "redis_version:"; @@ -625,7 +640,7 @@ static void test_blocking_io_errors(struct config config) { strcmp(c->errstr,"Server closed the connection") == 0); redisFree(c); - c = connect(config); + c = do_connect(config); test("Returns I/O error on socket timeout: "); struct timeval tv = { 0, 1000 }; assert(redisSetTimeout(c,tv) == REDIS_OK); @@ -659,7 +674,7 @@ static void test_invalid_timeout_errors(struct config config) { } static void test_throughput(struct config config) { - redisContext *c = connect(config); + redisContext *c = do_connect(config); redisReply **replies; int i, num; long long t1, t2; -- cgit v1.2.3