From 8f48b18621fbde6db52e05dd28d5be21dec5d2c8 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Wed, 19 Feb 2014 08:07:30 +0000 Subject: ext/socket/init.c (rsock_connect): simplify and avoid SO_ERROR Only retry connect() on EINTR/ERESTART. Avoid getsockopt(SO_ERROR) since its behavior is unpredictable on different systems. This should prevent infinite loops due to difficult-to-read platform-dependent cases. --- ext/socket/init.c | 195 +++++++++++++++++++----------------------------------- 1 file changed, 69 insertions(+), 126 deletions(-) diff --git a/ext/socket/init.c b/ext/socket/init.c index dc6b531bd6..11d44b1f51 100644 --- a/ext/socket/init.c +++ b/ext/socket/init.c @@ -342,61 +342,24 @@ rsock_socket(int domain, int type, int proto) static int wait_connectable(int fd) { - int sockerr; - socklen_t sockerrlen; - int revents; - int ret; - - for (;;) { - /* - * Stevens book says, successful finish turn on RB_WAITFD_OUT and - * failure finish turn on both RB_WAITFD_IN and RB_WAITFD_OUT. - */ - revents = rb_wait_for_single_fd(fd, RB_WAITFD_IN|RB_WAITFD_OUT, NULL); - - if (revents & (RB_WAITFD_IN|RB_WAITFD_OUT)) { - sockerrlen = (socklen_t)sizeof(sockerr); - ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen); - - /* - * Solaris getsockopt(SO_ERROR) return -1 and set errno - * in getsockopt(). Let's return immediately. - */ - if (ret < 0) - break; - if (sockerr == 0) - continue; /* workaround for winsock */ - - /* BSD and Linux use sockerr. */ - errno = sockerr; - ret = -1; - break; - } - - if ((revents & (RB_WAITFD_IN|RB_WAITFD_OUT)) == RB_WAITFD_OUT) { - ret = 0; - break; - } - } + /* + * Stevens book says, successful finish turn on RB_WAITFD_OUT and + * failure finish turn on both RB_WAITFD_IN and RB_WAITFD_OUT. + * + * We do not care about any socket errors right now, since any + * subsequent write/read call must have error checking anyways, + * so a preliminary checking for errors is redundant. + * + * Note: rb_wait_for_single_fd already retries on EINTR/ERESTART + */ + int revents = rb_wait_for_single_fd(fd, RB_WAITFD_IN|RB_WAITFD_OUT, NULL); - return ret; + if (revents < 0) + rb_bug_errno("rb_wait_for_single_fd used improperly", errno); + errno = 0; + return 0; } -#ifdef __CYGWIN__ -#define WAIT_IN_PROGRESS 10 -#endif -#ifdef __APPLE__ -#define WAIT_IN_PROGRESS 10 -#endif -#ifdef __linux__ -/* returns correct error */ -#define WAIT_IN_PROGRESS 0 -#endif -#ifndef WAIT_IN_PROGRESS -/* BSD origin code apparently has a problem */ -#define WAIT_IN_PROGRESS 1 -#endif - struct connect_arg { int fd; const struct sockaddr *sockaddr; @@ -419,17 +382,38 @@ socks_connect_blocking(void *data) } #endif +/* + * n.b.: any comments on this style of conditional case? + * perhaps we may generate it via ERB and use it elsewhere to avoid + * mixing C and CPP control flow + */ +#ifdef ERESTART +# define case_ERESTART case ERESTART: +#else +# define case_ERESTART /* ERESTART not defined by this system */ +#endif +#ifdef EINPROGRESS +# define case_EINPROGRESS case EINPROGRESS: +#else +# define case_EINPROGRESS /* EINPROGRESS not defined by this system */ +#endif +#ifdef EALREADY +# define case_EALREADY case EALREADY: +#else +# define case_EALREADY /* EALREADY not defined by this system */ +#endif +#ifdef EISCONN +# define case_EISCONN case EISCONN: +#else +# define case_EISCONN /* EISCONN not defined by this system */ +#endif + int rsock_connect(int fd, const struct sockaddr *sockaddr, int len, int socks) { int status; rb_blocking_function_t *func = connect_blocking; struct connect_arg arg; -#if WAIT_IN_PROGRESS > 0 - int wait_in_progress = -1; - int sockerr; - socklen_t sockerrlen; -#endif arg.fd = fd; arg.sockaddr = sockaddr; @@ -437,76 +421,35 @@ rsock_connect(int fd, const struct sockaddr *sockaddr, int len, int socks) #if defined(SOCKS) && !defined(SOCKS5) if (socks) func = socks_connect_blocking; #endif - for (;;) { - status = (int)BLOCKING_REGION_FD(func, &arg); - if (status < 0) { - switch (errno) { - case EINTR: -#if defined(ERESTART) - case ERESTART: -#endif - continue; - - case EAGAIN: -#ifdef EINPROGRESS - case EINPROGRESS: -#endif -#if WAIT_IN_PROGRESS > 0 - sockerrlen = (socklen_t)sizeof(sockerr); - status = getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen); - if (status) break; - if (sockerr) { - status = -1; - errno = sockerr; - break; - } -#endif -#ifdef EALREADY - case EALREADY: -#endif -#if WAIT_IN_PROGRESS > 0 - wait_in_progress = WAIT_IN_PROGRESS; -#endif - status = wait_connectable(fd); - if (status) { - break; - } - errno = 0; - continue; - -#if WAIT_IN_PROGRESS > 0 - case EINVAL: - if (wait_in_progress-- > 0) { - /* - * connect() after EINPROGRESS returns EINVAL on - * some platforms, need to check true error - * status. - */ - sockerrlen = (socklen_t)sizeof(sockerr); - status = getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen); - if (!status && !sockerr) { - struct timeval tv = {0, 100000}; - rb_thread_wait_for(tv); - continue; - } - status = -1; - errno = sockerr; - } - break; -#endif - -#ifdef EISCONN - case EISCONN: - status = 0; - errno = 0; - break; -#endif - default: - break; - } - } - return status; + status = (int)BLOCKING_REGION_FD(func, &arg); + + if (status < 0) { + switch (errno) { + retry_intr: + case EINTR: + case_ERESTART + status = (int)BLOCKING_REGION_FD(func, &arg); + if (status < 0) { + switch (errno) { + case EINTR: + case_ERESTART + goto retry_intr; + case_EINPROGRESS + case_EALREADY + case EAGAIN: + return wait_connectable(fd); + case_EISCONN + errno = 0; + return 0; /* success */ + } + } + return status; + case EAGAIN: + case_EINPROGRESS + return wait_connectable(fd); + } } + return status; } static void -- cgit v1.2.1-29-g51bb