From 9ef171ffe0b39ab6fbf1325c4b7d5a3e5441db75 Mon Sep 17 00:00:00 2001 From: Jonathan Rose Date: Wed, 30 Nov 2011 22:03:02 +0000 Subject: [PATCH] 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 ........ 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 | 13 +++------- include/asterisk/tcptls.h | 7 +++++ main/tcptls.c | 54 +++++++++++++++++++++++++++++++-------- 3 files changed, 53 insertions(+), 21 deletions(-) diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 3b3a3460a9..75b207e1b9 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -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; } diff --git a/include/asterisk/tcptls.h b/include/asterisk/tcptls.h index 6f187b20be..e9b2371fbf 100644 --- a/include/asterisk/tcptls.h +++ b/include/asterisk/tcptls.h @@ -176,6 +176,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 diff --git a/main/tcptls.c b/main/tcptls.c index 5a177a884a..ae30e95e12 100644 --- a/main/tcptls.c +++ b/main/tcptls.c @@ -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) {