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

[wdoekes]

> 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.
> 
> rmudgett wrote:
>     Looks like a define is in order for the 1300 limit and appropriate comments documenting such.  Need to eliminate the magic number.

Normally asterisk is in the be-loose-in-what-you-accept mode. I don't think we need to be trimming messages just because RFC says the sender shouldn't exceed a certain limit. Otherwise I think we can be quite a bit more strict in other places as well.

We're talking about the receive_message() method: it takes a buffer that a different UAC has created.
- if there's a dialog, it sends an AST_FRAME_TEXT (T140 over RTP) along (which probably doesn't have a similar limit)
- if there isn't a dialog, it enqueues the message (ast_msg_queue) which hits the dialplan

In neither of these cases do we (automatically) send it along over SIP, and even if we did, I still don't think we should simply trim it. One should either refuse it when it comes in or trust that the UAC (the original sender) does know the full path MTU.

(If you want to limit outgoing texts... there's add_text() and the callers of that (foremost sip_sendtext()) that we should be concerned about.)


Ergo, I'd vote for moving the get_msg_text2() from 10 to 1.8; simultaneously removing the old get_msg_text(), because having two functions that do the same thing is not good.
As for limits, a check in the MessageSend() dialplan app would probably be appropriate (returning TOO_LARGE).


- wdoekes


-----------------------------------------------------------
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/20111124/4d2140a8/attachment-0001.htm>