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

[George Joseph]

George Joseph has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/17472 )

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.

This patch makes the client socket non-blocking while connecting,
ensures a connect does not immediately fail due to EINTR "errors",
and adds a connect timeout option.

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, 100 insertions(+), 14 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved; Approved for Submit



diff --git a/include/asterisk/tcptls.h b/include/asterisk/tcptls.h
index 1e66201..1b1b56c 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..b2756d1 100644
--- a/main/tcptls.c
+++ b/main/tcptls.c
@@ -582,20 +582,82 @@
 #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));
+
+		ao2_ref(tcptls_session, -1);
+		return NULL;
 	}
 
 	ast_fd_clear_flags(desc->accept_fd, O_NONBLOCK);
@@ -606,10 +668,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 +689,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 +736,7 @@
 
 	/* Set current info */
 	ast_sockaddr_copy(&desc->old_address, &desc->remote_address);
+
 	return tcptls_session;
 
 error:

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I907571843a83e43c0742b95a64785f4411f02671
Gerrit-Change-Number: 17472
Gerrit-PatchSet: 4
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220105/7b8012d3/attachment.html>