Merge branch 'ssl-read-pull' - #1454

* ssl-read-pull:
  ssl: add some comments for lack of notify_close
  Drop unused le_ssl_ops::err_is_ok
  More SSL_read() to fill big buffer
  Make bufferevent_set_max_single_read() effect

bufferevent_mbedtls.c

@@ -136,11 +136,6 @@ mbedtls_is_want_write(int err)
 {
 	return err == MBEDTLS_ERR_SSL_WANT_WRITE;
 }
-static int mbedtls_err_is_ok(int err)
-{
-	/* What mbedtls_ssl_read() returns when the we can proceed existing data */
-	return err == 0;
-}
 
 static evutil_socket_t
 be_mbedtls_get_fd(void *ssl)
@@ -329,7 +324,6 @@ static struct le_ssl_ops le_mbedtls_ops = {
 	mbedtls_handshake_is_ok,
 	mbedtls_is_want_read,
 	mbedtls_is_want_write,
-	mbedtls_err_is_ok,
 	be_mbedtls_get_fd,
 	be_mbedtls_bio_set_fd,
 	(void (*)(struct bufferevent_ssl *))mbedtls_set_ssl_noops,

bufferevent_openssl.c

@@ -345,13 +345,6 @@ SSL_handshake_is_ok(int err)
 	return err == 1;
 }
 
-static int
-SSL_err_is_ok(int err)
-{
-	/* What SSL_read() returns when the we can proceed existing data */
-	return err == SSL_ERROR_ZERO_RETURN;
-}
-
 static int
 SSL_is_want_read(int err)
 {
@@ -424,7 +417,6 @@ static struct le_ssl_ops le_openssl_ops = {
 	SSL_handshake_is_ok,
 	SSL_is_want_read,
 	SSL_is_want_write,
-	SSL_err_is_ok,
 	(int (*)(void *))be_openssl_get_fd,
 	be_openssl_bio_set_fd,
 	init_bio_counts,

bufferevent_ssl.c

@@ -253,9 +253,10 @@ do_read(struct bufferevent_ssl *bev_ssl, int n_to_read) {
 	/* Requires lock */
 	struct bufferevent *bev = &bev_ssl->bev.bev;
 	struct evbuffer *input = bev->input;
-	int r, n, i, n_used = 0, atmost;
+	int r, n, i = 0, atmost;
 	struct evbuffer_iovec space[2];
 	int result = 0;
+	size_t len = 0;
 
 	if (bev_ssl->bev.read_suspended)
 		return 0;
@@ -268,23 +269,37 @@ do_read(struct bufferevent_ssl *bev_ssl, int n_to_read) {
 	if (n < 0)
 		return OP_ERR;
 
-	for (i=0; i<n; ++i) {
+	for (i = 0; i < n;) {
 		if (bev_ssl->bev.read_suspended)
 			break;
 		bev_ssl->ssl_ops->clear_error();
-		r = bev_ssl->ssl_ops->read(bev_ssl->ssl, space[i].iov_base, space[i].iov_len);
-		if (r>0) {
+		r = bev_ssl->ssl_ops->read(
+			bev_ssl->ssl, (unsigned char *)space[i].iov_base + len, space[i].iov_len - len);
+		if (r > 0) {
 			result |= OP_MADE_PROGRESS;
 			if (bev_ssl->read_blocked_on_write)
 				if (clear_rbow(bev_ssl) < 0)
 					return OP_ERR | result;
-			++n_used;
-			space[i].iov_len = r;
 			bev_ssl->ssl_ops->decrement_buckets(bev_ssl);
+			len += r;
+			if (space[i].iov_len - len > 0) {
+				continue;
+			} else {
+				space[i].iov_len = len;
+				len = 0;
+				++i;
+			}
 		} else {
 			int err = bev_ssl->ssl_ops->get_error(bev_ssl->ssl, r);
 			bev_ssl->ssl_ops->print_err(err);
-			if (bev_ssl->ssl_ops->err_is_ok(err) && result & OP_MADE_PROGRESS) {
+			/* NOTE: we ignore the error in case of some progress was done,
+			 * because currently we do not send close_notify, and this will
+			 * lead to error from SSL_read() (it will return 0, and
+			 * SSL_get_error() will return SSL_ERROR_SSL), and this is because
+			 * of lack of close_notify
+			 *
+			 * But AFAICS some code uses it the same way (i.e. nginx) */
+			if (result & OP_MADE_PROGRESS) {
 				/* Process existing data */
 				break;
 			} else if (bev_ssl->ssl_ops->err_is_want_read(err)) {
@@ -292,7 +307,7 @@ do_read(struct bufferevent_ssl *bev_ssl, int n_to_read) {
 				if (bev_ssl->read_blocked_on_write)
 					if (clear_rbow(bev_ssl) < 0)
 						return OP_ERR | result;
-			} else if(bev_ssl->ssl_ops->err_is_want_write(err)) {
+			} else if (bev_ssl->ssl_ops->err_is_want_write(err)) {
 				/* This read operation requires a write, and the
 				 * underlying is full */
 				if (!bev_ssl->read_blocked_on_write)
@@ -306,8 +321,13 @@ do_read(struct bufferevent_ssl *bev_ssl, int n_to_read) {
 		}
 	}
 
-	if (n_used) {
-		evbuffer_commit_space(input, space, n_used);
+	if (len > 0) {
+		space[i].iov_len = len;
+		++i;
+	}
+
+	if (i) {
+		evbuffer_commit_space(input, space, i);
 		if (bev_ssl->underlying)
 			BEV_RESET_GENERIC_READ_TIMEOUT(bev);
 	}
@@ -407,8 +427,6 @@ do_write(struct bufferevent_ssl *bev_ssl, int atmost)
 
 #define WRITE_FRAME 15000
 
-#define READ_DEFAULT 4096
-
 /* Try to figure out how many bytes to read; return 0 if we shouldn't be
  * reading. */
 static int
@@ -416,7 +434,7 @@ bytes_to_read(struct bufferevent_ssl *bev)
 {
 	struct evbuffer *input = bev->bev.bev.input;
 	struct event_watermark *wm = &bev->bev.bev.wm_read;
-	int result = READ_DEFAULT;
+	int result = 0;
 	ev_ssize_t limit;
 	/* XXX 99% of this is generic code that nearly all bufferevents will
 	 * want. */
@@ -439,13 +457,11 @@ bytes_to_read(struct bufferevent_ssl *bev)
 		}
 
 		result = wm->high - evbuffer_get_length(input);
-	} else {
-		result = READ_DEFAULT;
 	}
 
 	/* Respect the rate limit */
 	limit = bufferevent_get_read_max_(&bev->bev);
-	if (result > limit) {
+	if (result == 0 || result > limit) {
 		result = limit;
 	}
 
@@ -913,6 +929,10 @@ be_ssl_destruct(struct bufferevent *bev)
 	if (bev_ssl->bev.options & BEV_OPT_CLOSE_ON_FREE) {
 		if (! bev_ssl->underlying) {
 			evutil_socket_t fd = bev_ssl->ssl_ops->get_fd(bev_ssl);
+			/* NOTE: This is dirty shutdown, to send close_notify one of the
+			 * following should be used:
+			 * - SSL_shutdown()
+			 * - mbedtls_ssl_close_notify() */
 			if (fd >= 0)
 				evutil_closesocket(fd);
 		}

ssl-compat.h

@@ -23,7 +23,6 @@ struct le_ssl_ops {
 	int (*handshake_is_ok)(int err);
 	int (*err_is_want_read)(int err);
 	int (*err_is_want_write)(int err);
-	int (*err_is_ok)(int err);
 	evutil_socket_t (*get_fd)(void *ssl);
 	int (*bio_set_fd)(struct bufferevent_ssl *ssl, evutil_socket_t fd);
 	void (*init_bio_counts)(struct bufferevent_ssl *bev);