Module: kamailio Branch: master Commit: 39b89a18a8c357151a173ab02dc95dff1f02715d URL: https://github.com/kamailio/kamailio/commit/39b89a18a8c357151a173ab02dc95dff...

Author: Daniel-Constantin Mierla miconda@gmail.com Committer: Daniel-Constantin Mierla miconda@gmail.com Date: 2018-11-30T16:05:30+01:00

tm: reply_received() - simplify locking for processing sip response

- leverage the recursive mutex and skip several zones of unlock/lock, which can lead to races on delayed processing or fast reply retransmissions - related to GH #1613 #1744

---

Modified: src/modules/tm/t_reply.c

---

Diff: https://github.com/kamailio/kamailio/commit/39b89a18a8c357151a173ab02dc95dff... Patch: https://github.com/kamailio/kamailio/commit/39b89a18a8c357151a173ab02dc95dff...

---

diff --git a/src/modules/tm/t_reply.c b/src/modules/tm/t_reply.c index 8a95fe758d..859c73cd37 100644 --- a/src/modules/tm/t_reply.c +++ b/src/modules/tm/t_reply.c @@ -2189,7 +2189,7 @@ int reply_received( struct sip_msg *p_msg ) #ifdef WITH_XAVP sr_xavp_t **backup_xavps; #endif - int replies_locked; + int replies_locked = 0; #ifdef USE_DNS_FAILOVER int branch_ret; int prev_branch; @@ -2223,10 +2223,15 @@ int reply_received( struct sip_msg *p_msg ) /* if transaction found, increment the rpl_received counter */ t_stats_rpl_received();

+ /* lock -- onreply_route, safe avp usage, ... */ + /* - it is a recurrent mutex, so it is safe if a function executed + * down here does another lock/unlock */ + LOCK_REPLIES( t ); + replies_locked=1; + tm_ctx_set_branch_index(branch); init_cancel_info(&cancel_data); msg_status=p_msg->REPLY_STATUS; - replies_locked=0;

uac=&t->uac[branch]; LM_DBG("org. status uas=%d, uac[%d]=%d local=%d is_invite=%d)\n", @@ -2363,11 +2368,6 @@ int reply_received( struct sip_msg *p_msg ) /* processing of on_reply block */ if (onreply_route) { set_route_type(TM_ONREPLY_ROUTE); - - /* lock onreply_route, for safe avp usage */ - LOCK_REPLIES( t ); - replies_locked=1; - /* transfer transaction flag to message context */ if (t->uas.request) { p_msg->flags=t->uas.request->flags; @@ -2444,27 +2444,12 @@ int reply_received( struct sip_msg *p_msg ) if (unlikely((ctx.run_flags&DROP_R_F) && (msg_status<200))) #endif /* TM_ONREPLY_FINAL_DROP_OK */ { - if (likely(replies_locked)) { - replies_locked = 0; - UNLOCK_REPLIES( t ); - } goto done; } #ifdef TM_ONREPLY_FINAL_DROP_OK if (msg_status >= 200) { /* stop final reply timers, now that we executed the onreply route * and the reply was not DROPed */ - if (likely(replies_locked)){ - /* if final reply => we have to execute stop_rb_timers, - * but with replies unlocked to avoid a possible deadlock - * (if the timer is currently running, stop_rb_timers() - * will wait until the timer handler ends, but the - * final_response_handler() will try to lock replies - * => deadlock). - */ - UNLOCK_REPLIES( t ); - replies_locked=0; - } stop_rb_timers(&uac->request); } #endif /* TM_ONREPLY_FINAL_DROP_OK */ @@ -2520,11 +2505,6 @@ int reply_received( struct sip_msg *p_msg ) branch_ret=add_uac_dns_fallback(t, t->uas.request, uac, !replies_locked); prev_branch=-1; - /* unlock replies to avoid sending() while holding a lock */ - if (unlikely(replies_locked)) { - UNLOCK_REPLIES( t ); - replies_locked = 0; - } while((branch_ret>=0) &&(branch_ret!=prev_branch)){ prev_branch=branch_ret; branch_ret=t_send_branch(t, branch_ret, t->uas.request , 0, 1); @@ -2533,12 +2513,8 @@ int reply_received( struct sip_msg *p_msg ) #endif

if (unlikely(p_msg->msg_flags&FL_RPL_SUSPENDED)) { - goto skip_send_reply; - /* suspend the reply (async), no error */ - } - if (unlikely(!replies_locked)){ - LOCK_REPLIES( t ); - replies_locked=1; + /* suspended the reply (async) - no error */ + goto done; } if ( is_local(t) ) { /* local_reply() does UNLOCK_REPLIES( t ) */ @@ -2596,18 +2572,16 @@ int reply_received( struct sip_msg *p_msg ) uac->request.flags|=F_RB_FR_INV; /* mark fr_inv */ } /* provisional replies */

-skip_send_reply: - - if (likely(replies_locked)){ - /* unlock replies if still locked coming via goto skip_send_reply */ +done: + if (unlikely(replies_locked)){ + /* unlock replies if still locked coming via goto */ UNLOCK_REPLIES(t); replies_locked=0; }

-done: tm_ctx_set_branch_index(T_BR_UNDEFINED); - /* we are done with the transaction, so unref it - the reference - * was incremented by t_check() function -bogdan*/ + /* done processing the transaction, so unref it + * - the reference counter was incremented by t_check() function */ t_unref(p_msg); /* don't try to relay statelessly neither on success * (we forwarded statefully) nor on error; on troubles,