<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#file25890line19785" 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 check_pendings(struct sip_pvt *p)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">19785</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_clear_flag</span><span class="p">(</span><span class="o">&</span><span class="n">p</span><span class="o">-></span><span class="n">flags</span><span class="p">[</span><span class="mi">0</span><span class="p">],</span> <span class="n">SIP_PENDINGBYE</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">19771</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">19786</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">19772</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_clear_flag</span><span class="p">(</span><span class="o">&</span><span class="n">p</span><span class="o">-></span><span class="n">flags</span><span class="p">[</span><span class="mi">0</span><span class="p">],</span> <span class="n">SIP_PENDINGBYE</span><span class="p">);</span><span class="ew"> </span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">This change will result in some silliness from Asterisk in a specific CANCEL case. Let's walk through what happens with this change, along with the change in handle_response_invite() to call check_pendings() to the 487 case.
1. Asterisk sends an INVITE out.
2. Caller decides to hang up before the call is answered. No provisional response has been received by Asterisk on the outbound call.
3. sip_hangup() is called on the outbound call. We can't send a CANCEL yet since we have not received a provisional response so we set SIP_PENDINGBYE.
4. Asterisk gets a provisional response from the called party. check_pendings() is called.
5. SIP_PENDINGBYE tests positive. Because of the current invitestate, we send a CANCEL. Now, because we hit this case, we will NOT clear the SIP_PENDINGBYE flag.
6. Asterisk receives a 200 OK for the CANCEL
7. Asterisk receives a 487 for the INVITE. pending_invite is cleared at the beginning of handle_response_invite(). Since you have added check_pendings() to the 487 case, check_pendings() is called.
8. SIP_PENDINGBYE tests positive. Because of the current invitestate, and because there is no pending_invite, Asterisk sends a BYE.
9. Asterisk will likely get a 481 back from whoever we send that BYE to.
So what this boils down to is that in this specific case, Asterisk will send an extra unnecessary BYE.
The root of the issue is that Asterisk thinks of CANCEL and BYE as accomplishing the same goal, just during different stages of a call. This is why sending a BYE and sending a CANCEL are both handled using the same PENDINGBYE flag. The problem, as you have seen, is that a CANCEL can also be used on a reinvite. In that scenario, both a CANCEL and BYE are required.
I think that the proper change here is to move the flag clearing as you have done, but you also need to potentially clear the flag in the CANCEL case as well. Specifically, you'll need to clear the flag in the CANCEL case if it is not a reinvite that is being canceled.</pre>
</blockquote>
<p>On March 8th, 2012, 11:58 a.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;">Changed. Just to clarify, I do think we still need to call check_pendings() in the 487 case (couldn't tell if you were advocating removing the check_pending from there or not). If its an initial invite, then the flag will now be removed, which doesn't cause any harm. However, in a re-INVITE, the 487 I believe would be the only chance we'd have to trigger the necessary BYE.
1. Asterisk sends INVITE, and afterwards sip_hangup is called setting the SIP_PENDINGBYE flag
2. UA sends back a provisional response. This calls check_pendings, which triggers the CANCEL and, since its a re-INVITE, does not remove the SIP_PENDINGBYE flag. Asterisk sends CANCEL request.
3. UA sends back 200 Cancelling to the CANCEL request. This would also call check_pendings, but since pendinginvite hasn't been cleared yet, we return
4. UA sends 487 back for the INVITE. That call check_pendings, pendinginvite is now 0, and we send the BYE.</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;">Sorry to be unclear. Definitely keep the check_pendings() in the 487 case.</pre>
<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>
</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;">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>
<br />
<p>- Mark</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>