<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/2496/">https://reviewboard.asterisk.org/r/2496/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 6th, 2013, 2:13 p.m. UTC, <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/2496/diff/1/?file=37181#file37181line535" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/security_events.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 encode_timestamp(struct ast_str **str, const struct timeval *tv)</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">509</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">return</span> <span class="n">json_string</span> <span class="o">?</span> <span class="n">ast_json_object_set</span><span class="p">(</span><span class="n">json</span><span class="p">,</span> <span class="n">ast_event_get_ie_type_name</span><span class="p">(</span><span class="n">ie_type</span><span class="p">),</span> <span class="n">ast_json_ref</span><span class="p">(</span><span class="n">json_string</span><span class="p">))</span> <span class="o">:</span> <span class="o">-</span><span class="mi">1</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;">Don't combine the ref bump of the JSON string with setting the JSON object. If setting the JSON object fails, you have a ref leak.
You should only ref bump the string if everything succeeds.</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;">See below regarding when ast_json_object_set fails.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 6th, 2013, 2:13 p.m. UTC, <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/2496/diff/1/?file=37181#file37181line568" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/security_events.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int add_ip_ie(struct ast_event **event, enum ast_event_ie_type ie_type,</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int add_ip_json_object(struct ast_json *json, enum ast_event_ie_type ie_type,</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">542</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_json_ref</span><span class="p">(</span><span class="n">json_string</span><span class="p">);</span></pre></td>
</tr>
<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">543</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">res</span> <span class="o">=</span> <span class="n">ast_json_object_set</span><span class="p">(</span><span class="n">json</span><span class="p">,</span> <span class="n">ast_event_get_ie_type_name</span><span class="p">(</span><span class="n">ie_type</span><span class="p">),</span> <span class="n">json_string</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;">Don't ref bump the string if setting the JSON object fails. Ref bump it just prior to returning success.</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 actually asked David about this function (ast_json_object_set) as I was writing these. The documentation describes it with the following:
* The object steals the \a value reference; use ast_json_ref() to safely keep a pointer
* to it.
Specifically, what I asked was whether or not the reference would still be stolen if json_object_set fails. His response was that he believes this to be the case, and if not that we should adjust ast_json_object_set so that if json_object_set fails and it doesn't take the reference that we take the reference away anyway.
Anyway, what I'm getting is that for the json_string here, bumping the ref count after doing ast_json_object_set strikes me as technically incorrect. Yes, the reference should still be valid as long as ast_json_object_set succeeded, but it isn't valid through the json_string pointer.
In truth though, the way I'm using ast_json_object_set around here, it just isn't necessary to hold onto that reference anyway. My line of thinking here was that I should keep a reference for when the function exits because of RAII_VAR, but according to David json_decref is tolerant to NULL pointers anyway.
I'm doing away with RAII_VAR for the json_strings since their only purpose ever is to be created and then immediately shoved in the json object. There are only two sides to the operation. If it fails on the string creation side, we didn't get a reference to the string so there is no need to unref. If it fails on the object set side then the reference was already taken.
I'll add a short note about that in comments for the more elaborate function with multiple sets since it isn't explicitly clear.</pre>
<br />
<p>- jrose</p>
<br />
<p>On May 3rd, 2013, 7:42 p.m. UTC, jrose 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, David Lee, kmoore, and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated May 3, 2013, 7:42 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/ASTERISK-21103">ASTERISK-21103</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;">Stage 3/3 of ASTERISK-21103. In order to convert this set of messages I had to change all of the event blob stuff into JSON strings inside of a JSON blob loaded onto a JSON payload and sent out over stasis to the security topic where it is then consumed by stasis and read into a log message in the appropriate fashion with the fields in the right order to catch the bird to catch the spider to catch the fly.</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;">Made sure that the security messages were being generated in the same way as they were prior to the stasis change by using the following CLI command:
securityevents test generation
Examples Before:
[May 3 14:40:28] SECURITY[1305]: res_security_log.c:134 security_event_cb: SecurityEvent="FailedACL",EventTV="1367610028-869814",Severity="Error",Service="TEST",EventVersion="1",AccountID="Username",SessionID="Session123",LocalAddress="IPV4/UDP/192.168.1.1/12121",RemoteAddress="IPV4/UDP/192.168.1.2/12345",Module="test_security_events",ACLName="TEST_ACL",SessionTV="1367610028-869754"
[May 3 14:40:28] SECURITY[1305]: res_security_log.c:134 security_event_cb: SecurityEvent="InvalidAccountID",EventTV="1367610028-869895",Severity="Error",Service="TEST",EventVersion="1",AccountID="FakeUser",SessionID="Session456",LocalAddress="IPV4/TCP/10.1.2.3/4321",RemoteAddress="IPV4/TCP/10.1.2.4/123",Module="test_security_events",SessionTV="1367610028-869854"
[May 3 14:40:28] SECURITY[1305]: res_security_log.c:134 security_event_cb: SecurityEvent="SessionLimit",EventTV="1367610028-869960",Severity="Error",Service="TEST",EventVersion="1",AccountID="Jenny",SessionID="8675309",LocalAddress="IPV4/TLS/10.5.4.3/4444",RemoteAddress="IPV4/TLS/10.5.4.2/3333",Module="test_security_events",SessionTV="1367610028-869923"
Examples After:
[May 3 14:33:35] SECURITY[31607]: res_security_log.c:122 security_event_stasis_cb: SecurityEvent="FailedACL",EventTV="1367609615-957822",Severity="Error",Service="TEST",EventVersion="1",AccountID="Username",SessionID="Session123",LocalAddress="IPV4/UDP/192.168.1.1/12121",RemoteAddress="IPV4/UDP/192.168.1.2/12345",Module="test_security_events",ACLName="TEST_ACL",SessionTV="1367609615-957750"
[May 3 14:33:35] SECURITY[31608]: res_security_log.c:122 security_event_stasis_cb: SecurityEvent="InvalidAccountID",EventTV="1367609615-958101",Severity="Error",Service="TEST",EventVersion="1",AccountID="FakeUser",SessionID="Session456",LocalAddress="IPV4/TCP/10.1.2.3/4321",RemoteAddress="IPV4/TCP/10.1.2.4/123",Module="test_security_events",SessionTV="1367609615-957969"
[May 3 14:33:35] SECURITY[31608]: res_security_log.c:122 security_event_stasis_cb: SecurityEvent="SessionLimit",EventTV="1367609615-958404",Severity="Error",Service="TEST",EventVersion="1",AccountID="Jenny",SessionID="8675309",LocalAddress="IPV4/TLS/10.5.4.3/4444",RemoteAddress="IPV4/TLS/10.5.4.2/3333",Module="test_security_events",SessionTV="1367609615-958360"
As you can see it's basically identical aside from things that are expected to change between runs and the name of the function generating the messages.</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>/trunk/channels/chan_iax2.c <span style="color: grey">(387594)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(387594)</span></li>
<li>/trunk/include/asterisk/acl.h <span style="color: grey">(387594)</span></li>
<li>/trunk/include/asterisk/security_events.h <span style="color: grey">(387594)</span></li>
<li>/trunk/main/asterisk.c <span style="color: grey">(387594)</span></li>
<li>/trunk/main/manager.c <span style="color: grey">(387594)</span></li>
<li>/trunk/main/named_acl.c <span style="color: grey">(387594)</span></li>
<li>/trunk/main/security_events.c <span style="color: grey">(387594)</span></li>
<li>/trunk/res/res_security_log.c <span style="color: grey">(387594)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2496/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>