[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.

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.


> On Nov. 23, 2011, 12:46 p.m., mjordan wrote:
> > /branches/1.8/channels/chan_sip.c, line 16025
> > <https://reviewboard.asterisk.org/r/1586/diff/1/?file=21725#file21725line16025>
> >
> >     For SIP MESSAGEs, do we strip the linefeed when creating the sip_request?  If not, this will now append an extra '\n' to each line
> 
> mjordan wrote:
>     Well, I reread the initial description again, and realized you're implying that they aren't.  So, in that case, disregard this.

As I understand it. The problem was this:

Common SIP MESSAGE bodies are in text/plain. Because SIP has a Content-Length there isn't any need for any delimiting linefeeds. The most common short messages do not have any linefeeds in them.

However, the SIP body parser that exists right now in chan_sip, splits the body by line, replacing any trailing (CR)LF with NUL. This function reassembles the lines of the body, but always adds a trailing linefeed. This causes single line text messages that have no trailing LF to get an added LF. I think this is why Olle added the addnewline parameter. My proposed solution is to only trim any trailing LF, not LF's that may also be present in the middle of a message.

(The better fix would obviously to return the original body verbatim. But AFAIK, there is no original left, only the mutilated one with NULs over it. And fixing that is beyond the scope of this fix.)


- 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/20111123/8b861eca/attachment.htm>