r346525 | jrose | 2011-11-30 15:10:38 -0600 (Wed, 30 Nov 2011) | 18 lines
authorJonathan Rose <jrose@digium.com>
Wed, 30 Nov 2011 22:03:02 +0000 (22:03 +0000)
committerJonathan Rose <jrose@digium.com>
Wed, 30 Nov 2011 22:03:02 +0000 (22:03 +0000)
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
........

Merged revisions 346565 from http://svn.asterisk.org/svn/asterisk/branches/10

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@346566 65c4cc65-6c06-0410-ace0-fbb531ad65f3

channels/chan_sip.c
include/asterisk/tcptls.h
main/tcptls.c

index 3b3a346..75b207e 100644 (file)
@@ -2813,14 +2813,7 @@ cleanup:
 
        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);
 
@@ -26019,8 +26012,8 @@ create_tcptls_session_fail:
                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;
        }
index 6f187b2..e9b2371 100644 (file)
@@ -177,6 +177,13 @@ struct ast_tcptls_session_instance *ast_tcptls_client_create(struct ast_tcptls_s
 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().
index 5a177a8..ae30e95 100644 (file)
@@ -78,9 +78,23 @@ static HOOK_T ssl_write(void *cookie, const char *buf, LEN_T len)
 
 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 */
@@ -145,8 +159,7 @@ static void *handle_tcptls_connection(void *data)
        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);
                        }
                }
        }
@@ -212,8 +225,7 @@ static void *handle_tcptls_connection(void *data)
                                                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;
                                        }
@@ -226,11 +238,11 @@ static void *handle_tcptls_connection(void *data)
                if (!tcptls_session->f) {       /* no success opening descriptor stacking */
                        SSL_free(tcptls_session->ssl);
                }
-       }
+       }
 #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) {
@@ -276,7 +288,9 @@ void *ast_tcptls_server_root(void *data)
                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;
                }
 
@@ -293,7 +307,7 @@ void *ast_tcptls_server_root(void *data)
                /* 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);
                }
        }
@@ -558,6 +572,24 @@ error:
        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)
 {
        if (desc->master != AST_PTHREADT_NULL) {