[Home]

Summary:ASTERISK-07210: [patch] allow SIP Spiral to work instead of causing a '482 Loop Detected' condition
Reporter:stephen dredge (stephen_dredge)Labels:
Date Opened:2006-06-21 00:13:40Date Closed:2008-07-25 12:31:39
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip_spiral.patch
( 1) chan_sip.c.patch2
( 2) sip_find_call_1.diff
( 3) sip_find_call.diff
( 4) sip_spiral.patch
( 5) sip_spiral3.patch
( 6) sip_spiral4.patch
( 7) spiral_attempt.patch
( 8) working_spiral.patch
Description:A sip call originating from asterisk causes a '482 Loop Detected' response when forwarded back to asterisk from a external proxy. This should be allowed when the request URI has been changed by the proxy and the call is now targeted at a different user.

****** ADDITIONAL INFORMATION ******

I have made a small change to the function find_call so that a INVITE request without a to tag is not incorrectly matched with the originating channel on the basis of the callid.

Now when a spiraled INVITE is received new sip_pvt is created, the to tag for the dialog (call leg) is created by asterisk in the invite response and both dialogs are then uniquely identified by the to and from tags. The call then proceeds correctly.

Note: this assumes that pedantic = true.
Comments:By: Olle Johansson (oej) 2006-06-21 04:08:44

Cool.
I have some work to prevent his too, but never got it working fully. However, I can't look at your patch until you have filed a disclaimer with Digium. Please read the bug guidelines and confirm here in the bug tracker that you have filed a disclaimer with them, then I'll start working on this.

Thanks for contributing to Asterisk!

Regards,
/Olle

By: stephen dredge (stephen_dredge) 2006-06-21 19:00:03

Ok, but it looks like I jumped the gun a little bit CANCELS don't always find the right dialog with this change, I will see if I can fix this up and submit it and and the disclaimer.

By: Olle Johansson (oej) 2006-06-22 01:45:38

I know, it's hard. I've tried to get it right two times. If you contact me on IRC, we can discuss this a bit and perhaps solve it. I can point you to my test code. I am "oej" on IRC freenode.net - #asterisk-dev

Usually online weekdays, but this thursday and friday I'll be mostly offline.
/O

By: stephen dredge (stephen_dredge) 2006-07-05 21:06:26

The new patch should work correctly and the Disclaimer has been faxed.

You can potentialy get a few strange things happening if the proxy does not behave correctly. For instance openser will incorrectly send a intrium '200 cancelling' response to a CANCEL with a new tag thus creating a new dialog. Asterisk now ( i think ) keeps retransmitting the cancel waiting for a 487 from the dialog created by the proxy after it gets one from the remote end.

By: Olle Johansson (oej) 2006-07-06 02:02:19

Thanks, will try to test this next week.

By: Andriy I Pylypenko (bamby) 2006-09-04 07:35:05

I've faced similar problem and have done some investigation of asterisk's code performing SIP dialog recognition and found out that it misses very important point - while matching tags from SIP packet against tags from a channel the role of the channel being tested (UAC or UAS) is to be considered thoroughly as this role dictates how to treat tags. For example local tag for the SIP channel of UAC type is sent in From fields of both requests and responses regardless of where requests and responses come from and to (for example consider re-INVITE).

This role is properly handled throughout the rest of the code but the find_call() function ignores it completely concentrating on SIP packet type (response/request) instead.

The patch chan_sip_spiral.patch also ignores the role of the channel. So I would like to present different patch.

The patch is strictly conformant to RFC3261. It has been made with readability as the main goal.

By: Serge Vecher (serge-v) 2006-09-05 10:33:18

bamby: thanks for the patch. Can you please confirm your disclaimer status?

By: miconda (miconda) 2006-09-05 10:41:53

<snip>
You can potentialy get a few strange things happening if the proxy does not behave correctly. For instance openser will incorrectly send a intrium '200 cancelling' response to a CANCEL with a new tag thus creating a new dialog. Asterisk now ( i think ) keeps retransmitting the cancel waiting for a 487 from the dialog created by the proxy after it gets one from the remote end.
</snip>

Not sure that it was fixed by the latest patch, but the above statement is wrong.

An stateful proxy has to reply back 200 OK to any CANCEL that matches an ongoing call and then CANCEL all the branches associated with that call. Have in mind that the stateful proxy can do parallel forking, so it has to CANCEL multiple branches. It cannot send back the replies from all branches. Please refer to RFC3261 sections 9.2 and 16.10

By: stephen dredge (stephen_dredge) 2006-09-05 23:52:24

The new patch seems to be reversed. A spiraled Invite is matched with the originating channel.

The approach seems good but i don't think it's workable.

Quote RFC 3261:
12.2. Requests within a Dialog

  Once a dialog has been established between two UAs, either of them
  MAY initiate new transactions as needed within the dialog.  The UA
  sending the request will take the UAC role for the transaction.  The
  UA receiving the request will take the UAS role.  Note that these may
  be different roles than the UAs held during the transaction that
  established the dialog.

So we can't rely on just the SIP_OUTGOING flag. Afaik it is only set once when the initial Invite is sent on a new channel. If the UAS for instance reinvites, it sets the to and from tags as if it was the UAC and thus a request will match on the channel that created it.

miconda:
yes you a right a stateful proxy should send a 200  ok to a cancel if there is a existing dialog, but it shouldn't create a new dialog/context  by creating a new to tag. It should use a existing dialog for the response.

By: Andriy I Pylypenko (bamby) 2006-09-06 10:08:29

stephen_dredge: thank you very much for the quote. I happened to miss that. This changes things a bit but general approach can be preserved. Please review new version of the patch. Initial channel role now helps to find out what tag is mandatory and what is optional. In addition creation of a new channel for unrecognized ACK, BYE, CANCEL requests and unmatched responses is now prevented.
serge-v: I've faxed disclamer with bug report 4825. Can it be reused for these patches?

By: Serge Vecher (serge-v) 2006-09-06 10:28:41

bamby: if it was the first (short) version, then yes. If the second (long), then you will have to file a new disclaimer. Thanks.

By: stephen dredge (stephen_dredge) 2006-09-12 02:46:48

Find_call_1 doesn't work properly either. the original patch works but I have a few improvements. I'll post it in a couple of days when I have tested it some more.

By: please_remove_this_account (quintana) 2006-09-15 09:59:10

Hi stephen_dredge,

What version of asterisk did you use for this lastest patch please ?

By: stephen dredge (stephen_dredge) 2006-09-19 02:58:38

The original patch was against the then current CVS 37122.

I have got another patch which should be correct and more complete but I think I found a problem elsewhere in chan_sip. It looks  like there is a problem with the reqprep sub routine and it manages to get the to and from fields mixed up on the ACK to a reinvite ( they should be the same as on the reinvite should they not? ). I need to go browse some RFC's some more.

It looks like the current code is working under the assumption that the UAC/UAS roles are fixed for the duration of a dialog, but I don't see that this is the case.

By: stephen dredge (stephen_dredge) 2006-09-26 03:19:13

The new attached patch handles spiraled calls correctly and drops requests which don't have the correct tags. Also sends 491 error for requests with no matching dialog.

I have made a bit of a nasty patch to reqprep to get it to work correctly. reqprep reverses the from and to headers on a ack when the channel is not outgoing.

By: SimonJ (simon67) 2006-10-10 12:23:56

Hi Stephen_dredge,
I can't see the last patch you have attached. Could you upload it again ?
Regards,
Simon

By: stephen dredge (stephen_dredge) 2006-10-10 20:41:58

Doh! Patch is attached now.
I have reported the problem with reqprep as  bug 0008130 as it is really a separate issue.

By: SimonJ (simon67) 2006-10-11 10:29:36

Thanks a lot. I would like to use the patch to try it on our dev server (since we face this problem), but I can't find where to download the revision 43649 (we are currently using Asterisk 1.2.10). Thanks.
Regards,
Simon

By: jmls (jmls) 2006-11-08 05:41:41.000-0600

bamby: please could you confirm your disclaimer status ? Thanks.

By: Olle Johansson (oej) 2006-11-08 16:28:14.000-0600

The formatting of the chan_sip.c.patch2 is not in order. Could someone reformat it to follow the guidelines?

By: Andriy I Pylypenko (bamby) 2006-11-09 08:46:45.000-0600

I've just faxed the short version of disclaimer.

By: stephen dredge (stephen_dredge) 2006-11-10 01:41:16.000-0600

I tried to follow the style of the existing code, is there any thing in particular which needs to be fixed in the pacth formatting.

By: Olle Johansson (oej) 2006-11-12 14:46:25.000-0600

Committed a small part of this to trunk. Still need reformatting. You need to use tabs to indent. There's a lot of errors in formatting of this code.

Please update the patch to latest trunk. Seems to be very good work!!

By: stephen dredge (stephen_dredge) 2006-11-15 07:33:11.000-0600

reformated patch to latest svn attached

By: Serge Vecher (serge-v) 2006-11-17 09:01:57.000-0600

stephen_dredge:

Here are some more pointers on why oej mentioned "lot of errors in formatting"
1) Redundant spacing in places like:
+ if ( req->method == SIP_CANCEL && totag[0] == '\0'  && strcmp(p->tag, fromtag))
(between '(' and 'req->method' and between '\0' and '&&')

2) Single line 'if ... else' constructs do not need curly brackets.

Thanks for your work!

By: Andrew Whalan (awhalan) 2006-11-21 01:41:50.000-0600

I know you guys have basically fixed this issue now, but I can confirm it happens for me on Asterisk 1.2.7.1-BRIstuffed-0.3.0-PRE-1n using the transfer function a SNOM 320 IP phone. It doesn't happen all the time it seems, just seemingly randomly.

Is there a temporary work around to move calls between phones until this patch hits packages? Should I contact SNOM?



By: hristo (hristo) 2006-12-07 07:48:13.000-0600

This was reported at the time when 1.4 wasn't branched.
It says "trunk" in the detail above, but is it really trunk or 1.4?

I can put this to testing if you provide up to date patch for 1.4. The current patch fails to apply to 1.4-svn-r48347

# patch -p0 < ~/sip_spiral3.patch
patching file channels/chan_sip.c
Hunk #1 succeeded at 4275 (offset -75 lines).
Hunk #2 FAILED at 4318.
1 out of 2 hunks FAILED -- saving rejects to file channels/chan_sip.c.rej

By: stephen dredge (stephen_dredge) 2006-12-07 17:35:02.000-0600

Seems to apply cleanly to svn trunk today.

[root@steve]#svn checkout http://svn.digium.com/svn/asterisk/trunk asterisk
[root@steve asterisk]# svnversion .
48358

[root@steve asterisk]# patch  -p0 < sip_spiral3.patch
patching file channels/chan_sip.c
Hunk #1 succeeded at 4386 (offset 36 lines).

By: hristo (hristo) 2006-12-08 03:12:18.000-0600

Well, it's for trunk after all...I was trying to patch 1.4 branch :)

I'm not sure if this is considered feature or bug fix and if it will actually make it into the 1.4 branch (I'll definitely love to have there), but if you can also provide patch for 1.4 I can test it either way and report my findings.

I'm trying to loop calls via sip proxy back to asterisk to generate proper billing for call forwarding, but I get that "482 Loop Detected" and calls are dropped.

By: Olle Johansson (oej) 2006-12-08 04:23:24.000-0600

Will open a branch for this, so we can keep it up to date with trunk. Sorry for not testing yet. I'm trying to focus to on fixing the issues with 1.4 to get that release done. I am really interested in this work.

/Olle

By: juan salas (jsalas) 2007-03-01 13:30:19.000-0600

There is some version to patch the stable 1.2.x?

By: Serge Vecher (serge-v) 2007-03-01 14:01:39.000-0600

jsalas: no, this is strictly *trunk* work.

By: Olle Johansson (oej) 2007-05-16 15:07:27

Created branch "sip_spiral_7403"

By: Olle Johansson (oej) 2007-05-16 15:26:42

Integrated the patch in rev 48358, now upgrading the branch to current svn with automerge.

By: gautam_dawar (gautam_dawar) 2007-08-27 04:13:24

I am unable to access any of the attached files here. I want to see the patch chan_sip.c.patch2 but can't find a link for the same. Can you please help?

By: Iñaki Baz Castillo (ibc) 2007-08-27 20:37:12

Hi, I'm very interested too in this issue because I use OpenSer and need to forward SIP calls coming from Asterisk to Asterisk again but modified URI.

Maybe somebody could explain me how to try a version with the patch? (anyway I see "Not licenced" in the patches).

I've installed trunk 48358 version and it doesn't work (pedantic mode enabled but it rejects LoopBack with URI changed in a SIP proxy).

Thanks for any help.

By: Leif Madsen (lmadsen) 2007-08-28 12:42:29

You won't be able to access the patches until the original poster (stephen_dredge) submits a disclaimer, which can be done online in the bug tracker (here) with a link at the top. This is a new method which allows Digium to better track the licenses and control patches uploaded to the tracker.

Prior disclaimers are now moot, and the new online disclaimer will need to be sent. The process only take a minute, and approval is usually very fast (a day or two).

By: gautam_dawar (gautam_dawar) 2007-08-29 05:00:48

Hi stephen_dredge,

Can you please either submit the disclaimer or mail me the patch at gautam.dawar@gmail.com?

Actually I want to know that when the external proxy sends the same Call-ID and fromtag to asterisk in successive INVITEs, how do you distinguish between the first and second invite in find_call method?

By: Iñaki Baz Castillo (ibc) 2007-08-29 10:19:19

I ask too por the patch, please. I'd like to test it because I use OpenSer with Asterisk and calls from Asterisk to OpenSer coming back to Asterisk with modified URI are dropped by Asterisk because "LoopBack" detected.

I hope you could sign the disclaimer or send me the patch to ibc@aliax.net ?

Thanks a lot.

By: Jason Parker (jparker) 2007-08-29 14:19:51

For patches that were uploaded and disclaimer prior to the Mantis switch-over, there is a magic button we can click.  This button has been clicked.  You can now see the patches. :)

By: Iñaki Baz Castillo (ibc) 2007-08-29 15:10:46

Ohhh, I can't believe. "sip_spiral3.patch" is only valid for 48358 SVN revision (I think at least) so I've patched that Asterisk version sucessfuly, but now I **can't** try the patch because this version of Asterisk just crashes ALWAYS when receiving any SIP call   :(

Could it because I use an Asterisk 1.4.11 /etc/asterisk files? I get no error when doing "CLI>reload"

Thanks in advance for any suggestion.

By: Iñaki Baz Castillo (ibc) 2007-08-29 15:26:16

No, now I've deleted /etc/asterisk and done "make samples" in the 48358 SVN revision and it crashes anyway ALWAYS when receiving a SIP call. No debug (with "debug" enabled in "full" log file), no SIP response traced with ngrep, nothing.... :(

By: gautam_dawar (gautam_dawar) 2007-08-30 01:09:56

qwell: Thanks a lot for making the patches available.

However, there is a little issue with the patch. ;)

ibc: The crashes you are encountering are because of the invalid initialization of the variable chan_is_outgoing in the function find_call. When this variable is being initialized, sip_pvt pointer 'p' is pointing to NULL and hence call to ast_test_flag at that point causes the crash.

To fix the issue, just update the if statement where chan_is_outgoing is being evaluated.
i.e.
Change
<snip>
/* UAC channel does not have a dialog yet. reponse has a to tag establishing a new dialog */
if ( p->theirtag[0] == '\0' && chan_is_outgoing && ( req->method == SIP_RESPONSE )) {
</snip>

to

<snip>
/* UAC channel does not have a dialog yet. reponse has a to tag establishing a new dialog */
if ( p->theirtag[0] == '\0' && ast_test_flag(&p->flags[0], SIP_OUTGOING) && ( req->method == SIP_RESPONSE )) {
</snip>

By: Iñaki Baz Castillo (ibc) 2007-08-30 03:26:30

oh yes! it works! thanks a lot for your fix.

I've done some test and it seems to work, now Asterisk can call to an user in OpenSer and OpenSer rewrites URI and takes back the INVITE to Asterisk, so now Asterisk accepts the INVITE as a new leg. In fact Asterisk respects the Record-Route and sends the "OK" to OpenSer (which takes it back to Asterisk again).

Now a question, will it be integrated soon in the SVN trunk? I just could do it work with asterisk trunk svn 48358.

Thanks again and regards.

By: Leif Madsen (lmadsen) 2007-08-30 08:09:15

gautam_dawar: that's a pretty trivial one line fix, so we can leave it, but generally comments with code need to be deleted, and a patch submitted. If you haven't already done so, please sign a disclaimer by clicking the link at the top.

That way, next time you want to submit a patch, it'll already be done! If you have a chance to make a patch with your changes and upload it, I think the developers would appreciate it.

Thanks for testing guys! Testing lets the developers know the code works. There is no time frame for when it'll make it into trunk, but if a couple more people test it and the code passes review, it should make it in reasonably quickly.

By: Iñaki Baz Castillo (ibc) 2007-08-31 10:38:11

Hi, I read as the last action:

"File Added: sip_spiral4.patch"

but can't see that file in the attachment list. ¿?

By: gautam_dawar (gautam_dawar) 2007-09-10 04:04:35

Uploaded the patch file.

By: Iñaki Baz Castillo (ibc) 2007-09-24 02:54:54

Jeremy Mcnamara will handle this issue during Astricon 2007:

http://www.jeremy-mcnamara.com/2007/09/21/astricon-2007-codezone-plans/

:)

By: Abhay Gupta (agupta) 2007-09-26 09:07:38

i am trying to fix the issue and test with stable 1.2 branch but unable to patch with any of the attached patches . Please provide me with the correct patch .

We are having the problem with ARICENT SIP server as a proxy and should be able to test and provide the result fast .

By: Leif Madsen (lmadsen) 2007-09-26 09:10:35

From the bug note, looks like this should be applying to trunk, and probably 1.4 at the very least, since no bug fixes will be going into the 1.2 branch anymore as it is in security maintenance mode now.

By: Abhay Gupta (agupta) 2007-09-26 09:27:12

I am not able to patch even with 1.4.4

By: Brett Nemeroff (brettnem) 2007-10-04 09:21:27

What is the status of this? I assume there are a lot of people out there that really need this for production use?

Thanks!

By: Jim Lohiser (hilozer) 2007-11-05 20:29:50.000-0600

I am also seeing this problem when attempting to use DD-WRT VoIP package v24. The newer versions of DD-WRT use Milkfish, which seems to be OpenSER based. This is unfortunate because a Linksys or other DD-WRT router with an Asterisk compatible SIP proxy would make an awesome SOHO solution.

By: Olle Johansson (oej) 2007-12-16 03:26:22.000-0600

re-categorizing this lost bug to chan_sip, so I see it in my daily work. Something happened when upgrading mantis. Sorry.

By: Alex Hermann (alexh) 2007-12-24 06:02:55.000-0600

I patched 1.4.15 with the latest spiral_patch.

Although it works in preventing the 482, it exposes another issue: inbound authentication doesn't work anymore. Apparently, the nonce is stored with the first (unauthenticated) request. As the second request with authentication info comes in it isn't matched to the first request (as it should with this patch, because it is a new transaction) and authentication fails.

By: Mark Michelson (mmichelson) 2008-05-27 14:39:45

I assigned this issue to myself in the hope that I can have a solution in a reasonable period. I've spent a couple of days already dedicated to getting this just right. This is quite difficult, but I am confident that I can work out a sensible solution to this.

By: Iñaki Baz Castillo (ibc) 2008-05-28 02:41:07

If it can help I point to the RFC 3261 sections in which it's explained how to match loop and spiral for an incoming request:

--------------------------------
16.3 Request Validation

  4. Optional Loop Detection check

     An element MAY check for forwarding loops before forwarding a
     request.  If the request contains a Via header field with a sent-
     by value that equals a value placed into previous requests by the
     proxy, the request has been forwarded by this element before.  The
     request has either looped or is legitimately spiraling through the
     element.  To determine if the request has looped, the element MAY
     perform the branch parameter calculation described in Step 8 of
     Section 16.6 on this message and compare it to the parameter
     received in that Via header field.  If the parameters match, the
     request has looped.  If they differ, the request is spiraling, and
     processing continues.  If a loop is detected, the element MAY
     return a 482 (Loop Detected) response.


16.6 Request Forwarding

     8. Add a Via header field value

        The proxy MUST insert a Via header field value into the copy
        before the existing Via header field values.  The construction
        of this value follows the same guidelines of Section 8.1.1.7.
        This implies that the proxy will compute its own branch
        parameter, which will be globally unique for that branch, and
        contain the requisite magic cookie. Note that this implies that
        the branch parameter will be different for different instances
        of a spiraled or looped request through a proxy.

        Proxies choosing to detect loops have an additional constraint
        in the value they use for construction of the branch parameter.
        A proxy choosing to detect loops SHOULD create a branch
        parameter separable into two parts by the implementation.  The
        first part MUST satisfy the constraints of Section 8.1.1.7 as
        described above.  The second is used to perform loop detection
        and distinguish loops from spirals.

        Loop detection is performed by verifying that, when a request
        returns to a proxy, those fields having an impact on the
        processing of the request have not changed.  The value placed
        in this part of the branch parameter SHOULD reflect all of
        those fields (including any Route, Proxy-Require and Proxy-
        Authorization header fields).  This is to ensure that if the
        request is routed back to the proxy and one of those fields
        changes, it is treated as a spiral and not a loop (see Section
        16.3).  A common way to create this value is to compute a
        cryptographic hash of the To tag, From tag, Call-ID header
        field, the Request-URI of the request received (before
        translation), the topmost Via header, and the sequence number
        from the CSeq header field, in addition to any Proxy-Require
        and Proxy-Authorization header fields that may be present.  The
        algorithm used to compute the hash is implementation-dependent,
        but MD5 (RFC 1321 [35]), expressed in hexadecimal, is a
        reasonable choice.  (Base64 is not permissible for a token.)

        If a proxy wishes to detect loops, the "branch" parameter it
        supplies MUST depend on all information affecting processing of
        a request, including the incoming Request-URI and any header
        fields affecting the request's admission or routing.  This is
        necessary to distinguish looped requests from requests whose
        routing parameters have changed before returning to this
        server.

        The request method MUST NOT be included in the calculation of
        the branch parameter.  In particular, CANCEL and ACK requests
        (for non-2xx responses) MUST have the same branch value as the
        corresponding request they cancel or acknowledge.  The branch
        parameter is used in correlating those requests at the server
        handling them (see Sections 17.2.3 and 9.2).
------------------

By: Mark Michelson (mmichelson) 2008-05-28 11:11:24

Much appreciated. Yesterday, I managed to write a method which successfully spiralled an INVITE through Asterisk. The problem is that my code is not nearly robust enough (I'll get into why a bit later). I'll post the patch I wrote and anyone can feel free to modify or critique it. I'll be actively working on this more today as well.

The way this implementation works is quite different than the approaches taken by others on this issue so far. Previous attempts changed the find_call function of chan_sip in order to match the call-id and the tags when run in pedantic mode. A spiralled INVITE would not match properly (due to the fact that a spiralled INVITE will contain no To: tag but Asterisk will have generated a local tag for the dialog already) and so a new dialog would be established with the new recipient of the call.

In my implementation, I don't modify the find_call function at all. Instead, I added a few lines to handle_request_invite(), inside the block where loop detection happens. Since I imagine that about 95% of SIP spirals are INVITEs where the request-URI is changed, I do a string comparison of the old vs. new request URI. If the new request URI is different than the old, then I treat this as a spiral. I then parse out the section between "sip:" and the "@" of the request URI, and call create_addr() and transmit_invite(). The result is that the existing dialog in Asterisk is modified to have the proper routing information and the call can proceed.

As I said very early on, this method is not even close to being robust enough to commit. It relies on several assumptions, some of which I'm fine with and others I'm not so fine with.

1. Spiral detection is limited to INVITEs and will only be properly detected if the Request URI has been modified. This is one of the assumptions that I'm okay with since for one thing, Asterisk has never done loop detection on any other request type, and also I estimate that 95% of spirals (maybe more) are identifiable by inspecting the request URI.

2. The comparison of request-uri's is a simple string comparison. The URI comparison should probably be more refined to take into account the fact that there could be differences in the parameters of the Request-URI. Also, there could be a strange case where the host portion of the URI is changed textually, but from a routing perspective equates to no change (i.e. a hostname is replaced with one of its IP addresses).

3. Most importantly, the code is written in a way that assumes that the "user" section of the Request-URI is a peer registered to the local Asterisk server. This is probably the most grievous of the assumptions made in the code so far, and needs the most work towards being correct. I made an adjustment at one point to attempt routing using the IP address and port of the Requeust-URI, but there is some packet manipulation needed before that will work correctly.

Okay, so as I said, I will be working on this more today to get it past the bare-bones implementation that is there so far. Please don't laugh too hard at the patch when I upload it :)

***EDIT***

One last detail to mention: this patch will not function properly if run in pedantic mode.



By: Iñaki Baz Castillo (ibc) 2008-05-28 11:27:39

Hi, you said:
---
2. The comparison of request-uri's is a simple string comparison. The URI comparison should probably be more refined to take into account the fact that there could be differences in the parameters of the Request-URI. Also, there could be a strange case where the host portion of the URI is changed textually, but from a routing perspective equates to no change (i.e. a hostname is replaced with one of its IP addresses).
---

Note that RFC 3261 says that an IP as URI hostpart **doesn't** match a domain resolving that IP. There are also some complex rulex when comparing URI parameters:
http://tools.ietf.org/html/rfc3261#section-19.1.4

19.1.4 URI Comparison

     o  An IP address that is the result of a DNS lookup of a host name
        does not match that host name.

By: Mark Michelson (mmichelson) 2008-05-28 11:32:19

Wow, I somehow completely glossed over the URI comparison section of RFC 3261. Thanks for pointing it out.

By: Mark Michelson (mmichelson) 2008-05-28 11:42:02

One additional comment about the patch I uploaded. It is against 1.4 branch.

By: Mark Michelson (mmichelson) 2008-06-18 16:34:40

I have uploaded working_spiral.patch. This has passed the "works for me" stage of development.

Here's a breakdown of the patch:

If a possible loop is detected on an incoming INVITE, a comparison is made to see if the Request URI has changed. If pedantic is turned on, then the comparison will follow RFC 3261 section 19.1.4's rules for URI comparison. If pedantic is turned off, then the comparison is just a string comparison of the two URI's. Asterisk then will update the routing information and send the INVITE to the new URI.

I would like to see if it works for others as well. The patch is made against the 1.4 branch of Asterisk. Thanks!

By: Iñaki Baz Castillo (ibc) 2008-06-19 02:31:20

Thanks a lot for your work. I'll try it ASAP, but let me some long days (1-2 weeks) since I'll be very busy.

By: Mark Michelson (mmichelson) 2008-06-23 15:24:27

I meant to move this issue to "Ready for testing" when I put the latest patch up, but I forgot. :)

By: Iñaki Baz Castillo (ibc) 2008-07-01 09:13:47

Hi, I'm trying to patch chan_sip.c (rev 123742) but I get this error:

/usr/src/asterisk-trunk$ patch -p0 < working_spiral.patch
patching file channels/chan_sip.c
Hunk #1 FAILED at 4641.
Hunk #2 succeeded at 17406 (offset 3642 lines).
Hunk #3 succeeded at 17734 with fuzz 1 (offset 3658 lines).
1 out of 3 hunks FAILED -- saving rejects to file channels/chan_sip.c.rej

It's seems that the path is done for this 123742 revision, isn't?

The content of channels/chan_sip.c.rej is:

-------------
***************
*** 4641,4648 ****
                       if (!found && option_debug > 4)
                               ast_log(LOG_DEBUG, "= Being pedantic: This is not our match on request: Call ID: %s Ourtag <null> Totag %s Method %s\n", p->callid, totag, sip_methods[req->method].text);
               }
-
-
               if (found) {
                       /* Found the call */
                       ast_mutex_lock(&p->lock);
--- 4641,4646 ----
                       if (!found && option_debug > 4)
                               ast_log(LOG_DEBUG, "= Being pedantic: This is not our match on request: Call ID: %s Ourtag <null> Totag %s Method %s\n", p->callid, totag, sip_methods[req->method].text);
               }
               if (found) {
                       /* Found the call */
                       ast_mutex_lock(&p->lock);
***************
*** 17985,17994 ****
               being able to call yourself */
               /* If pedantic is on, we need to check the tags. If they're different, this is
               in fact a forked call through a SIP proxy somewhere. */
-               transmit_response(p, "482 Loop Detected", req);
-               p->invitestate = INV_COMPLETED;
-               sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
-               return 0;
       }

       if (!ast_test_flag(req, SIP_PKT_IGNORE) && p->pendinginvite) {
--- 18248,18289 ----
               being able to call yourself */
               /* If pedantic is on, we need to check the tags. If they're different, this is
               in fact a forked call through a SIP proxy somewhere. */
+               int different;
+               if (pedanticsipchecking)
+                       different = sip_uri_cmp(p->initreq.rlPart2, req->rlPart2);
+               else
+                       different = strcmp(p->initreq.rlPart2, req->rlPart2);
+               if (!different) {
+                       transmit_response(p, "482 Loop Detected", req);
+                       p->invitestate = INV_COMPLETED;
+                       sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
+                       return 0;
+               } else {
+                       /* This is a spiral. What we need to do is to just change the outgoing INVITE
+                        * so that it now routes to the new Request URI. Since we created the INVITE ourselves
+                        * that should be all we need to do.
+                        */
+                       char *uri = ast_strdupa(req->rlPart2);
+                       char *at = strchr(uri, '@');
+                       char *peerorhost;
+                       struct sip_pkt *pkt = NULL;
+                       ast_log(LOG_NOTICE, "Spiral detected\n");
+                       if (at) {
+                               *at = '\0';
+                       }
+                       /* Parse out "sip:" */
+                       if ((peerorhost = strchr(uri, ':'))) {
+                               *peerorhost++ = '\0';
+                       }
+                       create_addr(p, peerorhost);
+                       ast_string_field_free(p, theirtag);
+                       for (pkt = p->packets; pkt; pkt = pkt->next) {
+                               if (pkt->seqno == p->icseq && pkt->method == SIP_INVITE) {
+                                       AST_SCHED_DEL(sched, pkt->retransid);
+                               }
+                       }
+                       return transmit_invite(p, SIP_INVITE, 1, 2);
+               }
       }

       if (!ast_test_flag(req, SIP_PKT_IGNORE) && p->pendinginvite) {

-------------

By: Mark Michelson (mmichelson) 2008-07-01 09:45:42

Sorry. The patch was made against the 1.4 branch of Asterisk.

By: Iñaki Baz Castillo (ibc) 2008-07-02 02:22:35

Ok, so in which revision of chan_sip.c must I apply the patch? Thanks a lot.

By: Sean Bright (seanbright) 2008-07-02 05:42:40

The patch itself was made against revision 123742 of chan_sip.c.  As putnopvut mentioned, the patch was made against the 1.4 branch (http://svn.digium.com/svn/asterisk/branches/1.4).

By: Mark Michelson (mmichelson) 2008-07-25 12:30:57

Hmmm, for some reason svnbot didn't close this issue when I fixed this. I tweaked the patch that I had attached after I found some flaws and committed the fix in rev 132790 to the 1.4 branch. Committed to trunk in rev 132795.

Any further issues with SIP spirals should be opened in new bug reports about specific problems with the now-implemented spiral code.

Thanks.