From 7642e741b9c6d283f4a5d235cd6980e5f57f7cba Mon Sep 17 00:00:00 2001 From: Kevin Easton Date: Sun, 3 Dec 2017 02:07:19 +1100 Subject: [PATCH] Switch to using NULL server.ssl_fd to mark new SSL connection instead of NULL server.ctx server.ctx is now allocated once for each server on first connection, then reused for subsequent connections. server.ctx wasn't being cleared when the server was closed (as server.ssl_fd is), so this was causing reconnection to fail until try_connect() got called. This also removes the last uses of the CHK_* macros in ssl.h, so remove them. --- Changelog | 2 ++ include/ssl.h | 4 --- source/server.c | 78 +++++++++++++++++++++++++++++++++---------------- 3 files changed, 55 insertions(+), 29 deletions(-) diff --git a/Changelog b/Changelog index a71919d..ddb6de7 100644 --- a/Changelog +++ b/Changelog @@ -1,5 +1,7 @@ [Changes 1.2.2] +* Fix problem where reconnecting to SSL servers could stall. (caf) + * Fix memory leak when reconnecting to SSL servers. (caf) * Add SSL version and cipher to SSL connect message. (caf) diff --git a/include/ssl.h b/include/ssl.h index caf5db1..2d9fe70 100644 --- a/include/ssl.h +++ b/include/ssl.h @@ -17,10 +17,6 @@ #define FALSE 1 #endif -#define CHK_NULL(x) if ((x)==NULL) { say("SSL error - NULL data form server"); close_server(refnum, empty_string); return(-1); } -#define CHK_ERR(err,s) if ((err)==-1) { say("SSL prime error - %s",s); close_server(refnum, empty_string); return(-1); } -#define CHK_SSL(err) if ((err)==-1) { say("SSL CHK error - %d %d",err,SSL_get_error(server_list[refnum].ssl_fd, err)); close_server(refnum, empty_string); return(-2); } - void SSL_show_errors(void); /* Make these what you want for cert & key files */ diff --git a/source/server.c b/source/server.c index ba826ab..33ae9c1 100644 --- a/source/server.c +++ b/source/server.c @@ -146,8 +146,7 @@ void BX_close_server (int cs_index, char *message) { server_list[cs_index].closing = 1; if (x_debug & DEBUG_OUTBOUND) - yell("Closing server %d because [%s]", - cs_index, message ? message : empty_string); + yell("Closing server %d because [%s]", cs_index, message); snprintf(buffer, MAX_PROTOCOL_SIZE + 1, "QUIT :%s", message); strlcat(buffer, "\r\n", sizeof buffer); #ifdef HAVE_LIBSSL @@ -158,7 +157,7 @@ void BX_close_server (int cs_index, char *message) write(server_list[cs_index].write, buffer, strlen(buffer)); } #ifdef HAVE_LIBSSL - if (get_server_ssl(cs_index)) + if (server_list[cs_index].ssl_fd) { SSL_shutdown(server_list[cs_index].ssl_fd); SSL_free(server_list[cs_index].ssl_fd); @@ -503,7 +502,7 @@ void do_server (fd_set *rd, fd_set *wr) if (getpeername(des, (struct sockaddr *) &sa, &salen) != -1) { #ifdef HAVE_LIBSSL - if(!server_list[i].ctx || server_list[i].ssl_error == SSL_ERROR_WANT_WRITE) + if (!server_list[i].ssl_fd || server_list[i].ssl_error == SSL_ERROR_WANT_WRITE) { #endif server_list[i].connect_wait = 0; @@ -530,9 +529,9 @@ void do_server (fd_set *rd, fd_set *wr) { #ifdef NON_BLOCKING_CONNECTS /* If we get here before getting above we have problems. */ - if(!(server_list[i].login_flags & SF_LOGGED_IN)) + if (!(server_list[i].login_flags & SF_LOGGED_IN)) { - if(!server_list[i].ctx || server_list[i].ssl_error == SSL_ERROR_WANT_READ) + if (!server_list[i].ssl_fd || server_list[i].ssl_error == SSL_ERROR_WANT_READ) { server_list[i].connect_wait = 0; finalize_server_connect(i, server_list[i].c_server, i); @@ -868,7 +867,8 @@ void remove_from_server_list (int i) new_free(&server_list[i].sent_nick); new_free(&server_list[i].sent_body); #ifdef HAVE_LIBSSL - SSL_CTX_free(server_list[i].ctx); + if (server_list[i].ctx) + SSL_CTX_free(server_list[i].ctx); #endif clear_server_sping(i, NULL); @@ -1341,27 +1341,60 @@ int finalize_server_connect(int refnum, int c_server, int my_from_server) } #ifdef HAVE_LIBSSL - if(get_server_ssl(refnum)) + if (get_server_ssl(refnum)) { int err = 0; - if(!server_list[refnum].ctx) + if (!server_list[refnum].ssl_fd) { - server_list[refnum].ctx = SSL_CTX_new (SSLv23_client_method()); - CHK_NULL(server_list[refnum].ctx); - server_list[refnum].ssl_fd = SSL_new (server_list[refnum].ctx); - CHK_NULL(server_list[refnum].ssl_fd); + /* Lazily allocate an SSL_CTX the first time this server connects. This + * is reused for subsequent connections to this server. + */ + if (!server_list[refnum].ctx) + { + server_list[refnum].ctx = SSL_CTX_new(SSLv23_client_method()); + + if (!server_list[refnum].ctx) + { + say("SSL error - failed to allocate SSL_CTX"); + SSL_show_errors(); + close_server(refnum, NULL); + return -1; + } + } + + /* Allocate an SSL for this connection. This will be freed at close time. */ + server_list[refnum].ssl_fd = SSL_new(server_list[refnum].ctx); + if (!server_list[refnum].ssl_fd) + { + say("SSL error - failed to create SSL"); + SSL_show_errors(); + close_server(refnum, NULL); + return -1; + } + SSL_set_fd (server_list[refnum].ssl_fd, server_list[refnum].read); } - err = SSL_connect (server_list[refnum].ssl_fd); - if(err == -1) + + err = SSL_connect(server_list[refnum].ssl_fd); + + if (err < 1) { server_list[refnum].ssl_error = SSL_get_error(server_list[refnum].ssl_fd, err); - if(server_list[refnum].ssl_error == SSL_ERROR_WANT_READ || server_list[refnum].ssl_error == SSL_ERROR_WANT_WRITE) + + /* The SSL_connect can't complete yet. Return without calling register_server(), + * and this function will be called again later. + */ + if (server_list[refnum].ssl_error == SSL_ERROR_WANT_READ || + server_list[refnum].ssl_error == SSL_ERROR_WANT_WRITE) return 0; + + say("SSL_connect error: %d", err, server_list[refnum].ssl_error); + SSL_show_errors(); + close_server(refnum, NULL); + return -2; } - SSL_show_errors(); - CHK_SSL(err); + say("SSL server connected using %s (%s)", SSL_get_version(server_list[refnum].ssl_fd), SSL_get_cipher(server_list[refnum].ssl_fd)); @@ -1552,15 +1585,10 @@ void try_connect (int server, int old_server) { if (server_list) { - if (server >= number_of_servers) - server = 0; - else if (server < 0) + if (server < 0 || server >= number_of_servers) server = 0; -#ifdef HAVE_LIBSSL - server_list[server].ctx = NULL; -#endif - if(server_list[server].server_change_refnum > -1) + if (server_list[server].server_change_refnum > -1) set_display_target_by_winref(server_list[server].server_change_refnum); set_server_old_server(server, old_server);