Merged revisions 225445 via svnmerge from

https://origsvn.digium.com/svn/asterisk/trunk

........
  r225445 | dvossel | 2009-10-22 14:55:51 -0500 (Thu, 22 Oct 2009) | 50 lines
  
  SIP TCP/TLS: move client connection setup/write into tcp helper thread, various related locking/memory fixes.
  
          What this patch fixes
  1.Moves sip TCP/TLS connection setup into the TCP helper thread:
    Connection setup takes awhile and before this it was being
    done while holding the monitor lock.
  2.Moves TCP/TLS writing to the TCP helper thread:  Through the
    use of a packet queue and an alert pipe, the TCP helper thread
    can now be woken up to write data as well as read data.
  3.Locking error: sip_xmit returned an XMIT_ERROR without giving
    up the tcptls_session lock.  This lock has been completely removed
    from sip_xmit and placed in the new sip_tcptls_write() function.
  4.Memory leak:  When creating a tcptls_client the tls_cfg was alloced
    but never freed unless the tcptls_session failed to start.  Now the
    session_args for a sip client are an ao2 object which frees the
    tls_cfg on destruction.
  5.Pointer to stack variable: During sip_prepare_socket the creation
    of a client's ast_tcptls_session_args was done on the stack and
    stored as a pointer in the newly created tcptls_session.  Depending
    on the events that followed, there was a slight possibility that
    pointer could have been accessed after the stack returned.  Given
    the new changes, it is always accessed after the stack returns
    which is why I found it.
  
  Notable code changes
  1.I broke tcptls.c's ast_tcptls_client_start() function into two
    functions.  One for creating and allocating the new tcptls_session,
    and a separate one for starting and handling the new connection.
    This allowed me to create the tcptls_session, launch the helper
    thread, and then establish the connection within the helper thread.
  2.Writes to a tcptls_session are now done within the helper thread.
    This is done by using an alert pipe to wake up the thread if new
    data needs to be sent.  The thread's sip_threadinfo object contains
    the alert pipe as well as the packet queue.
  3.Since the threadinfo object contains the alert pipe, it must now be
    accessed outside of the helper thread for every write (queuing of a
    packet).  For easy lookup, I moved the threadinfo objects from a
    linked list to an ao2_container.
  
  (closes issue #13136)
  Reported by: pabelanger
  Tested by: dvossel, whys
  
  (closes issue #15894)
  Reported by: dvossel
  Tested by: dvossel
  
  Review: https://reviewboard.asterisk.org/r/380/
........


git-svn-id: http://svn.digium.com/svn/asterisk/branches/1.6.2@225489 f38db490-d61c-443f-a65b-d21fe96a405b

1 files changed, 41 insertions, 27 deletions

diff --git a/main/tcptls.c b/main/tcptls.c
index 749188039..4b0bab30d 100644
--- a/main/tcptls.c
+++ b/main/tcptls.c
@@ -125,7 +125,7 @@ static void session_instance_destructor(void *obj)
 *
 * \note must decrement ref count before returning NULL on error
 */
-static void *handle_tls_connection(void *data)
+static void *handle_tcptls_connection(void *data)
 {
 	struct ast_tcptls_session_instance *tcptls_session = data;
 #ifdef DO_SSL
@@ -197,6 +197,7 @@ static void *handle_tls_connection(void *data)
 						ast_log(LOG_ERROR, "Certificate common name did not match (%s)\n", tcptls_session->parent->hostname);
 						if (peer)
 							X509_free(peer);
+						close(tcptls_session->fd);
 						fclose(tcptls_session->f);
 						ao2_ref(tcptls_session, -1);
 						return NULL;
@@ -266,7 +267,7 @@ void *ast_tcptls_server_root(void *data)
 		tcptls_session->client = 0;
 			
 		/* This thread is now the only place that controls the single ref to tcptls_session */
-		if (ast_pthread_create_detached_background(&launched, NULL, handle_tls_connection, tcptls_session)) {
+		if (ast_pthread_create_detached_background(&launched, NULL, handle_tcptls_connection, tcptls_session)) {
 			ast_log(LOG_WARNING, "Unable to launch helper thread: %s\n", strerror(errno));
 			close(tcptls_session->fd);
 			ao2_ref(tcptls_session, -1);
@@ -330,9 +331,45 @@ int ast_ssl_setup(struct ast_tls_config *cfg)
 	return __ssl_setup(cfg, 0);
 }
 
-struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_session_args *desc)
+struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_session_instance *tcptls_session)
 {
+	struct ast_tcptls_session_args *desc;
 	int flags;
+
+	if (!(desc = tcptls_session->parent)) {
+		goto client_start_error;
+	}
+
+	if (connect(desc->accept_fd, (const struct sockaddr *) &desc->remote_address, sizeof(desc->remote_address))) {
+		ast_log(LOG_ERROR, "Unable to connect %s to %s:%d: %s\n",
+			desc->name,
+			ast_inet_ntoa(desc->remote_address.sin_addr), ntohs(desc->remote_address.sin_port),
+			strerror(errno));
+		goto client_start_error;
+	}
+
+	flags = fcntl(desc->accept_fd, F_GETFL);
+	fcntl(desc->accept_fd, F_SETFL, flags & ~O_NONBLOCK);
+
+	if (desc->tls_cfg) {
+		desc->tls_cfg->enabled = 1;
+		__ssl_setup(desc->tls_cfg, 1);
+	}
+
+	return handle_tcptls_connection(tcptls_session);
+
+client_start_error:
+	close(desc->accept_fd);
+	desc->accept_fd = -1;
+	if (tcptls_session) {
+		ao2_ref(tcptls_session, -1);
+	}
+	return NULL;
+
+}
+
+struct ast_tcptls_session_instance *ast_tcptls_client_create(struct ast_tcptls_session_args *desc)
+{
 	int x = 1;
 	struct ast_tcptls_session_instance *tcptls_session = NULL;
 
@@ -367,39 +404,16 @@ struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_se
 		}
 	}
 
-	if (connect(desc->accept_fd, (const struct sockaddr *) &desc->remote_address, sizeof(desc->remote_address))) {
-		ast_log(LOG_ERROR, "Unable to connect %s to %s:%d: %s\n",
-			desc->name,
-			ast_inet_ntoa(desc->remote_address.sin_addr), ntohs(desc->remote_address.sin_port),
-			strerror(errno));
-		goto error;
-	}
-
 	if (!(tcptls_session = ao2_alloc(sizeof(*tcptls_session), session_instance_destructor)))
 		goto error;
 
 	ast_mutex_init(&tcptls_session->lock);
-
-	flags = fcntl(desc->accept_fd, F_GETFL);
-	fcntl(desc->accept_fd, F_SETFL, flags & ~O_NONBLOCK);
-
+	tcptls_session->client = 1;
 	tcptls_session->fd = desc->accept_fd;
 	tcptls_session->parent = desc;
 	tcptls_session->parent->worker_fn = NULL;
 	memcpy(&tcptls_session->remote_address, &desc->remote_address, sizeof(tcptls_session->remote_address));
 
-	tcptls_session->client = 1;
-
-	if (desc->tls_cfg) {
-		desc->tls_cfg->enabled = 1;
-		__ssl_setup(desc->tls_cfg, 1);
-	}
-
-	/* handle_tls_connection controls the single ref to tcptls_session. If
-	 * tcptls_session returns NULL then the session has been destroyed */
-	if (!(tcptls_session = handle_tls_connection(tcptls_session)))
-		goto error;
-
 	return tcptls_session;
 
 error: