<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1586/">https://reviewboard.asterisk.org/r/1586/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 23rd, 2011, 12:46 p.m., <b>mjordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1586/diff/1/?file=21725#file21725line16013" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/channels/chan_sip.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static void receive_message(struct sip_pvt *p, struct sip_request *req)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">16003</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">char</span> <span class="n">buf</span><span class="p">[</span><span class="mi">1400</span><span class="p">];</span><span class="ew"> </span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">16010</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">char</span> <span class="n">buf</span><span class="p">[</span><span class="mi">1400</span><span class="p">];</span><span class="ew"> </span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 23rd, 2011, 1:43 p.m., <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 23rd, 2011, 2:21 p.m., <b>Terry Wilson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 23rd, 2011, 2:52 p.m., <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It seems there already is a get_msg_text2() in the 10 branch which takes an ast_str.</pre>
</blockquote>
<p>On November 23rd, 2011, 3:10 p.m., <b>Terry Wilson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 23rd, 2011, 4:28 p.m., <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks like a define is in order for the 1300 limit and appropriate comments documenting such. Need to eliminate the magic number.</pre>
</blockquote>
<p>On November 24th, 2011, 2:07 a.m., <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> As for limits, a check in the MessageSend() dialplan app would probably be appropriate (returning TOO_LARGE).
(A check done by the specific channel driver obviously.)</pre>
<br />
<p>- wdoekes</p>
<br />
<p>On November 13th, 2011, 9:02 a.m., wdoekes wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and Olle E Johansson.</div>
<div>By wdoekes.</div>
<p style="color: grey;"><i>Updated Nov. 13, 2011, 9:02 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It compiles.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-18389">ASTERISK-18389</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(345022)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1586/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>