<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/1690/">https://reviewboard.asterisk.org/r/1690/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 25th, 2012, 10:58 a.m., <b>Matt Jordan</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/1690/diff/1/?file=23605#file23605line3655" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/manager.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 int action_command(struct mansession *s, const struct message *m)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3652</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">AST_STRING_FIELD</span><span class="p">(</span><span class="n">data</span><span class="p">);</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;">I'm not sure I agree with this change. For read purposes, string fields are treated as const char * const - for write purposes, you have to use the string field API. The data field ends up being passed as a void * to ast_request - and its up to the individual channel drivers to interpret them.
A channel driver may assume that they can cast this to a char and modify the internals of the string field, going outside of the normal string field API calls.</pre>
</blockquote>
<p>On January 25th, 2012, 11:46 a.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;">Channel drivers should not do that anyway precisely because they cannot make an assumption about the buffer they are dealing with.
I have not seen a channel driver or most other parsing functions in Asterisk that did not first copy the string into a local buffer for dissecting.</pre>
</blockquote>
<p>On January 25th, 2012, 12:19 p.m., <b>Matt Jordan</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;">While I haven't looked through all of them, chan_phone assigns data to a char*:
char *name = data;
Later on, chan_phone does the following:
size_t length = strlen(p->dev + 5);
if (strncmp(name, p->dev + 5, length) == 0 &&
!isalnum(name[length])) {
p->dev is a char buffer of length 256. Your string fields are initialized to a length of 252. So, in this case, if someone originated through AMI and created a channel of this type, you'd end up looking into ... something in the string field pool, I'm not sure where or what.
Granted, chan_phone shouldn't be doing any of this. Its making assumptions it shouldn't be making. However, you're still stripping off the const'ness and assuming that the channel techs will do the right thing and treat it as immutable data. Since it isn't documented anywhere that they need to treat the void * as immutable, it's not fair to assume that they will.</pre>
</blockquote>
<p>On January 25th, 2012, 12:44 p.m., <b>Matt Jordan</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;">FYI: chan_sip also just assigns the void * to a char *. Although in that particular case, it shouldn't cause any problems, as it doesn't attempt to modify the contents of the char *, or index to some locally known location.
</pre>
</blockquote>
<p>On January 25th, 2012, 1:25 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;">That section of code in chan_phone is not changing the contents of data, nor is it accessing beyond the length of data. The initial size of the string field memory pool is irrelevant here. The string fields here are initialized with an allocated string memory pool with *initial* size of 252 and since it is allocated memory it can grow as needed.
I have looked at chan_dahdi/sig_analog/sig_ss7/sig_pri, chan_iax2, chan_misdn, and chan_sip. These channel drivers all copy the data string before parsing it.
Stripping off const'ness is not automatically a bad thing.
A code improvement (for another patch) would be to make more of the tech callbacks specify const strings.</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;">I was going to suggest that the "correct" change is to make the data in ast_request a const char * as it should have been from the beginning. Of course, that sort of API change can't be made in Asterisk 1.8 or 10, so a note that strongly encourages people to treat the field as such would do well. For trunk, though, I like Richard's idea of improving APIs to reflect what they really should have.
Also, *BARF* at chan_phone. With or without Richard's change, it is making terrible assumptions about the length of the data passed in.</pre>
<br />
<p>- Mark</p>
<br />
<p>On January 25th, 2012, 10:43 a.m., rmudgett 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 Mark Michelson.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated Jan. 25, 2012, 10:43 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;">* Fix memory leak of vars in error paths for action_originate().
* Moved struct fast_originate_helper tech and data members to stringfields.
* Simplified ActionID header handling for fast_originate().</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>
<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/main/manager.c <span style="color: grey">(352510)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1690/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>