<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/3377/">https://reviewboard.asterisk.org/r/3377/</a>
</td>
</tr>
</table>
<br />
<div>
<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/3377/diff/3/?file=56813#file56813line260" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/include/asterisk/astobj2.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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; ">all the objects, or you can use "util/refcounter" to scan the file</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">and run Asterisk, exit asterisk with "stop gracefully", which should</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">251</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm">tag to each call. Recompile, and run Asterisk, exit asterisk with</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">259</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm">and run Asterisk, exit asterisk with "stop gracefully", which should</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Since you're editing this let's specify the real command "core stop gracefully" instead of the alias.</pre>
</div>
<br />
<div>
<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/3377/diff/3/?file=56815#file56815line35" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/astobj2.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<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">35</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">FILE</span> <span class="o">*</span><span class="n">ref_log</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">#ifdef REF_DEBUG
To take this suggestion we would need to put all "if (ref_log) {}" blocks in #ifdef sections. I believe this would be correct, as those blocks of code are unreachable if REF_DEBUG isn't defined during compile of astobj2.c (ref_log cannot be initialized). Previously this wasn't the case since modules could have REF_DEBUG enabled without astobj2.c enabling it.
We also need to declare static FILE *ref_log = NULL, otherwise it can be used uninitialized.</pre>
</div>
<br />
<div>
<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/3377/diff/3/?file=56815#file56815line1141" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/astobj2.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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 struct ast_cli_entry cli_astobj2[] = {</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">1132</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">fclose</span><span class="p">(</span><span class="n">ref_log</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">1133</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ref_log</span> <span class="o">=</span> <span class="nb">NULL</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think this can cause a race, especially during non-graceful shutdowns. This should only be possible when someone enables REF_DEBUG and shuts down with a kill signal or "core stop now", but it could also happen if "core stop gracefully" fails to stop an ao2 using thread.
>From the view of other threads that are not running astobj2_cleanup, ref_log is closed but not NULL for a brief moment. if (ref_log) { fprintf(ref_log, ...); } races against this. This could cause fprintf(ref_log, ...) to be called with ref_log==NULL, or with ref_log pointing to an already closed FILE*.
I don't think it makes a difference when we fclose ref_log. Not with Asterisk, but I've seen leaked threads cause a segfault while the main thread is running __attribute__((destructor)) functions.</pre>
</div>
<br />
<div>
<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/3377/diff/3/?file=56815#file56815line1156" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/astobj2.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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 struct ast_cli_entry cli_astobj2[] = {</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">1147</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_ERROR</span><span class="p">,</span> <span class="s">"Could not open ref debug log file: %s</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">ref_filename</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">logger.c is not initialized yet. What fun start-up ordering is!</pre>
</div>
<br />
<div>
<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/3377/diff/3/?file=56815#file56815line1162" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/astobj2.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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 struct ast_cli_entry cli_astobj2[] = {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1142</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_register_atexit</span><span class="p">(</span><span class="n">astobj2_cleanup</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1153</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_register_atexit</span><span class="p">(</span><span class="n">astobj2_cleanup</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is needed when defined(REF_DEBUG) || defined(AO2_DEBUG). I'm not against registering astobj2_cleanup unconditionally, and just having the procedure do nothing when no debug modes are enabled.</pre>
</div>
<br />
<p>- Corey Farrell</p>
<br />
<p>On March 27th, 2014, 5:08 p.m. EDT, Matt Jordan 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Corey Farrell and wdoekes.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated March 27, 2014, 5:08 p.m.</i></p>
<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;">Note: while an improvement to Asterisk, this patch only affects Asterisk when compiled in -dev-mode. Since it has benefit only for developers looking to fix bugs, I'm proposing it for Asterisk 1.8+. The removal of refcounter should be done in trunk only.
Asterisk uses reference counted objects. A lot. As their use has spread, the bugs related to reference counting errors has grown as well. Central to resolving reference counting issues is the usage of REF_DEBUG; unfortunately, REF_DEBUG has had several problems:
(1) It could not be enabled through menuselect
(2) When not enabled globally, updates to objects were often lost, resulting in insufficient data to resolve problems
(3) The format of the ref debug log file was not suitable for parsing
This patch changes REF_DEBUG in the following ways:
(1) It makes REF_DEBUG a meneselect item when Asterisk is compiled with --enable-dev-mode
(2) The ref debug log file is now created in the AST_LOG_DIR directory. Every run will now blow away the previous run (as large ref files sometimes caused issues). We now also no longer open/close the file on each write, instead relying on fflush to make sure data gets written to the file (in case the ao2 call being performed is about to cause a crash)
(3) It goes with a comma delineated format for the ref debug file. This makes parsing much easier.
(4) It throws out the refcounter utility and goes with a python script instead.</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;">Things spit out ref logs and the script parses them. Example below:
==== Object 0x21ed9b8 history ====
features.c[5427]:create_parkinglot 1 - [**constructor**]
features.c[5707]:build_parkinglot +1 - [1]
features.c[5392]:parkinglot_unref -1 - [2]
features.c[6358]:build_dialplan_useage_map +1 - [1]
features.c[6358]:build_dialplan_useage_map -1 - [2]
features.c[4985]:find_parkinglot +1 - [1]
features.c[5392]:parkinglot_unref -1 - [2]
features.c[6358]:build_dialplan_useage_map +1 - [1]
features.c[6358]:build_dialplan_useage_map -1 - [2]
features.c[4955]:do_parking_thread +1 - [1]
features.c[4957]:do_parking_thread -1 - [2]
astobj2.c[955]:cd_cb_debug -1 deref object via container destroy - [1]
astobj2.c[955]:cd_cb_debug -1 deref object via container destroy - [**call destructor**]
</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/utils/refcounter.c <span style="color: grey">(410891)</span></li>
<li>/branches/1.8/utils/Makefile <span style="color: grey">(410891)</span></li>
<li>/branches/1.8/main/astobj2.c <span style="color: grey">(410891)</span></li>
<li>/branches/1.8/main/asterisk.c <span style="color: grey">(410891)</span></li>
<li>/branches/1.8/include/asterisk/astobj2.h <span style="color: grey">(410891)</span></li>
<li>/branches/1.8/contrib/scripts/refcounter.py <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(410891)</span></li>
<li>/branches/1.8/build_tools/cflags.xml <span style="color: grey">(410891)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3377/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>