<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/2930/">https://reviewboard.asterisk.org/r/2930/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 22nd, 2013, 3:10 a.m. CDT, <b>Guenther Kelleter</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;">Scott,
all I can offer you is to test this patch if it fixes the original issue as I don't understand how this context extension tree works. So if you introduced a new bug in the code I won't see it.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So I've duplicated your results exactly (tested your code, then checked my slightly different approach). The only concern I have, not being 100% familiar with the protocol spec, is whether the test string being sent ended in 5 or 4. The length seems to indicate a byte less than what would include the last 2-byte value, thus ending in 5, but according to your patch the last 4 would also be included "..54". I've got the patch ready to go, but was hoping you could recall what the last digit of the original text message transmitted through the other carrier was so that we could confirm the correct message decoding length. The patch eliminates the possibility of a crash, and it's easy to adjust it to include or not that last byte.
If you don't recall that's fine, we'll go ahead with this version and it can be corrected later if the last byte should not be included in the message text after conversion.
Thanks!
</pre>
<br />
<p>- Scott</p>
<br />
<p>On October 21st, 2013, 4:18 p.m. CDT, Scott Griepentrog wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/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 Guenther Kelleter.</div>
<div>By Scott Griepentrog.</div>
<p style="color: grey;"><i>Updated Oct. 21, 2013, 4:18 p.m.</i></p>
<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/AST-1235">AST-1235</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">This patch fixes a bug report where restarting Asterisk repeatedly would cause a crash due to accidentally deleting extensions that were still referenced in the context's root_table. This occurred due to confusion over matching or not matching extensions based on callerid, which is also being cleared up via an enum'd third state.
There are two values in an extension entry: matchcid which says whether cid should be matched, and cidmatch which is the string value to match. The matchcid value was previously non-zero when cidmatch was signficant. This is important since a null length callerid match must be kept distinct from a match-anything condition.
This patch adds an enum to define the AST_EXT_MATCHCID_OFF (0) or AST_EXT_MATCHCID_ON (1) behaviors, and adds AST_EXT_MATCHCID_ANY (2) which allows matching to both exten->matchcid=0 and exten->matchcid=1 cases. For example, all exten's with the same exten (number) can be deleted with ANY, but when looking for a dialplan match it must match to matchcid, and if set to ON, further match cidmatch.
The logic to match extensions has been reworked to implement these three types, and then matchcid values updated to use the enum for clarity.
The most significant change is in ast_context_remove_extension_callerid2() where after removing extensions from the hashtable, the deletion would destroy the extensions. This was deleting exten's that didn't have matchcid set but matched a null cidmatch, where they hadn't been removed from the hashtable.
- (!callerid || !matchcallerid || (matchcallerid && !strcmp(peer->cidmatch, callerid))) &&
+ (!callerid || (!matchcallerid && !peer->matchcid) || (matchcallerid && peer->matchcid && !strcmp(peer->cidmatch, callerid))) &&
</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;">This patch has been tested with the testsuite additions dialplan_reload and pbx/callerid_match (posted separately) to insure that the crash case has been resolved without any other unwanted changes in behavior.
</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/include/asterisk/pbx.h <span style="color: grey">(400531)</span></li>
<li>/branches/1.8/main/pbx.c <span style="color: grey">(400531)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2930/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>