Hi Ross/Alexei,
We have made some modification to the patch in order to incorporate the group lock.
Please find the patch on the attachment, could you help us in testing it?
Best Regards,
Riza
On Fri, Oct 7, 2016 at 4:47 PM, Ross Beer <ross.beer@xxxxxxxxxxx> wrote:
Hi Ming,
Sorry to chase, however is this still undergoing review?
Kind regards,
Ross
From: pjsip <pjsip-bounces@xxxxxxxxxxxxxxx> on behalf of Ming <ming@xxxxxxxxx>
Sent: 03 October 2016 12:34
To: pjsip list
Subject: Re: patch: crash on using already destroyed ssl socketHi Ross,
Yes, the patch is undergoing some changes and currently under
(hopefully) final review.
Regards,
Ming
On Mon, Oct 3, 2016 at 5:23 PM, Ross Beer <ross.beer@xxxxxxxxxxx> wrote:
> Hi PJSIP team,
>
>
> Could this patch be considered for inclusion in the project?
>
>
> This issue is causing segfaults a least once a day.
>
>
> Kind regards,
>
>
> Ross
>
>
>
> ________________________________
> From: pjsip <pjsip-bounces@xxxxxxxxxxxxxxx> on behalf of Alexei Gradinari
> <alex2grad@xxxxxxxxx>
> Sent: 21 September 2016 16:38
> To: pjsip list
> Subject: patch: crash on using already destroyed ssl socket
>
> Hello,
>
> On heavy loaded system with TLS,
> one thread could destroy the ssl socket on SSL_ERROR_SYSCALL
> while another thread still uses this socket which
> was already freed, so we get segfault.
> Attached 2 backtraces.
>
> To avoid race condition need to lock the socket before destroying it.
>
> The attached patch adds the socket lock on destroying it
> and adds a checking on all openssl calls that the socket wasn't destroyed.
>
> Regards,
> Alexei
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists. pjsip.org
>
_______________________________________________
Visit our blog: http://blog.pjsip.org
pjsip mailing list
pjsip@xxxxxxxxxxxxxxx
http://lists.pjsip.org/mailman/listinfo/pjsip_lists. pjsip.org
_______________________________________________
Visit our blog: http://blog.pjsip.org
pjsip mailing list
pjsip@xxxxxxxxxxxxxxx
http://lists.pjsip.org/mailman/listinfo/pjsip_lists. pjsip.org
Index: pjlib/src/pj/ssl_sock_ossl.c
===================================================================
--- pjlib/src/pj/ssl_sock_ossl.c (revision 5446)
+++ pjlib/src/pj/ssl_sock_ossl.c (working copy)
@@ -822,7 +822,10 @@
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,10 +844,10 @@
/* Reset SSL socket state */
static void reset_ssl_sock_state(pj_ssl_sock_t *ssock)
{
- ssock->ssl_state = SSL_STATE_NULL;
+ pj_lock_acquire(ssock->write_mutex);
+ ssock->ssl_state = SSL_STATE_NULL;
+ pj_lock_release(ssock->write_mutex);
- destroy_ssl(ssock);
-
close_sockets(ssock);
/* Upon error, OpenSSL may leave any error description in the thread
@@ -1612,7 +1615,22 @@
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);
+}
+
+
/*
*******************************************************************
* Active socket callbacks.
@@ -1830,7 +1848,7 @@
/* 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;
@@ -1895,7 +1913,7 @@
asock_cfg.async_cnt = ssock->param.async_cnt;
asock_cfg.concurrency = ssock->param.concurrency;
asock_cfg.whole_data = PJ_TRUE;
-
+
/* If listener socket has group lock, automatically create group lock
* for the new socket.
*/
@@ -1906,12 +1924,10 @@
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 @@
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;
@@ -2225,8 +2236,8 @@
* Create SSL socket instance.
*/
PJ_DEF(pj_status_t) pj_ssl_sock_create (pj_pool_t *pool,
- const pj_ssl_sock_param *param,
- pj_ssl_sock_t **p_ssock)
+ const pj_ssl_sock_param *param,
+ pj_ssl_sock_t **p_ssock)
{
pj_ssl_sock_t *ssock;
pj_status_t status;
@@ -2251,17 +2262,26 @@
/* 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_close(pj_ssl_sock_t *ssock)
{
- pj_pool_t *pool;
-
PJ_ASSERT_RETURN(ssock, PJ_EINVAL);
if (!ssock->pool)
@@ -2290,12 +2308,11 @@
}
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 @@
/* 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;
_______________________________________________ Visit our blog: http://blog.pjsip.org pjsip mailing list pjsip@xxxxxxxxxxxxxxx http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
