<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1807/">https://reviewboard.asterisk.org/r/1807/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 8th, 2012, 11:29 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1807/diff/1/?file=25890#file25890line19949" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/channels/chan_sip.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static void handle_response_publish(struct sip_pvt *p, int resp, const char *rest, struct sip_request *req, uint32_t seqno)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">19934</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">int</span> <span class="n">reinvite</span> <span class="o">=</span> <span class="p">(</span><span class="n">p</span><span class="o">-></span><span class="n">owner</span> <span class="o">&&</span> <span class="n">p</span><span class="o">-></span><span class="n">owner</span><span class="o">-></span><span class="n">_state</span> <span class="o">==</span> <span class="n">AST_STATE_UP</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">19948</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">enum</span> <span class="n">invite_types</span> <span class="n">invite_type</span> <span class="o">=</span> <span class="n">p</span><span class="o">-></span><span class="n">invite_type</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If you take my advice about removing the invite_type altogether, the way you should be setting the "reinvite" variable here is to test the SIP_PAGE2_DIALOG_ESTABLISHED flag. This should be done the same way in handle_request_invite() as well.</pre>
</blockquote>
<p>On March 8th, 2012, 12:02 p.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Done for handle_response_invite. I don't think anything is needed anymore in handle_request_invite, since SIP_PAGE2_DIALOG_ESTABLISHED will already be set at the appropriate time.</pre>
</blockquote>
<p>On March 8th, 2012, 12:15 p.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What I meant was that the "reinvite" variable in handle_request_invite() should be changed to not be dependent on the existence of p->owner. Unfortunately, it's a bit more screwy in that function so it may be worth saving for a different review/revision.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Agreed. Minimizing changes to handle_request_invite is good.</pre>
<br />
<p>- Matt</p>
<br />
<p>On March 8th, 2012, 12:13 p.m., Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Joshua Colp and Mark Michelson.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated March 8, 2012, 12:13 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Scenario:
Two UAs are connected to Asterisk, where UA1 and UA2 are remotely bridged together (using some directmedia setting). When UA1 hangs up, a BYE is sent to Asterisk and, assuming nominal conditions, Asterisk responds with a 200 OK. Immediately following the tear down of the remote bridge, but before the channel hangup is processed, a re-INVITE is sent to UA2 informing it of the change in media destination. All of this behavior is expected, as (potentially) UA2 isn't going anywhere, and now needs to send its media to Asterisk as opposed to UA1. As a result, the pendinginvite flag is set on the dialog between Asterisk and UA2.
The problem arises when the processing of the re-INVITE takes a significant amount of time - for example, the final response takes awhile (lost in transmission), or some authentication has to occur between Asterisk and UA2 before the INVITE request is accepted. In that case, sip_hangup has potentially been called on UA2, and, since an INVITE is pending, has set the PENDINGBYE flag; this defers sending the BYE request until the INVITE request is finished.
There are two problems that occur in this scenario:
1) The channel object may no longer exist. This skews the handling of the re-INVITE, as (with no p->owner and no channel state), things start to look more like an initial INVITE then a re-INVITE.
2) When we perform a process_pending on receiving a provisional response for the re-INVITE, we note that we need to cancel this request, as we are already hung up. Unfortunately, when we do that we also clear the pending BYE - so that when the final response for the INVITE is returned, we no longer think we have a pending BYE.
One scenario where the problem manifests itself is shown below (ASCII art!) - but since this is timing based, you could come up with others:
UA1 Asterisk UA2
BYE
1 ---------------->
200 OK
2 <---------------
INVITE
3 ----------------->
401
4 <-----------------
ACK
5 ----------------->
INVITE
6 ----------------->
100
7 <-----------------
CANCEL
8 ----------------->
200
9 <-----------------
487
10 <-----------------
In the sequence above, the sip_hangup would be called sometime after step 3. The pending BYE flag would be set on the dialog between Asterisk and UA2, and would be cleared accidentally when the CANCEL is sent in step 8. Asterisk should, after receiving a 487 terminating the re-INVITE, send a BYE to UA2.
This patch primarily does three things to address this:
1. It adds a call to process_pending when handling 487 responses for INVITE requests
2. It prevents clearing the BYE flag in process_pending unless a BYE has actually been sent
3. It adds a new enum to chan_sip called invite_types, where the types are INV_TYPE_NONE, INV_TYPE_INITIAL, and INV_TYPE_REINVITE. Although a new flag is hardly desirable in chan_sip, it is probably necessary as we previously looked at whether or not a dialog had a channel associated with it to determine if the INVITE being processed is a re-INVITE or an initial INVITE. This makes sense when first processing an INVITE request, however, since the channel can go away before the final response for an INVITE is received, it does not make sense when handling responses.
I've tried to limit how many places the flag needs to be inserted to minimize the impact. I purposefully did not change the behavior of existing or similar flags, such as pendinginvite.
At this point in time, this patch needs some review to see if this solution is on the right track, or if there is a better way to handle this situation that is not overly invasive in chan_sip.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ran through the Asterisk Test Suite, which does do some re-INVITE processing. Tested a nominal remote bridge scenario using two Polycom phones and had no issues (although getting them to exhibit the above problem is also difficult, since the sip_hangup happens immediately after the initial INVITE is sent out).
More testing will need to be done by the issue reporter and, hopefully, some more elaborate system level tests.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19365">ASTERISK-19365</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(358643)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1807/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>