On Sat, Jan 7, 2017 at 2:30 PM, Peter Moody <mindrot at hda3.com> wrote:> so I spent a bit of time looking at this and it seems like the only > way to go, at least if I want to keep it in ssh_connect_direct(), is > to use pthreads. further, it seems like getting that accepted is > something of a long shot:Sorry, pthreads is a non-starter. I would have thought that using non-blocking connect (ie set O_NONBLOCK on the fds, initiate the connections then select on the set until one succeeds) would be feasible, though.> so, approaching this from a different angle, what if I wanted to have > something else establish the tcp connection and then fork/dup2/exec > ssh and pass off the fd's for the network connection?That's how ProxyComand and ProxyUseFdpass work. Your dialler is a separate program so it can do whatever you like, including use pthreads if that's your thing. -- Darren Tucker (dtucker at zip.com.au) GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new) Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
On Thu, Jan 12, 2017 at 01:49:29PM +1100, Darren Tucker wrote: [...]> I would have thought that using non-blocking connect (ie set > O_NONBLOCK on the fds, initiate the connections then select on the set > until one succeeds) would be feasible, though.In fact most of the required pieces are already there. Below is a rough, barely tested patch that nonetheless does seem to sort of work. So, it's at least feasible. I'm still not sure it's worth doing, though. I can see some downsides: it'll spam server logs with "Did not receive identification string" and consume MaxStartups connections. Maybe best to leave it to a custom ProxyCommand for people who really want the functionality. diff --git a/sshconnect.c b/sshconnect.c index 96b91ce..bb8b6ee 100644 --- a/sshconnect.c +++ b/sshconnect.c @@ -328,89 +328,6 @@ ssh_create_socket(int privileged, struct addrinfo *ai) return sock; } -static int -timeout_connect(int sockfd, const struct sockaddr *serv_addr, - socklen_t addrlen, int *timeoutp) -{ - fd_set *fdset; - struct timeval tv, t_start; - socklen_t optlen; - int optval, rc, result = -1; - - gettimeofday(&t_start, NULL); - - if (*timeoutp <= 0) { - result = connect(sockfd, serv_addr, addrlen); - goto done; - } - - set_nonblock(sockfd); - rc = connect(sockfd, serv_addr, addrlen); - if (rc == 0) { - unset_nonblock(sockfd); - result = 0; - goto done; - } - if (errno != EINPROGRESS) { - result = -1; - goto done; - } - - fdset = xcalloc(howmany(sockfd + 1, NFDBITS), - sizeof(fd_mask)); - FD_SET(sockfd, fdset); - ms_to_timeval(&tv, *timeoutp); - - for (;;) { - rc = select(sockfd + 1, NULL, fdset, NULL, &tv); - if (rc != -1 || errno != EINTR) - break; - } - - switch (rc) { - case 0: - /* Timed out */ - errno = ETIMEDOUT; - break; - case -1: - /* Select error */ - debug("select: %s", strerror(errno)); - break; - case 1: - /* Completed or failed */ - optval = 0; - optlen = sizeof(optval); - if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &optval, - &optlen) == -1) { - debug("getsockopt: %s", strerror(errno)); - break; - } - if (optval != 0) { - errno = optval; - break; - } - result = 0; - unset_nonblock(sockfd); - break; - default: - /* Should not occur */ - fatal("Bogus return (%d) from select()", rc); - } - - free(fdset); - - done: - if (result == 0 && *timeoutp > 0) { - ms_subtract_diff(&t_start, timeoutp); - if (*timeoutp <= 0) { - errno = ETIMEDOUT; - result = -1; - } - } - - return (result); -} - /* * Opens a TCP/IP connection to the remote server on the given host. * The address of the remote host will be returned in hostaddr. @@ -427,15 +344,25 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, struct sockaddr_storage *hostaddr, u_short port, int family, int connection_attempts, int *timeout_ms, int want_keepalive, int needpriv) { - int on = 1; - int sock = -1, attempt; + int i, on = 1, rc, inprogress = 0; + int sock = -1, connected_sock = -1, attempt, fdmax; char ntop[NI_MAXHOST], strport[NI_MAXSERV]; struct addrinfo *ai; + struct timeval tv, t_start; + fd_set fdset; + socklen_t rlen; +#define MAXCONNECT 16 + int inprogress_fd[MAXCONNECT]; + void *ai_addr[MAXCONNECT]; + size_t ai_len[MAXCONNECT]; debug2("%s: needpriv %d", __func__, needpriv); memset(ntop, 0, sizeof(ntop)); memset(strport, 0, sizeof(strport)); + FD_ZERO(&fdset); + gettimeofday(&t_start, NULL); + for (attempt = 0; attempt < connection_attempts; attempt++) { if (attempt > 0) { /* Sleep a moment before retrying. */ @@ -443,10 +370,11 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, debug("Trying again..."); } /* - * Loop through addresses for this host, and try each one in - * sequence until the connection succeeds. + * Loop through addresses for this host, initiate a nonblocking + * connnection then see which one succeeds. */ - for (ai = aitop; ai; ai = ai->ai_next) { + for (ai = aitop; ai != NULL && inprogress < MAXCONNECT && + connected_sock == -1; ai = ai->ai_next) { if (ai->ai_family != AF_INET && ai->ai_family != AF_INET6) continue; @@ -465,24 +393,56 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, /* Any error is already output */ continue; - if (timeout_connect(sock, ai->ai_addr, ai->ai_addrlen, - timeout_ms) >= 0) { - /* Successful connection. */ - memcpy(hostaddr, ai->ai_addr, ai->ai_addrlen); + set_nonblock(sock); + if (connect(sock, ai->ai_addr, ai->ai_addrlen) == 0) { + debug("connection established immediately"); + connected_sock = sock; + } else if (errno == EINPROGRESS) + debug("nonblocking connect fd %d", sock); + else + continue; + inprogress_fd[inprogress] = sock; + ai_len[inprogress] = ai->ai_addrlen; + ai_addr[inprogress] = ai->ai_addr; + inprogress++; + } + } + + for (i = 0; i < inprogress; i++) + unset_nonblock(inprogress_fd[i]); + + ms_to_timeval(&tv, *timeout_ms); + while (connected_sock == -1 && timeout_ms > 0) { + FD_ZERO(&fdset); + fdmax = 0; + for (i = 0; i < inprogress; i++) { + FD_SET(inprogress_fd[i], &fdset); + fdmax = MAX(fdmax, inprogress_fd[i]); + } + rc = select(fdmax + 1, NULL, &fdset, NULL, &tv); + ms_subtract_diff(&t_start, timeout_ms); + rlen = sizeof(rc); + for (i = 0; i < fdmax; i++) { + if (!FD_ISSET(i, &fdset)) + continue; + if (getsockopt(i, SOL_SOCKET, SO_ERROR, &rc, &rlen) + == 0 && rc == 0) { + debug("connected to fd %d", i); + connected_sock = i; break; - } else { - debug("connect to address %s port %s: %s", - ntop, strport, strerror(errno)); - close(sock); - sock = -1; } } - if (sock != -1) - break; /* Successful connection. */ + } + + for (i = 0; i < inprogress; i++) { + if (connected_sock == inprogress_fd[i]) + memcpy(hostaddr, ai_addr[i], ai_len[i]); + else + close(inprogress_fd[i]); } /* Return failure if we didn't get a successful connection. */ - if (sock == -1) { + if (connected_sock == -1) { error("ssh: connect to host %s port %s: %s", host, strport, strerror(errno)); return (-1); @@ -492,12 +452,12 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, /* Set SO_KEEPALIVE if requested. */ if (want_keepalive && - setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, (void *)&on, + setsockopt(connected_sock, SOL_SOCKET, SO_KEEPALIVE, (void *)&on, sizeof(on)) < 0) error("setsockopt SO_KEEPALIVE: %.100s", strerror(errno)); /* Set the connection. */ - packet_set_connection(sock, sock); + packet_set_connection(connected_sock, connected_sock); return 0; } -- Darren Tucker (dtucker at zip.com.au) GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new) Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
On Thu, Jan 12, 2017 at 04:52:05PM +1100, Darren Tucker wrote:> On Thu, Jan 12, 2017 at 01:49:29PM +1100, Darren Tucker wrote: > [...] > > I would have thought that using non-blocking connect (ie set > > O_NONBLOCK on the fds, initiate the connections then select on the set > > until one succeeds) would be feasible, though. > > In fact most of the required pieces are already there. Below is a > rough, barely tested patch that nonetheless does seem to sort of work. > > So, it's at least feasible. I'm still not sure it's worth doing, though. > I can see some downsides: it'll spam server logs with "Did not receive > identification string" and consume MaxStartups connections. Maybe best > to leave it to a custom ProxyCommand for people who really want the > functionality.Tested it a bit, found it didn't handle failed connections. diff --git a/sshconnect.c b/sshconnect.c index 96b91ce..8c1f54d 100644 --- a/sshconnect.c +++ b/sshconnect.c @@ -328,89 +328,6 @@ ssh_create_socket(int privileged, struct addrinfo *ai) return sock; } -static int -timeout_connect(int sockfd, const struct sockaddr *serv_addr, - socklen_t addrlen, int *timeoutp) -{ - fd_set *fdset; - struct timeval tv, t_start; - socklen_t optlen; - int optval, rc, result = -1; - - gettimeofday(&t_start, NULL); - - if (*timeoutp <= 0) { - result = connect(sockfd, serv_addr, addrlen); - goto done; - } - - set_nonblock(sockfd); - rc = connect(sockfd, serv_addr, addrlen); - if (rc == 0) { - unset_nonblock(sockfd); - result = 0; - goto done; - } - if (errno != EINPROGRESS) { - result = -1; - goto done; - } - - fdset = xcalloc(howmany(sockfd + 1, NFDBITS), - sizeof(fd_mask)); - FD_SET(sockfd, fdset); - ms_to_timeval(&tv, *timeoutp); - - for (;;) { - rc = select(sockfd + 1, NULL, fdset, NULL, &tv); - if (rc != -1 || errno != EINTR) - break; - } - - switch (rc) { - case 0: - /* Timed out */ - errno = ETIMEDOUT; - break; - case -1: - /* Select error */ - debug("select: %s", strerror(errno)); - break; - case 1: - /* Completed or failed */ - optval = 0; - optlen = sizeof(optval); - if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &optval, - &optlen) == -1) { - debug("getsockopt: %s", strerror(errno)); - break; - } - if (optval != 0) { - errno = optval; - break; - } - result = 0; - unset_nonblock(sockfd); - break; - default: - /* Should not occur */ - fatal("Bogus return (%d) from select()", rc); - } - - free(fdset); - - done: - if (result == 0 && *timeoutp > 0) { - ms_subtract_diff(&t_start, timeoutp); - if (*timeoutp <= 0) { - errno = ETIMEDOUT; - result = -1; - } - } - - return (result); -} - /* * Opens a TCP/IP connection to the remote server on the given host. * The address of the remote host will be returned in hostaddr. @@ -427,15 +344,25 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, struct sockaddr_storage *hostaddr, u_short port, int family, int connection_attempts, int *timeout_ms, int want_keepalive, int needpriv) { - int on = 1; - int sock = -1, attempt; + int i, j, on = 1, rc, inprogress = 0, lasterr = 0, fds = -1; + int sock = -1, connected_sock = -1, attempt, fdmax; char ntop[NI_MAXHOST], strport[NI_MAXSERV]; struct addrinfo *ai; + struct timeval tv, t_start; + fd_set fdset; + socklen_t rlen = sizeof(rc); +#define MAXCONNECT 16 + int inprogress_fd[MAXCONNECT]; + void *ai_addr[MAXCONNECT]; + size_t ai_len[MAXCONNECT]; debug2("%s: needpriv %d", __func__, needpriv); memset(ntop, 0, sizeof(ntop)); memset(strport, 0, sizeof(strport)); + FD_ZERO(&fdset); + gettimeofday(&t_start, NULL); + for (attempt = 0; attempt < connection_attempts; attempt++) { if (attempt > 0) { /* Sleep a moment before retrying. */ @@ -443,10 +370,11 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, debug("Trying again..."); } /* - * Loop through addresses for this host, and try each one in - * sequence until the connection succeeds. + * Loop through addresses for this host, initiate a nonblocking + * connnection then see which one succeeds. */ - for (ai = aitop; ai; ai = ai->ai_next) { + for (ai = aitop; ai != NULL && inprogress < MAXCONNECT && + connected_sock == -1; ai = ai->ai_next) { if (ai->ai_family != AF_INET && ai->ai_family != AF_INET6) continue; @@ -465,26 +393,72 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, /* Any error is already output */ continue; - if (timeout_connect(sock, ai->ai_addr, ai->ai_addrlen, - timeout_ms) >= 0) { - /* Successful connection. */ - memcpy(hostaddr, ai->ai_addr, ai->ai_addrlen); - break; - } else { - debug("connect to address %s port %s: %s", - ntop, strport, strerror(errno)); - close(sock); - sock = -1; + set_nonblock(sock); + if (connect(sock, ai->ai_addr, ai->ai_addrlen) == 0) { + debug("connection established immediately"); + connected_sock = sock; + } else if (errno == EINPROGRESS) + debug("nonblocking connect fd %d", sock); + else + continue; + inprogress_fd[inprogress] = sock; + ai_len[inprogress] = ai->ai_addrlen; + ai_addr[inprogress] = ai->ai_addr; + inprogress++; + } + } + + ms_to_timeval(&tv, *timeout_ms); + while (connected_sock == -1 && timeout_ms > 0) { + FD_ZERO(&fdset); + fds = fdmax = 0; + for (i = 0; i < inprogress; i++) { + if (inprogress_fd[i] == -1) + continue; + FD_SET(inprogress_fd[i], &fdset); + fdmax = MAX(fdmax, inprogress_fd[i]); + fds++; + } + if (fds == 0) /* no descriptors left */ + break; + rc = select(fdmax + 1, NULL, &fdset, NULL, &tv); + ms_subtract_diff(&t_start, timeout_ms); + for (i = 0; i <= fdmax; i++) { + if (!FD_ISSET(i, &fdset)) + continue; + if (getsockopt(i, SOL_SOCKET, SO_ERROR, &rc, &rlen) + == 0) { + if (rc == EINPROGRESS) { + ; + } else if (rc == 0) { + debug("connected to fd %d", i); + connected_sock = i; + unset_nonblock(connected_sock); + break; + } else { + debug("fd %d error %d (%s)", i, rc, + strerror(rc)); + lasterr = rc; + close(i); + for (j = 0; j < inprogress; j++) + if (inprogress_fd[j] == i) + inprogress_fd[j] = -1; + } } } - if (sock != -1) - break; /* Successful connection. */ + } + + for (i = 0; i < inprogress; i++) { + if (connected_sock == inprogress_fd[i]) + memcpy(hostaddr, ai_addr[i], ai_len[i]); + else + close(inprogress_fd[i]); } /* Return failure if we didn't get a successful connection. */ - if (sock == -1) { + if (connected_sock == -1) { error("ssh: connect to host %s port %s: %s", - host, strport, strerror(errno)); + host, strport, strerror(lasterr)); return (-1); } @@ -492,12 +466,12 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop, /* Set SO_KEEPALIVE if requested. */ if (want_keepalive && - setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, (void *)&on, + setsockopt(connected_sock, SOL_SOCKET, SO_KEEPALIVE, (void *)&on, sizeof(on)) < 0) error("setsockopt SO_KEEPALIVE: %.100s", strerror(errno)); /* Set the connection. */ - packet_set_connection(sock, sock); + packet_set_connection(connected_sock, connected_sock); return 0; } -- Darren Tucker (dtucker at zip.com.au) GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new) Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
On Wed, Jan 11, 2017 at 6:49 PM, Darren Tucker <dtucker at zip.com.au> wrote:> On Sat, Jan 7, 2017 at 2:30 PM, Peter Moody <mindrot at hda3.com> wrote: >> so I spent a bit of time looking at this and it seems like the only >> way to go, at least if I want to keep it in ssh_connect_direct(), is >> to use pthreads. further, it seems like getting that accepted is >> something of a long shot: > > Sorry, pthreads is a non-starter. > > I would have thought that using non-blocking connect (ie set > O_NONBLOCK on the fds, initiate the connections then select on the set > until one succeeds) would be feasible, though.d'oh, you're absolutely right. I've spent so much time in golang over the last 18 months that I'd completely forgotten about O_NONBLOCK.>> so, approaching this from a different angle, what if I wanted to have >> something else establish the tcp connection and then fork/dup2/exec >> ssh and pass off the fd's for the network connection? > > That's how ProxyComand and ProxyUseFdpass work. Your dialler is a > separate program so it can do whatever you like,oh man, I think this is exactly what I want. I searched the manpage for FD and fd, missed Fd I'll let you know if this doesn't work but I suspect this is perfect for my use-case. Thanks!> including use > pthreads if that's your thing.you take that back right now :)> -- > Darren Tucker (dtucker at zip.com.au) > GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new) > Good judgement comes with experience. Unfortunately, the experience > usually comes from bad judgement.