[Asterisk-code-review] tcptls.c: refactor client connection to be more robust (asterisk[18])

[Kevin Harwell]

Kevin Harwell has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/17721 )


Change subject: tcptls.c: refactor client connection to be more robust
......................................................................

tcptls.c: refactor client connection to be more robust

The current TCP client connect code, blocks and does not handle EINTR
error case. As well the underlying file descriptor is set to not poll
on reads, which can also cause inadvertent errors.

This patch makes the client socket non-blocking, ensures a connect
does not immediately fail due to EINTR "errors", adds a connect
timeout option, and sets the underyling iostream to poll on read.

The original client start call sets the new timeout option to
"infinite", thus making sure old, orginal behavior is retained.

ASTERISK-29746 #close

Change-Id: I907571843a83e43c0742b95a64785f4411f02671
---
M include/asterisk/tcptls.h
M main/tcptls.c
2 files changed, 101 insertions(+), 16 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/21/17721/1

diff --git a/include/asterisk/tcptls.h b/include/asterisk/tcptls.h
index 1e66201..eb4e72d 100644
--- a/include/asterisk/tcptls.h
+++ b/include/asterisk/tcptls.h
@@ -164,8 +164,30 @@
 };
 
 /*!
-  * \brief attempts to connect and start tcptls session, on error the tcptls_session's
-  * ref count is decremented, fd and file are closed, and NULL is returned.
+  * \brief Attempt to connect, and start a tcptls session within the given timeout
+  *
+  * Note, on error the tcptls_session's ref count is decremented,* fd and file
+  * are closed, and NULL is returned.
+  *
+  * \param tcptls_session The session instance to connect and start
+  * \param timeout How long (in milliseconds) to attempt to connect (-1 equals infinite)
+  *
+  * \return The tcptls_session, or NULL on error
+  */
+struct ast_tcptls_session_instance *ast_tcptls_client_start_timeout(
+	struct ast_tcptls_session_instance *tcptls_session, int timeout);
+
+/*!
+  * \brief Attempt to connect, and start a tcptls session
+  *
+  * Blocks until a connection is established, or an error occurs.
+  *
+  * Note, on error the tcptls_session's ref count is decremented,* fd and file
+  * are closed, and NULL is returned.
+  *
+  * \param tcptls_session The session instance to connect and start
+  *
+  * \return The tcptls_session, or NULL on error
   */
 struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_session_instance *tcptls_session);
 
diff --git a/main/tcptls.c b/main/tcptls.c
index 45eb29b..8d85a29 100644
--- a/main/tcptls.c
+++ b/main/tcptls.c
@@ -582,23 +582,83 @@
 #endif
 }
 
-struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_session_instance *tcptls_session)
+/*!
+ * \internal
+ * \brief Connect a socket
+ *
+ * Attempt to connect to a given address for up to 'timeout' milliseconds. A negative
+ * timeout value equates to an infinite wait time.
+ *
+ * A -1 is returned on error, and an appropriate errno value is set based on the
+ * type of error.
+ *
+ * \param sockfd The socket file descriptor
+ * \param addr The address to connect to
+ * \param timeout How long, in milliseconds, to attempt to connect
+ *
+ * \return 0 if successfully connected, -1 otherwise
+ */
+static int socket_connect(int sockfd, const struct ast_sockaddr *addr, int timeout)
+{
+	int optval = 0;
+	socklen_t optlen = sizeof(int);
+
+	errno = 0;
+
+	if (ast_connect(sockfd, addr)) {
+		int res;
+
+		/*
+		 * A connect failure could mean things are still in progress.
+		 * If so wait for it to complete.
+		 */
+
+		if (errno != EINPROGRESS) {
+			return -1;
+		}
+
+		while ((res = ast_wait_for_output(sockfd, timeout)) != 1) {
+			if (res == 0) {
+				errno = ETIMEDOUT;
+				return -1;
+			}
+
+			if (errno != EINTR) {
+				return -1;
+			}
+		}
+	}
+
+	/* Check the status to ensure it actually connected successfully */
+	if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &optval, &optlen) < 0) {
+		return -1;
+	}
+
+	if (optval) {
+		errno = optval;
+		return -1;
+	}
+
+	return 0;
+}
+
+struct ast_tcptls_session_instance *ast_tcptls_client_start_timeout(
+	struct ast_tcptls_session_instance *tcptls_session, int timeout)
 {
 	struct ast_tcptls_session_args *desc;
 
 	if (!(desc = tcptls_session->parent)) {
-		goto client_start_error;
+		ao2_ref(tcptls_session, -1);
+		return NULL;
 	}
 
-	if (ast_connect(desc->accept_fd, &desc->remote_address)) {
-		ast_log(LOG_ERROR, "Unable to connect %s to %s: %s\n",
-			desc->name,
-			ast_sockaddr_stringify(&desc->remote_address),
-			strerror(errno));
-		goto client_start_error;
-	}
+	if (socket_connect(desc->accept_fd, &desc->remote_address, timeout)) {
+		ast_log(LOG_WARNING, "Unable to connect %s to %s: %s\n", desc->name,
+				ast_sockaddr_stringify(&desc->remote_address), strerror(errno));
 
-	ast_fd_clear_flags(desc->accept_fd, O_NONBLOCK);
+		ao2_ref(tcptls_session, -1);
+		return NULL;
+	}
 
 	if (desc->tls_cfg) {
 		desc->tls_cfg->enabled = 1;
@@ -606,10 +666,11 @@
 	}
 
 	return handle_tcptls_connection(tcptls_session);
+}
 
-client_start_error:
-	ao2_ref(tcptls_session, -1);
-	return NULL;
+struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_session_instance *tcptls_session)
+{
+	return ast_tcptls_client_start_timeout(tcptls_session, -1);
 }
 
 struct ast_tcptls_session_instance *ast_tcptls_client_create(struct ast_tcptls_session_args *desc)
@@ -626,7 +687,7 @@
 	/* If we return early, there is no connection */
 	ast_sockaddr_setnull(&desc->old_address);
 
-	fd = desc->accept_fd = socket(ast_sockaddr_is_ipv6(&desc->remote_address) ?
+	fd = desc->accept_fd = ast_socket_nonblock(ast_sockaddr_is_ipv6(&desc->remote_address) ?
 				      AF_INET6 : AF_INET, SOCK_STREAM, IPPROTO_TCP);
 	if (desc->accept_fd < 0) {
 		ast_log(LOG_ERROR, "Unable to allocate socket for %s: %s\n",
@@ -673,6 +734,8 @@
 
 	/* Set current info */
 	ast_sockaddr_copy(&desc->old_address, &desc->remote_address);
+	/* Wait for input on socket reads */
+	ast_iostream_set_exclusive_input(tcptls_session->stream, 1);
 	return tcptls_session;
 
 error:

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/17721
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: I907571843a83e43c0742b95a64785f4411f02671
Gerrit-Change-Number: 17721
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211215/584e2c40/attachment.html>