[asterisk-dev] [Code Review]: fix odd logic in get_msg_text() and a couple of related quirks

[Terry Wilson]

> On Nov. 23, 2011, 12:46 p.m., mjordan wrote:
> > /branches/1.8/channels/chan_sip.c, line 16013
> > <https://reviewboard.asterisk.org/r/1586/diff/1/?file=21725#file21725line16013>
> >
> >     It seems very arbitrary that we only support message bodies with 1400 characters.  This whole thing seems like it would be much simpler using ast_str.
> 
> wdoekes wrote:
>     Agreed that it is better. But IMHO beyond the scope of this fix. I mainly did this patch to get rid of the "XXX" in the somments.
> 
> Terry Wilson wrote:
>     Actually, 1400 is very much *not* arbitrary. It is an RFC supplied value for the maximum length of a SIP MESSAGE request to prevent fragmentation.
> 
> wdoekes wrote:
>     It seems there already is a get_msg_text2() in the 10 branch which takes an ast_str.

Actually the limit is 1300 bytes, apparently. So, technically the get_msg_text2 should limit to 1300 bytes as well since there is no way for us to know the path MTU. From RFC-3428:

   Whenever possible, MESSAGE requests SHOULD be sent over transports
   that implement end-to-end congestion control, such as TCP or SCTP.
   However, SIP does not provide a mechanism to prevent a downstream hop
   from sending a request over UDP.  Even the requirement to use TCP for
   requests over a certain size can be overridden by the receiver.
   Therefore use of a congestion-controlled transport by the UAC is not
   sufficient.

   The size of MESSAGE requests outside of a media session MUST NOT
   exceed 1300 bytes, unless the UAC has positive knowledge that the
   message will not traverse a congestion-unsafe link at any hop, or
   that the message size is at least 200 bytes less than the lowest MTU
   value found en route to the UAS.  Larger payloads may be sent as part
   of a media session, or using some type of content-indirection.


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1586/#review4857
-----------------------------------------------------------


On Nov. 13, 2011, 9:02 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1586/
> -----------------------------------------------------------
> 
> (Updated Nov. 13, 2011, 9:02 a.m.)
> 
> 
> Review request for Asterisk Developers and Olle E Johansson.
> 
> 
> Summary
> -------
> 
> The chan_sip get_msg_text was a bit of a mess:
> - it contained strlen(buf) where buf was always terminated on 0
> - it reserved 5 bytes for the buffer for who knows what
> - it decremented the allowed size by more than needed if addnewline was off
> - the addnewline parameter was a misnomer -- and imho the wrong fix to a problem
> - the \brief docs referred to a SIP MESSAGE, but it was used to get any sip request body
> 
> I did the following:
> - I fixed the doxygen docs
> - looked over the numbers in get_msg_text() so buf is always filled to the brim if too small
> - removed the addnewline parameter that Olle added in r116240
> - instead, I stripped all trailing newlines in the function that needed it
> 
> I believe r116240 was the wrong fix for the problem of trailing newlines: if the content is text/plain, newlines should certainly be allowed. Unless they're somehow used for folding the text/plain body -- which I haven't found any docs about -- removing them could cause linefeed separated text to be incorrectly joined.
> 
> (This is not an important issue at all.. but this review is here to clean up the patch in r1533.)
> 
> 
> This addresses bug ASTERISK-18389.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18389
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 345022 
> 
> Diff: https://reviewboard.asterisk.org/r/1586/diff
> 
> 
> Testing
> -------
> 
> It compiles.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111123/3e6a632e/attachment.htm>