[Home]

Summary:ASTERISK-23306: chan_sip: Asterisk creates ACK with empty Route: headers
Reporter:Matt Jordan (mjordan)Labels:
Date Opened:2014-02-13 14:36:18.000-0600Date Closed:2014-02-21 11:27:47.000-0600
Priority:MajorRegression?
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:SVN Frequency of
Occurrence
Related
Issues:
is caused byASTERISK-22582 [patch] chan_sip refactor - sip_route
Environment:Attachments:( 0) chan_sip-add_route.patch
( 1) chan_sip-route.patch
( 2) typescript.gz
Description:When sending an ACK to a 200 OK, {{chan_sip}} is sending a Route: header unpopulated with any value. Hilarity ensues as the device continues to send the 200 OK to Asterisk.

Example from the attached log file for channel {{SIP/200-00000002}}:

{noformat}
--- (11 headers 16 lines) ---
Transmitting (NAT) to 10.10.6.214:5061:
ACK sip:200@10.10.6.214:5061;ob SIP/2.0

Via: SIP/2.0/UDP 10.10.9.206:5060;branch=z9hG4bK1d66c404;rport

Route:

Max-Forwards: 70

From: "F208" <sip:208@10.10.9.206>;tag=as7688ab8f

To: <sip:200@10.10.6.214:5061;ob>;tag=575da22f-4f89-40c4-b8f3-6bee207b6f07

Contact: <sip:208@10.10.9.206:5060>

Call-ID: 5958b06f2925d5a2670f31f86dc90d01@10.10.9.206:5060

CSeq: 102 ACK

User-Agent: Asterisk PBX (digium)

Content-Length: 0
{noformat}

Comments:By: Corey Farrell (coreyfarrell) 2014-02-13 16:48:02.792-0600

I was able to reproduce this issue using sipp.

The issue happened when:
* No Record-Route was received
* Contact header does not contain ";lr" - causing strict mode.

This caused add_route to add the empty header since {{sip_route_empty}} is false, but {{sip_route_list}} produced an empty string.  

The attached patch causes {{sip_route_list}} to return NULL when formatcli==0, skip > 0 and all hops are skipped.

Another way to fix this would be to make {{sip_route_empty}} into a method that accepts a skip parameter, that way sip_route_empty(1) would be true for a route with one or less hops.
By: Corey Farrell (coreyfarrell) 2014-02-18 14:41:53.058-0600

I think it's best to just have {{add_route}} just check for blank string return from {{sip_route_list}}.  chan_sip-add_route.patch does this.

The other options I was thinking of make the API more confusing for the sake of avoiding a 64 char ast_str allocation.
By: Walter Doekes (wdoekes) 2014-02-18 15:25:42.378-0600

Yes.

If we view the patch in it's surroundings:
{noformat}
static void add_route(struct sip_request *req, struct sip_route *route, int skip)
{
       struct ast_str *r;

       if (sip_route_empty(route)) {
               return;
       }

       if ((r = sip_route_list(route, 0, skip))) {
               if (ast_str_strlen(r)) {
                       add_header(req, "Route", ast_str_buffer(r));
               }
               ast_free(r);
       }
}
{noformat}
We see that the ugliness is contained to a small function :)

And comparing to the old pre-r3173 code:
{noformat}
static void add_route(struct sip_request *req, struct sip_route *route)
{
       char r[SIPBUFSIZE * 2];

       if (!route)
               return;

       make_route_list(route, r, sizeof(r));
       add_header(req, "Route", r);
}
{noformat}

The only thing that stands out is that the route_list is apparently empty in the old case, whereas it is filled with a single entry in the new case.

AFAICT, we've traded that in for clarity here:
{noformat}
add_route(req, is_strict ? p->route->next : p->route);
->
add_route(req, &p->route, is_strict ? 1 : 0);
{noformat}

At the end of the day, I'm fine with the second patch.
By: Matt Jordan (mjordan) 2014-02-21 07:00:21.020-0600

Works for me as well; I'd say get it in :-)