[Asterisk-code-review] channels/chan pjsip: fix deadlock in response retransmissions (asterisk[13])

[Torrey Searle]

Torrey Searle has posted comments on this change. ( https://gerrit.asterisk.org/9477 )

Change subject: channels/chan_pjsip: fix deadlock in response retransmissions
......................................................................


Patch Set 8:

> > > The timer must be stopped before the call to pjsip_inv_answer
 > as
 > > > pjsip_inv_answer is what is modifying the tdata (and thus
 > causing
 > > > the re transmission timer to get in an infinite loop)
 > >
 > > To be more precise pjsip_inv_answer calls pjsip_dlg_modify_response
 > > on the inv->last_answer which is the same tdata object as the
 > > tsx->last_tx being used by the re-transmission timer.  so
 > > pjsip_inv_answer can not be called safely while any
 > re-transmission
 > > timer is active
 > 
 > That makes sense.  However, it would make more sense for
 > pjsip_inv_answer() to cancel the timer itself since there are three
 > places in chan_pjsip.c that call pjsip_inv_answer() and the
 > deadlock can happen for any response after we send 100 trying.
 > 
 > In the pjsip/src/pjsip/sip_transaction.c:tsx_on_state_proceeding_uas()
 > function it calls:
 > 
 > /* Stop 1xx retransmission timer, if any */
 > tsx_cancel_timer(tsx, &tsx->retransmit_timer);
 > 
 > Where tsx can be obtained from pjsip_inv_answer()'s
 > inv->invite_tsx.
 > 
 > Which is effectively the same thing as pjsip_tsx_stop_retransmit().
 > 
 > 
 > Unfortunately, I don't think this will completely eliminate the
 > deadlock because the timer callback could already be running in the
 > monitor thread when you try to cancel the timer.

Actually it should be safe as the both the timer and the cancel timer both take the group lock on tsx.


-- 
To view, visit https://gerrit.asterisk.org/9477
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic381004a3a212fe1d8eca0e707fe09dba4a6ab4e
Gerrit-Change-Number: 9477
Gerrit-PatchSet: 8
Gerrit-Owner: Torrey Searle <tsearle at gmail.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Torrey Searle <tsearle at gmail.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 15:57:48 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180725/fde7fb59/attachment.html>