Summary:	ASTERISK-20837: [patch] build_route fails to parse Record-Route headers longer than 255 characters
Reporter:	Corey Farrell (coreyfarrell)	Labels:
Date Opened:	2012-12-23 20:41:41.000-0600	Date Closed:	2013-01-17 23:26:03.000-0600
Priority:	Critical	Regression?
Status:	Closed/Complete	Components:	Channels/chan_sip/Interoperability
Versions:	1.8.19.0	Frequency of Occurrence	Constant
Related Issues:
Environment:		Attachments:	( 0) asterisk-large-rr-header.patch ( 1) chan_sip-build_route-optimized-rev1.patch
Description:	build_route copies each Record-Route header to char rr_copy\[256\].  When the header is longer than this it cuts parts off.  This causes the header to be parsed wrong and the call fails to connect. This issue was found when connecting with the SIP core of a large organization.  The SIP core provides a single Record-Route header with comma separated values.  My patch malloc's rr_copy to the exact length required for each header, freeing after each is processed.
Comments:	By: Michael L. Young (elguero) 2012-12-24 08:49:14.738-0600 Can you provide a sample of the Record-Route header?  Are you saying that this one sip proxy adds all the sip proxies in one header? If so, that just seems wrong based on what I am reading in the RFC in regards to how to use Record-Route. By: Corey Farrell (coreyfarrell) 2012-12-24 15:47:02.114-0600 I will be able to talk to the client on Dec26th to get permission to provide a call setup trace.  I'll likely need to submit that privately via the clients Digium contract. The sip proxy closest to Asterisk combines the existing record-route's to one header, the only proxy it adds is itself. It seems to me this is allowed per RFC3261, end of page 29: {quote}   Multiple header field rows with the same field-name MAY be present in   a message if and only if the entire field-value for that header field   is defined as a comma-separated list (that is, if follows the grammar   defined in Section 7.3).  It MUST be possible to combine the multiple   header field rows into one "field-name: field-value" pair, without   changing the semantics of the message, by appending each subsequent   field-value to the first, each separated by a comma. {quote} By: Michael L. Young (elguero) 2012-12-24 17:21:59.047-0600 Corey, it would appear that you are correct.  It seems that it is valid.  I guess it is just not that common since I would think that someone would have had this problem before now.  If you look later on in the RFC (which is what I had looked at when making my comment above) the examples provided and terminology to "insert" a Record-Route leads one to think they shouldn't do it all in one header field row.  But, it would appear that you are correct unless someone else chimes in to say otherwise. By: Michael L. Young (elguero) 2012-12-24 19:15:41.054-0600 I wonder if we could use an ast_str here and then we have a buffer to work with that grows as need be instead of malloc and free for every header being processed? By: Corey Farrell (coreyfarrell) 2012-12-24 20:16:33.248-0600 ast_str uses realloc, which has the ability to simply expand an allocation.  But in other cases realloc() has to allocate new memory, copy the existing buffer, then free the existing buffer. Not sure if ast_str would be faster, slower or equal to just having malloc/free for each header.  Probably depends on exactly how realloc is implemented and if it's able to expand an existing allocation instead of copying to a new one. For optimizing this I think that zero-copy replacements for strsep and get_in_brackets seems like the way to go.  strchr could serve as a replacement for strsep, I haven't gotten a chance to look at get_in_brackets yet.  This would make it so the only copy performed would be directly to thishop->hop. By: Corey Farrell (coreyfarrell) 2012-12-25 15:05:36.407-0600 chan_sip-build_route-optimized-rev1.patch fixes the issue by not copying the header to any temporary buffers.  Tested briefly on 1.8.19.0. This code will correctly handle valid headers: Record-Route: <sip:id1@10.10.10.10;lr> Record-Route: <sip:id1@10.10.10.10;lr>, <sip:id1@10.10.10.20;lr> Reasonable results are obtained from malformed headers: Record-Route: <sip:id1,id2@10.10.10.10;lr> * The comma is accepted as part of the value in brackets. Record-Route: <sip:id1@10., <sip:id2@10.10.10.10;lr> * The comma before 'sip:id1@10.10.10.10;lr' is treated as a separator between two values.  get_in_brackets_const first returns everything between the first open bracket and the close bracket.  But a comma is found inside with a 2nd open bracket after the comma.  This causes the part before the comma to be ignored as invalid. I might be wrong, but it seems Record-Route doesn't allow quoted strings before hops?  Example: Record-Route: "quoted text" <sip:dlg1@10.10.10.10;lr> If this is not allowed the parser can be simplified further by removing processing of quoted strings. By: Walter Doekes (wdoekes) 2013-01-04 07:36:22.037-0600 {quote}If this is not allowed the parser can be simplified further by removing processing of quoted strings.{quote} Unfortunately: {noformat} Record-Route  =  "Record-Route" HCOLON rec-route *(COMMA rec-route) rec-route     =  name-addr *( SEMI rr-param ) name-addr      =  [ display-name ] LAQUOT addr-spec RAQUOT {noformat} I just ran into this same problem with large uri-parameters.