[asterisk-commits] jrose: branch 10 r346565 - in /branches/10: ./ channels/ include/asterisk/ main/

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Wed Nov 30 15:46:54 CST 2011


Author: jrose
Date: Wed Nov 30 15:46:47 2011
New Revision: 346565

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=346565
Log:
r346525 | jrose | 2011-11-30 15:10:38 -0600 (Wed, 30 Nov 2011) | 18 lines

Cleaning up chan_sip/tcptls file descriptor closing.

This patch attempts to eliminate various possible instances of undefined behavior caused
by invoking close/fclose in situations where fclose may have already been issued on a
tcptls_session_instance and/or closing file descriptors that don't have a valid index
for fd (-1). Thanks for more than a little help from wdoekes.

(closes issue ASTERISK-18700)
Reported by: Erik Wallin

(issue ASTERISK-18345)
Reported by: Stephane Cazelas

(issue ASTERISK-18342)
Reported by: Stephane Chazelas

Review: https://reviewboard.asterisk.org/r/1576/
........

Merged revisions 346564 from http://svn.asterisk.org/svn/asterisk/branches/1.8

Modified:
    branches/10/   (props changed)
    branches/10/channels/chan_sip.c
    branches/10/include/asterisk/tcptls.h
    branches/10/main/tcptls.c

Propchange: branches/10/
------------------------------------------------------------------------------
Binary property 'branch-1.8-merged' - no diff available.

Modified: branches/10/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/branches/10/channels/chan_sip.c?view=diff&rev=346565&r1=346564&r2=346565
==============================================================================
--- branches/10/channels/chan_sip.c (original)
+++ branches/10/channels/chan_sip.c Wed Nov 30 15:46:47 2011
@@ -2810,14 +2810,7 @@
 
 	if (tcptls_session) {
 		ast_mutex_lock(&tcptls_session->lock);
-		if (tcptls_session->f) {
-			fclose(tcptls_session->f);
-			tcptls_session->f = NULL;
-		}
-		if (tcptls_session->fd != -1) {
-			close(tcptls_session->fd);
-			tcptls_session->fd = -1;
-		}
+		ast_tcptls_close_session_file(tcptls_session);
 		tcptls_session->parent = NULL;
 		ast_mutex_unlock(&tcptls_session->lock);
 
@@ -25970,8 +25963,8 @@
 		ao2_t_ref(ca, -1, "failed to create client, getting rid of client tcptls_session arguments");
 	}
 	if (s->tcptls_session) {
-		close(tcptls_session->fd);
-		s->fd = tcptls_session->fd = -1;
+		ast_tcptls_close_session_file(tcptls_session);
+		s->fd = -1;
 		ao2_ref(s->tcptls_session, -1);
 		s->tcptls_session = NULL;
 	}

Modified: branches/10/include/asterisk/tcptls.h
URL: http://svnview.digium.com/svn/asterisk/branches/10/include/asterisk/tcptls.h?view=diff&rev=346565&r1=346564&r2=346565
==============================================================================
--- branches/10/include/asterisk/tcptls.h (original)
+++ branches/10/include/asterisk/tcptls.h Wed Nov 30 15:46:47 2011
@@ -169,6 +169,13 @@
 void *ast_tcptls_server_root(void *);
 
 /*!
+ * \brief Closes a tcptls session instance's file and/or file descriptor.
+ * The tcptls_session will be set to NULL and it's file descriptor will be set to -1
+ * by this function.
+ */
+void ast_tcptls_close_session_file(struct ast_tcptls_session_instance *tcptls_session);
+
+/*!
  * \brief This is a generic (re)start routine for a TCP server,
  * which does the socket/bind/listen and starts a thread for handling
  * accept().

Modified: branches/10/main/tcptls.c
URL: http://svnview.digium.com/svn/asterisk/branches/10/main/tcptls.c?view=diff&rev=346565&r1=346564&r2=346565
==============================================================================
--- branches/10/main/tcptls.c (original)
+++ branches/10/main/tcptls.c Wed Nov 30 15:46:47 2011
@@ -76,9 +76,23 @@
 
 static int ssl_close(void *cookie)
 {
-	close(SSL_get_fd(cookie));
-	SSL_shutdown(cookie);
-	SSL_free(cookie);
+	int cookie_fd = SSL_get_fd(cookie);
+	int ret;
+	if (cookie_fd > -1) {
+		/*
+		 * According to the TLS standard, it is acceptable for an application to only send its shutdown
+		 * alert and then close the underlying connection without waiting for the peer's response (this
+		 * way resources can be saved, as the process can already terminate or serve another connection).
+		 */
+		if ((ret = SSL_shutdown(cookie)) < 0) {
+			ast_log(LOG_ERROR, "SSL_shutdown() failed: %d\n", SSL_get_error(cookie, ret));
+		}
+		SSL_free(cookie);
+		/* adding shutdown(2) here has no added benefit */
+		if (close(cookie_fd)) {
+			ast_log(LOG_ERROR, "close() failed: %s\n", strerror(errno));
+		}
+	}
 	return 0;
 }
 #endif	/* DO_SSL */
@@ -141,8 +155,7 @@
 	if (!tcptls_session->parent->tls_cfg) {
 		if ((tcptls_session->f = fdopen(tcptls_session->fd, "w+"))) {
 			if(setvbuf(tcptls_session->f, NULL, _IONBF, 0)) {
-				fclose(tcptls_session->f);
-				tcptls_session->f = NULL;
+				ast_tcptls_close_session_file(tcptls_session);
 			}
 		}
 	}
@@ -200,10 +213,10 @@
 					}
 					if (!found) {
 						ast_log(LOG_ERROR, "Certificate common name did not match (%s)\n", tcptls_session->parent->hostname);
-						if (peer)
+						if (peer) {
 							X509_free(peer);
-						close(tcptls_session->fd);
-						fclose(tcptls_session->f);
+						}
+						ast_tcptls_close_session_file(tcptls_session);
 						ao2_ref(tcptls_session, -1);
 						return NULL;
 					}
@@ -218,7 +231,7 @@
 #endif /* DO_SSL */
 
 	if (!tcptls_session->f) {
-		close(tcptls_session->fd);
+		ast_tcptls_close_session_file(tcptls_session);
 		ast_log(LOG_WARNING, "FILE * open failed!\n");
 #ifndef DO_SSL
 		if (tcptls_session->parent->tls_cfg) {
@@ -260,7 +273,9 @@
 		tcptls_session = ao2_alloc(sizeof(*tcptls_session), session_instance_destructor);
 		if (!tcptls_session) {
 			ast_log(LOG_WARNING, "No memory for new session: %s\n", strerror(errno));
-			close(fd);
+			if (close(fd)) {
+				ast_log(LOG_ERROR, "close() failed: %s\n", strerror(errno));
+			}
 			continue;
 		}
 
@@ -277,7 +292,7 @@
 		/* This thread is now the only place that controls the single ref to 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);
+			ast_tcptls_close_session_file(tcptls_session);
 			ao2_ref(tcptls_session, -1);
 		}
 	}
@@ -534,6 +549,24 @@
 error:
 	close(desc->accept_fd);
 	desc->accept_fd = -1;
+}
+
+void ast_tcptls_close_session_file(struct ast_tcptls_session_instance *tcptls_session)
+{
+	if (tcptls_session->f) {
+		if (fclose(tcptls_session->f)) {
+			ast_log(LOG_ERROR, "fclose() failed: %s\n", strerror(errno));
+		}
+		tcptls_session->f = NULL;
+		tcptls_session->fd = -1;
+	} else if (tcptls_session->fd != -1) {
+		if (close(tcptls_session->fd)) {
+			ast_log(LOG_ERROR, "close() failed: %s\n", strerror(errno));
+		}
+		tcptls_session->fd = -1;
+	} else {
+		ast_log(LOG_ERROR, "ast_tcptls_close_session_file invoked on session instance without file or file descriptor\n");
+	}
 }
 
 void ast_tcptls_server_stop(struct ast_tcptls_session_args *desc)




More information about the asterisk-commits mailing list