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

[rmudgett]

> 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.
> 
> Terry Wilson wrote:
>     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.

Looks like a define is in order for the 1300 limit and appropriate comments documenting such.  Need to eliminate the magic number.


- rmudgett


-----------------------------------------------------------
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/47fe8b25/attachment-0001.htm>