[Asterisk-code-review] pjproject bundled: Add patch to address SSL crash (asterisk[13])

Joshua Colp asteriskteam at digium.com
Mon Oct 17 13:47:56 CDT 2016


Joshua Colp has submitted this change and it was merged.

Change subject: pjproject_bundled:  Add patch to address SSL crash
......................................................................


pjproject_bundled:  Add patch to address SSL crash

Addresses crashes when an attempt is made to operate on an SSL socket
after the socket has been closed.

ASTERISK-26477 #close

Change-Id: I421305b357558b4f9e690210dc0f4831ef4b3002
---
A third-party/pjproject/patches/0005-Re-1969-Fix-crash-on-using-an-already-destroyed-SSL-.patch
1 file changed, 164 insertions(+), 0 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/third-party/pjproject/patches/0005-Re-1969-Fix-crash-on-using-an-already-destroyed-SSL-.patch b/third-party/pjproject/patches/0005-Re-1969-Fix-crash-on-using-an-already-destroyed-SSL-.patch
new file mode 100644
index 0000000..551e61a
--- /dev/null
+++ b/third-party/pjproject/patches/0005-Re-1969-Fix-crash-on-using-an-already-destroyed-SSL-.patch
@@ -0,0 +1,164 @@
+From 9e67e0d5c3fdc747530a956038b374fca4748b76 Mon Sep 17 00:00:00 2001
+From: riza <riza at localhost>
+Date: Thu, 13 Oct 2016 09:02:50 +0000
+Subject: [PATCH 1/4] Re #1969: Fix crash on using an already destroyed SSL
+ socket.
+
+---
+ pjlib/src/pj/ssl_sock_ossl.c | 66 ++++++++++++++++++++++++++++----------------
+ 1 file changed, 42 insertions(+), 24 deletions(-)
+
+diff --git a/pjlib/src/pj/ssl_sock_ossl.c b/pjlib/src/pj/ssl_sock_ossl.c
+index fa0db2d..ceab67a 100644
+--- a/pjlib/src/pj/ssl_sock_ossl.c
++++ b/pjlib/src/pj/ssl_sock_ossl.c
+@@ -822,7 +822,10 @@ static void close_sockets(pj_ssl_sock_t *ssock)
+     pj_lock_acquire(ssock->write_mutex);
+     asock = ssock->asock;
+     if (asock) {
+-        ssock->asock = NULL;
++        // Don't set ssock->asock to NULL, as it may trigger assertion in
++        // send operation. This should be safe as active socket will simply
++        // return PJ_EINVALIDOP on any operation if it is already closed.
++        //ssock->asock = NULL;
+         ssock->sock = PJ_INVALID_SOCKET;
+     }
+     sock = ssock->sock;
+@@ -841,9 +844,9 @@ static void close_sockets(pj_ssl_sock_t *ssock)
+ /* Reset SSL socket state */
+ static void reset_ssl_sock_state(pj_ssl_sock_t *ssock)
+ {
++    pj_lock_acquire(ssock->write_mutex);
+     ssock->ssl_state = SSL_STATE_NULL;
+-
+-    destroy_ssl(ssock);
++    pj_lock_release(ssock->write_mutex);
+ 
+     close_sockets(ssock);
+ 
+@@ -1612,6 +1615,21 @@ static pj_status_t do_handshake(pj_ssl_sock_t *ssock)
+     return PJ_EPENDING;
+ }
+ 
++static void ssl_on_destroy(void *arg)
++{
++    pj_pool_t *pool = NULL;
++    pj_ssl_sock_t *ssock = (pj_ssl_sock_t*)arg;
++
++    destroy_ssl(ssock);
++
++    pj_lock_destroy(ssock->write_mutex);
++
++    pool = ssock->pool;
++    ssock->pool = NULL;
++    if (pool)
++	pj_pool_release(pool);
++}
++
+ 
+ /*
+  *******************************************************************
+@@ -1830,7 +1848,7 @@ static pj_bool_t asock_on_accept_complete (pj_activesock_t *asock,
+ 
+     /* Create new SSL socket instance */
+     status = pj_ssl_sock_create(ssock_parent->pool,
+-    				&ssock_parent->newsock_param, &ssock);
++				&ssock_parent->newsock_param, &ssock);
+     if (status != PJ_SUCCESS)
+ 	goto on_return;
+ 
+@@ -1906,12 +1924,10 @@ static pj_bool_t asock_on_accept_complete (pj_activesock_t *asock,
+ 	if (status != PJ_SUCCESS)
+ 	    goto on_return;
+ 
+-	/* Temporarily add ref the group lock until active socket creation,
+-	 * to make sure that group lock is destroyed if the active socket
+-	 * creation fails.
+-	 */
+ 	pj_grp_lock_add_ref(glock);
+ 	asock_cfg.grp_lock = ssock->param.grp_lock = glock;
++	pj_grp_lock_add_handler(ssock->param.grp_lock, ssock->pool, ssock,
++				ssl_on_destroy);
+     }
+ 
+     pj_bzero(&asock_cb, sizeof(asock_cb));
+@@ -1927,11 +1943,6 @@ static pj_bool_t asock_on_accept_complete (pj_activesock_t *asock,
+ 				  ssock,
+ 				  &ssock->asock);
+ 
+-    /* This will destroy the group lock if active socket creation fails */
+-    if (asock_cfg.grp_lock) {
+-	pj_grp_lock_dec_ref(asock_cfg.grp_lock);
+-    }
+-
+     if (status != PJ_SUCCESS)
+ 	goto on_return;
+ 
+@@ -2251,17 +2262,26 @@ PJ_DEF(pj_status_t) pj_ssl_sock_create (pj_pool_t *pool,
+     /* Create secure socket mutex */
+     status = pj_lock_create_recursive_mutex(pool, pool->obj_name,
+ 					    &ssock->write_mutex);
+-    if (status != PJ_SUCCESS)
++    if (status != PJ_SUCCESS) {
++	pj_pool_release(pool);
+ 	return status;
++    }
+ 
+     /* Init secure socket param */
+     pj_ssl_sock_param_copy(pool, &ssock->param, param);
++
++    if (ssock->param.grp_lock) {
++	pj_grp_lock_add_ref(ssock->param.grp_lock);
++	pj_grp_lock_add_handler(ssock->param.grp_lock, pool, ssock,
++				ssl_on_destroy);
++    }
++
+     ssock->param.read_buffer_size = ((ssock->param.read_buffer_size+7)>>3)<<3;
+     if (!ssock->param.timer_heap) {
+ 	PJ_LOG(3,(ssock->pool->obj_name, "Warning: timer heap is not "
+ 		  "available. It is recommended to supply one to avoid "
+-		  "a race condition if more than one worker threads "
+-		  "are used."));
++	          "a race condition if more than one worker threads "
++	          "are used."));
+     }
+ 
+     /* Finally */
+@@ -2277,8 +2297,6 @@ PJ_DEF(pj_status_t) pj_ssl_sock_create (pj_pool_t *pool,
+  */
+ PJ_DEF(pj_status_t) pj_ssl_sock_close(pj_ssl_sock_t *ssock)
+ {
+-    pj_pool_t *pool;
+-
+     PJ_ASSERT_RETURN(ssock, PJ_EINVAL);
+ 
+     if (!ssock->pool)
+@@ -2290,12 +2308,11 @@ PJ_DEF(pj_status_t) pj_ssl_sock_close(pj_ssl_sock_t *ssock)
+     }
+ 
+     reset_ssl_sock_state(ssock);
+-    pj_lock_destroy(ssock->write_mutex);
+-    
+-    pool = ssock->pool;
+-    ssock->pool = NULL;
+-    if (pool)
+-	pj_pool_release(pool);
++    if (ssock->param.grp_lock) {
++	pj_grp_lock_dec_ref(ssock->param.grp_lock);
++    } else {
++	ssl_on_destroy(ssock);
++    }
+ 
+     return PJ_SUCCESS;
+ }
+@@ -2782,6 +2799,7 @@ pj_ssl_sock_start_accept2(pj_ssl_sock_t *ssock,
+ 
+     /* Start accepting */
+     pj_ssl_sock_param_copy(pool, &ssock->newsock_param, newsock_param);
++    ssock->newsock_param.grp_lock = NULL;
+     status = pj_activesock_start_accept(ssock->asock, pool);
+     if (status != PJ_SUCCESS)
+ 	goto on_error;
+-- 
+2.7.4
+

-- 
To view, visit https://gerrit.asterisk.org/4134
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I421305b357558b4f9e690210dc0f4831ef4b3002
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>



More information about the asterisk-code-review mailing list