<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 />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think the addition of the invite_type to a sip_pvt is unnecessary and will only cause confusion since it is so minimally applied here (e.g. only to outbound INVITEs). The type of INVITE can be determined at any given point by checking the SIP_PAGE2_DIALOG_ESTABLISHED flag. This flag gets set at the end of processing a 200 OK response for an INVITE (along with other request types such as SUBSCRIBE and PUBLISH). If the flag is set, then that means that we are dealing with a reINVITE. If the flag is not set, then that means we are dealing with an initial INVITE.</pre>
<br />
<div>
<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="margin-left: 2em; 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>
</div>
<br />
<div>
<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="margin-left: 2em; 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>
</div>
<br />
<p>- Mark</p>
<br />
<p>On March 7th, 2012, 5:50 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 7, 2012, 5:50 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/sip/include/sip.h <span style="color: grey">(357718)</span></li>
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(358377)</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>