[Home]

Summary:ASTERISK-08320: Large SIP messages are truncated to 4096 bytes
Reporter:mikma (mikma)Labels:
Date Opened:2006-12-10 17:05:30.000-0600Date Closed:2008-03-14 15:02:33
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Interoperability
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) sipdebug.txt
Description:According to RFC 3261, SIP implementations MUST be able to handle messages up
to 65535 bytes.
Asterisk currently can't handle SIP messages larger than 4096 bytes excluding IP and UDP headers. Larger messages are truncated.

RFC 3261, Section 18.1.1 Sending Requests

  However,
  implementations MUST be able to handle messages up to the maximum
  datagram packet size.  For UDP, this size is 65,535 bytes, including
  IP and UDP headers.
Comments:By: Tilghman Lesher (tilghman) 2006-12-10 18:54:20.000-0600

Please upload a "sip set debug" of the situation that you are having where something is sending a message larger than 4096 bytes.
By: mikma (mikma) 2006-12-11 03:06:20.000-0600

Uploaded sip debug containing INVITE from minisip soft phone.


By: Joshua C. Colp (jcolp) 2006-12-11 10:02:02.000-0600

Ah - there is an encryption key in there... that would explain why it is so big.
By: mikma (mikma) 2006-12-11 10:18:05.000-0600

Yes, it's a MIKEY offer as part of a Diffie-Hellman Key Exchange.
MIKEY messages often get pretty big since they may contain certificates.

RFC 3830, 4567
By: Russell Bryant (russell) 2006-12-13 14:10:52.000-0600

For a short-term workaround, if you want to fix this locally, you can change the SIP_MAX_PACKET define that is located near the top of chan_sip.

Thinking ahead to the long-term solution, the data field of the sip_request structure is currently statically sized at 4096 bytes.  It really needs to be dynamically sized so that it's not wasting memory and can also grow to be as big as it needs to be to accomodate large packets.
By: Olle Johansson (oej) 2006-12-15 05:02:02.000-0600

mikma, I really like your bug reports, since most of them are really challenging :-)

We need to allocate the memory in a different way, agreed.
By: Olle Johansson (oej) 2007-05-16 04:00:41

Creating branch to play around with this. Solved it already in pineapple, but that code is different from chan_sip already, so I can't take the fix from there.
By: Olle Johansson (oej) 2007-05-16 04:47:57

Try this branch, based on 1.4

svn checkout http://svn.digium.com/svn/asterisk/team/oej/sip_request_size_8556 sip_request_size

(not tested yet by myself, but it does compile)

I've changed so that we still allocate 4096 bytes for SENDING requests, but we can handle up to UDP size packets when receiving. This needs to be thourogly checked so that we don't have memory leaks.

/Olle
By: Jason Parker (jparker) 2007-09-12 13:32:55

mikma, have you had any luck with testing this branch?
By: mikma (mikma) 2007-09-17 10:43:21

After porting the patch to trunk patched with SRTP support I got the following SIGSEGV:

Program received signal SIGSEGV, Segmentation fault.
sipsock_read (id=0x462d7861, fd=1635218031, events=25714, ignore=0xd303720)
   at chan_sip.c:5408
5408                    if (msgbuf[h] == '\r') {


I would prefer a patch against trunk or a branch based on trunk.
By: Olle Johansson (oej) 2007-12-11 01:36:43.000-0600

Need to go back to this issue and fix the branch. I have no device that sends large requests though.

Mikma, if you're in the Stockholm area, maybe we could meet and test together to make sure this is fixed quickly.

/Olle
By: Brandon Kruse (bkruse) 2008-01-27 23:47:27.000-0600

Any update on this?

Can we close it?

-bk
By: James Golovich (jamesgolovich) 2008-01-28 01:51:59.000-0600

I've created a branch at http://svn.digium.com/view/asterisk/team/jamesgolovich/chan_sip-ast_str/ that handles this slightly different that oej's patch.  ast_str is used.  Still needs further testing though
By: Olle Johansson (oej) 2008-01-28 01:53:52.000-0600

No, we should not close it. I will check James' branch since I never got started on this really.
By: James Golovich (jamesgolovich) 2008-01-28 13:32:57.000-0600

I was thinking a bit about my branch to solve this, and I think I'm going to change it so it initially uses 512 as the size of the ast_str.  There might be a better size to use, but I suspect that on average most packets are less than 512 bytes so there isn't any reason to basically malloc 4096 bytes when only a small bit is used.  The ast_str code will expand the size if it needs more space.

Maybe I'll take a look and see what the average size of packets are on my test box.
By: Russell Bryant (russell) 2008-01-31 14:41:57.000-0600

James, I see some potential issues with ast_str_dup().  Also, I see that it's only used in one place right now.  How about we change this line:

if (!(dst->data = ast_str_dup(src->data, dup)))

to ...

if (ast_str_set(&dst->data, 0, "%s", src->data->str) == AST_DYNSTR_FAILED))

?
By: James Golovich (jamesgolovich) 2008-02-01 02:31:55.000-0600

I agree that ast_str_dup isn't needed, but I just realized why I didn't use ast_str_set in the first place.  After a request is parsed it all the \r\n get replaced with \0 so at that point we can't really treat it as a normal string.  Will probably need to keep doing a memcpy on it
By: jmls (jmls) 2008-02-17 14:47:20.000-0600

and we are where with this ? Does the branch need further testing ?
By: James Golovich (jamesgolovich) 2008-02-17 18:00:32.000-0600

Yes this could use further testing.  The branch is working great for me, but its possible theres something I've missed testing
By: Digium Subversion (svnbot) 2008-03-13 14:50:40

Repository: asterisk
Revision: 108439

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r108439 | russell | 2008-03-13 14:50:39 -0500 (Thu, 13 Mar 2008) | 8 lines

Merge changes from team/jamesgolovich/chan_sip-ast_str

This set of changes removes the hard coded maximum packet size of 4kB from chan_sip.
It now starts by allocating 1kB, and growing the buffer as needed to accommodate large
packets.

(closes issue ASTERISK-8320, reported by mikma, patch by jamesgolovich)

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

http://svn.digium.com/view/asterisk?view=rev&revision=108439
By: Digium Subversion (svnbot) 2008-03-14 15:02:33

Repository: asterisk
Revision: 108795

_U  branches/1.6.0/

------------------------------------------------------------------------
r108795 | russell | 2008-03-14 15:02:31 -0500 (Fri, 14 Mar 2008) | 32 lines

Blocked revisions 108404,108439,108523,108639 via svnmerge

........
r108404 | jpeeler | 2008-03-13 13:59:04 -0500 (Thu, 13 Mar 2008) | 6 lines

(closes issue ASTERISK-11288)
Reported by: ctooley
Patches:
     eivr_tcp_generic.patch uploaded by jpeeler (license 325)
This change adds the ability to communicate over a TCP socket instead of forking a child process.

........
r108439 | russell | 2008-03-13 14:54:44 -0500 (Thu, 13 Mar 2008) | 8 lines

Merge changes from team/jamesgolovich/chan_sip-ast_str

This set of changes removes the hard coded maximum packet size of 4kB from chan_sip.
It now starts by allocating 1kB, and growing the buffer as needed to accommodate large
packets.

(closes issue ASTERISK-8320, reported by mikma, patch by jamesgolovich)

........
r108523 | jpeeler | 2008-03-13 15:38:56 -0500 (Thu, 13 Mar 2008) | 1 line

set variable to NULL to prevent uninitialized warning
........
r108639 | jpeeler | 2008-03-13 18:12:59 -0500 (Thu, 13 Mar 2008) | 1 line

documenting changes as a result of adding TCP functionality to ExternalIVR
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=108795