Redo fetch_read() using non-blocking sockets. This is necessary to

avoid a hang in the SSL case if the server sends a close notification
before we are done reading.  In the non-SSL case, it can provide a
minor (but probably not noticeable) performance improvement for small
transfers.

MFC after:	3 weeks

1 files changed, 89 insertions, 20 deletions

diff --git a/lib/libfetch/common.c b/lib/libfetch/common.c
index c54804b..adeeb45 100644
--- a/lib/libfetch/common.c
+++ b/lib/libfetch/common.c
@@ -38,6 +38,7 @@ __FBSDID("$FreeBSD$");
 
 #include <ctype.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <netdb.h>
 #include <pwd.h>
 #include <stdarg.h>
@@ -295,7 +296,8 @@ fetch_connect(const char *host, int port, int af, int verbose)
 			close(sd);
 			continue;
 		}
-		if (connect(sd, res->ai_addr, res->ai_addrlen) == 0)
+		if (connect(sd, res->ai_addr, res->ai_addrlen) == 0 &&
+		    fcntl(sd, F_SETFL, O_NONBLOCK) == 0)
 			break;
 		close(sd);
 	}
@@ -319,6 +321,7 @@ fetch_connect(const char *host, int port, int af, int verbose)
 int
 fetch_ssl(conn_t *conn, int verbose)
 {
+	int ret, ssl_err;
 
 #ifdef WITH_SSL
 	/* Init the SSL library and context */
@@ -339,9 +342,13 @@ fetch_ssl(conn_t *conn, int verbose)
 		return (-1);
 	}
 	SSL_set_fd(conn->ssl, conn->sd);
-	if (SSL_connect(conn->ssl) == -1){
-		ERR_print_errors_fp(stderr);
-		return (-1);
+	while ((ret = SSL_connect(conn->ssl)) == -1) {
+		ssl_err = SSL_get_error(conn->ssl, ret);
+		if (ssl_err != SSL_ERROR_WANT_READ &&
+		    ssl_err != SSL_ERROR_WANT_WRITE) {
+			ERR_print_errors_fp(stderr);
+			return (-1);
+		}
 	}
 
 	if (verbose) {
@@ -370,6 +377,46 @@ fetch_ssl(conn_t *conn, int verbose)
 #endif
 }
 
+#define FETCH_READ_WAIT		-2
+#define FETCH_READ_ERROR	-1
+#define FETCH_READ_DONE		 0
+
+#ifdef WITH_SSL
+static ssize_t
+fetch_ssl_read(SSL *ssl, char *buf, size_t len)
+{
+	ssize_t rlen;
+	int ssl_err;
+
+	rlen = SSL_read(ssl, buf, len);
+	if (rlen < 0) {
+		ssl_err = SSL_get_error(ssl, rlen);
+		if (ssl_err == SSL_ERROR_WANT_READ ||
+		    ssl_err == SSL_ERROR_WANT_WRITE) {
+			return (FETCH_READ_WAIT);
+		} else {
+			ERR_print_errors_fp(stderr);
+			return (FETCH_READ_ERROR);
+		}
+	}
+	return (rlen);
+}
+#endif
+
+static ssize_t
+fetch_socket_read(int sd, char *buf, size_t len)
+{
+	ssize_t rlen;
+
+	rlen = read(sd, buf, len);
+	if (rlen < 0) {
+		if (errno == EAGAIN || (errno == EINTR && fetchRestartCalls))
+			return (FETCH_READ_WAIT);
+		else
+			return (FETCH_READ_ERROR);
+	}
+	return (rlen);
+}
 
 /*
  * Read a character from a connection w/ timeout
@@ -390,6 +437,43 @@ fetch_read(conn_t *conn, char *buf, size_t len)
 
 	total = 0;
 	while (len > 0) {
+		/*
+		 * The socket is non-blocking.  Instead of the canonical
+		 * select() -> read(), we do the following:
+		 *
+		 * 1) call read() or SSL_read().
+		 * 2) if an error occurred, return -1.
+		 * 3) if we received data but we still expect more,
+		 *    update our counters and loop.
+		 * 4) if read() or SSL_read() signaled EOF, return.
+		 * 5) if we did not receive any data but we're not at EOF,
+		 *    call select().
+		 *
+		 * In the SSL case, this is necessary because if we
+		 * receive a close notification, we have to call
+		 * SSL_read() one additional time after we've read
+		 * everything we received.
+		 *
+		 * In the non-SSL case, it may improve performance (very
+		 * slightly) when reading small amounts of data.
+		 */
+#ifdef WITH_SSL
+		if (conn->ssl != NULL)
+			rlen = fetch_ssl_read(conn->ssl, buf, len);
+		else
+#endif
+			rlen = fetch_socket_read(conn->sd, buf, len);
+		if (rlen == 0) {
+			break;
+		} else if (rlen > 0) {
+			len -= rlen;
+			buf += rlen;
+			total += rlen;
+			continue;
+		} else if (rlen == FETCH_READ_ERROR) {
+			return (-1);
+		}
+		// assert(rlen == FETCH_READ_WAIT);
 		while (fetchTimeout && !FD_ISSET(conn->sd, &readfds)) {
 			FD_SET(conn->sd, &readfds);
 			gettimeofday(&now, NULL);
@@ -413,22 +497,6 @@ fetch_read(conn_t *conn, char *buf, size_t len)
 				return (-1);
 			}
 		}
-#ifdef WITH_SSL
-		if (conn->ssl != NULL)
-			rlen = SSL_read(conn->ssl, buf, len);
-		else
-#endif
-			rlen = read(conn->sd, buf, len);
-		if (rlen == 0)
-			break;
-		if (rlen < 0) {
-			if (errno == EINTR && fetchRestartCalls)
-				continue;
-			return (-1);
-		}
-		len -= rlen;
-		buf += rlen;
-		total += rlen;
 	}
 	return (total);
 }
@@ -548,6 +616,7 @@ fetch_writev(conn_t *conn, struct iovec *iov, int iovcnt)
 			wlen = writev(conn->sd, iov, iovcnt);
 		if (wlen == 0) {
 			/* we consider a short write a failure */
+			/* XXX perhaps we shouldn't in the SSL case */
 			errno = EPIPE;
 			fetch_syserr();
 			return (-1);