SEGV in pjsip due to race condition (+ patch)

[Martin Oberhuber <mober.at@xxxxxxxxx>]

Dear PJSIP project,

I've come across a SEGV in pjproject-2.6 on Linux under the following circumstances:

• An active account re-registers
• The re-registration fails (in my case, DNS resolution timeout after 10 seconds)
• At exactly the same time, the keep-alive timer fires.

The issue was consistently reproducible for me under a very high network load, with the re-registration interval set to 5 seconds, DNS timeout default (10 seconds) and keepalive interval default (15 seconds). Since 5+10 seconds == 15 seconds, the 2 events coincide and lead to the following backtrace:

Thread #1 (Suspended : Signal : SIGSEGV:Segmentation fault)

keep_alive_timer_cb() at pjsua_acc.c:1,981 0x76bcb424

pj_timer_heap_poll() at timer.c:643 0x76e43244

pjsip_endpt_handle_events2() at sip_endpoint.c:713 0x76cc0dd8

worker_thread() at pjsua_core.c:695 0x76bde404

Thread #2
__pthread_mutex_unlock_usercnt() at pthread_mutex_unlock.c:66 0x4940a4d8

__GI___pthread_mutex_unlock() at pthread_mutex_unlock.c:315 0x4940a588

pj_mutex_unlock() at os_core_unix.c:1,323 0x76e4241c

PJSUA_UNLOCK() at pjsua_internal.h:584 0x76be4244

pjsua_acc_set_registration() at pjsua_acc.c:2,682 0x76be4244

pj::Account::getInfo() at account.cpp:737 0x76e86438

Analyzing the backtrace, I found 2 problems which are fixed in attached patch:

1. timer.c: pj_timer_heap_poll places the timer onto the freelist and releases the global lock before calling the callback -- thus the callback may operate on a timer already freed! Proposed fix: keep timer_entry out of the freelist until the callback is done.
2. pjsua_acc.c: Even with the 1st issue fixed, the account registration could still be canceled "exactly when the callback fires", because the lock is released before the callback ... thus putting NULL into the ka_transport thus causing the SEGV. Proposed fix: protect against NULL in ka_transport.

While the patch is against pjroject-2.6 , I believe that the issue is still in latest trunk as well.
Please let me know what you think about the attached patch, and consider it for inclusion in pjproject.

Thanks,
Martin

--

Martin Oberhuber | Software Architect, Project Lead & Consultant | Austria

--- pjproject-2.6.orig/pjlib/src/pj/timer.c	2014-06-04 11:23:10.000000000 +0200
+++ pjproject-2.6/pjlib/src/pj/timer.c	2017-12-31 00:33:06.839693219 +0100
@@ -628,6 +628,8 @@ PJ_DEF(unsigned) pj_timer_heap_poll( pj_
             count < ht->max_entries_per_poll ) 
     {
 	pj_timer_entry *node = remove_node(ht, 0);
+	//Avoid re-use of this timer until the callback is done
+	pj_timer_id_t node_timer_id = pop_freelist(ht);
 	pj_grp_lock_t *grp_lock;
 
 	++count;
@@ -646,6 +648,8 @@ PJ_DEF(unsigned) pj_timer_heap_poll( pj_
 	    pj_grp_lock_dec_ref(grp_lock);
 
 	lock_timer_heap(ht);
+	//Now, the timer is really free for re-use
+	push_freelist(ht, node_timer_id);
     }
     if (ht->cur_size && next_delay) {
 	*next_delay = ht->heap[0]->_timer_value;
--- pjproject-2.6.orig/pjsip/src/pjsua-lib/pjsua_acc.c	2017-01-19 11:31:38.000000000 +0100
+++ pjproject-2.6/pjsip/src/pjsua-lib/pjsua_acc.c	2018-01-01 23:39:33.095689862 +0100
@@ -1967,6 +1967,12 @@ static void keep_alive_timer_cb(pj_timer
 
     acc = (pjsua_acc*) te->user_data;
 
+    /* Check if the account is still active. It might have just been deleted
+     * while the keep-alive timer was about to be called (race condition).
+     */
+    if (acc->ka_transport == 0)
+	goto on_return;
+
     /* Select the transport to send the packet */
     pj_bzero(&tp_sel, sizeof(tp_sel));
     tp_sel.type = PJSIP_TPSELECTOR_TRANSPORT;

_______________________________________________
Visit our blog: http://blog.pjsip.org

pjsip mailing list
pjsip@xxxxxxxxxxxxxxx
http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org