No subject
Fri Jun 28 13:27:35 CDT 2013
anti-pattern that doesn't solve any actual problem. It's akin to
repeatedly unlocking a recursive mutex "just to make sure it's
unlocked", which is a Bad Ideaâ„¢. Do you have a reference that shows
when the while() loop is a good thing?
The situation I ran into was dlclose() _never_ failed, resulting in
infinite loops. I saw this happen when a module links to a symbol
exported by another; even if the using module was first unloaded.
If dlclose() fails, ast_do_crash() seems a bit extreme. The module
will remain resident in memory, but is otherwise harmless. We could
log a message on failure, but since there's nothing to do about the
situation, I'm not sure what the point would be.
> On Aug. 27, 2013, 2:53 p.m., Tilghman Lesher wrote:
> > /branches/12/main/optional_api.c, lines 105-106
> > <https://reviewboard.asterisk.org/r/2797/diff/1/?file=45202#file45202line105>
> >
> > By convention, put the comment phrase "/* SAFE */" after the function, on the same line, so that when code auditors check for unsafe functions, they see the phrase and know that the use has been audited and can eliminate the instance from their report.
Sure, but of the 616 other strcpy()'s in Asterisk, only 27 follow this
convention.
> On Aug. 27, 2013, 2:53 p.m., Tilghman Lesher wrote:
> > /branches/12/include/asterisk/optional_api.h, line 154
> > <https://reviewboard.asterisk.org/r/2797/diff/1/?file=45199#file45199line154>
> >
> > You're using a new attribute, here, and yet you're not detecting whether the compiler supports it in the configure script?
Good catch. Thanks!
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2797/#review9528
-----------------------------------------------------------
On Aug. 27, 2013, 12:17 p.m., David Lee wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2797/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2013, 12:17 p.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-22296
> https://issues.asterisk.org/jira/browse/ASTERISK-22296
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> With the new work in Asterisk 12, there are some uses of the
> optional_api that are prone to failure. The details are rather involved,
> and captured on [the wiki][1].
>
> This patch addresses the issue by removing almost all of the magic from
> the optional API implementation. Instead of relying on weak symbol
> resolution, a new optional_api.c module was added to Asterisk core.
>
> For modules providing an optional API, the pointer to the implementation
> function is registered with the core. For modules that use an optional
> API, a pointer to a stub function, along with a optional_ref function
> pointer are registered with the core. The optional_ref function pointers
> is set to the implementation function when it's provided, or the stub
> function when it's now.
>
> Since the implementation no longer relies on magic, it is now supported
> on all platforms. In the spirit of choice, an OPTIONAL_API flag was
> added, so we can disable the optional_api if needed (maybe it's buggy on
> some bizarre platform I haven't tested on)
>
> The AST_OPTIONAL_API*() macros themselves remained unchanged, so
> existing code could remain unchanged. But to help with debugging the
> optional_api, the patch limits the #include of optional API's to just
> the modules using the API. This also reduces resource waste maintaining
> optional_ref pointers that aren't used.
>
> Other changes made as a part of this patch:
> * The stubs for http_websocket that wrap system calls set errno to
> ENOSYS.
>
> * res_http_websocket now properly increments module use count.
>
> * In loader.c, the while() wrappers around dlclose() were removed. The
> while(!dlclose()) is actually an anti-pattern, which can lead to
> infinite loops if the module you're attempting to unload exports a
> symbol that was directly linked to.
>
> * The special handling of nonoptreq on systems without weak symbol
> support was removed, since we no longer rely on weak symbols for
> optional_api.
>
> [1]: https://wiki.asterisk.org/wiki/x/wACUAQ
>
>
> Diffs
> -----
>
> /branches/12/tests/test_optional_api.c PRE-CREATION
> /branches/12/rest-api-templates/swagger_model.py 397673
> /branches/12/rest-api-templates/res_ari_resource.c.mustache 397673
> /branches/12/res/res_http_websocket.c 397673
> /branches/12/res/res_ari_events.c 397673
> /branches/12/res/res_ari.c 397673
> /branches/12/res/ari/internal.h 397673
> /branches/12/res/ari/ari_websockets.c 397673
> /branches/12/main/optional_api.c PRE-CREATION
> /branches/12/main/loader.c 397673
> /branches/12/main/asterisk.c 397673
> /branches/12/include/asterisk/optional_api.h 397673
> /branches/12/include/asterisk/http_websocket.h 397673
> /branches/12/include/asterisk/ari.h 397673
> /branches/12/channels/sip/include/sip.h 397673
> /branches/12/channels/chan_sip.c 397673
> /branches/12/build_tools/cflags.xml 397673
>
> Diff: https://reviewboard.asterisk.org/r/2797/diff/
>
>
> Testing
> -------
>
> New optional_api tests pass.
>
> The config below consistently fails when attempting to connect to the
> ARI WebSocket without this patch; consistently passes with this patch.
>
> modules.conf:
> [modules]
> load => res_stasis.so
> load => res_ari.so
> load => res_ari_model.so
> load => res_ari_events.so
> load => res_http_websocket.so
>
> http.conf:
> [general]
> enabled=yes
> bindaddr=127.0.0.1
>
> ari.conf:
> [general]
> enabled = yes
>
> [ari]
> type = user
> password = ari
>
>
> Thanks,
>
> David Lee
>
>
--===============0188111266255015812==
Content-Type: text/html; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
<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/2797/">https://reviewboard.asterisk.org/r/2797/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 27th, 2013, 2:53 p.m. CDT, <b>Tilghman Lesher</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/2797/diff/1/?file=45199#file45199line154" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/include/asterisk/optional_api.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">118</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">typedef</span> <span class="nf">void</span> <span class="p">(</span><span class="o">*</span><span class="n">ast_optional_fn</span><span class="p">)(</span><span class="kt">void</span><span class="p">)</span> <span class="n">__attribute__</span><span class="p">((</span><span class="n">__may_alias__</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;">You're using a new attribute, here, and yet you're not detecting whether the compiler supports it in the configure script?</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;">Good catch. Thanks!</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 27th, 2013, 2:53 p.m. CDT, <b>Tilghman Lesher</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/2797/diff/1/?file=45201#file45201line418" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/loader.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 unload_dynamic_module(struct ast_module *mod)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">415</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"><span class="tb"> </span><span class="tb"> </span></span><span class="k"><span class="hl">while</span></span><span class="hl"> </span><span class="p"><span class="hl">(</span></span><span class="o"><span class="hl">!</span></span><span class="n">dlclose</span><span class="p">(</span><span class="n">lib</span><span class="p">)<span class="hl">)</span>;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">418</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">dlclose</span><span class="p">(</span><span class="n">lib</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 know the while seems a little weird, but on some systems, dlclose() has to be run several times before it will succeed.
Otherwise, this would be a candidate for ast_do_crash() when dlclose() doesn't return 0.</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;">From what I've been able to find, the while(!dlclose()) is actually an
anti-pattern that doesn't solve any actual problem. It's akin to
repeatedly unlocking a recursive mutex "just to make sure it's
unlocked", which is a Bad Ideaâ„¢. Do you have a reference that shows
when the while() loop is a good thing?
The situation I ran into was dlclose() _never_ failed, resulting in
infinite loops. I saw this happen when a module links to a symbol
exported by another; even if the using module was first unloaded.
If dlclose() fails, ast_do_crash() seems a bit extreme. The module
will remain resident in memory, but is otherwise harmless. We could
log a message on failure, but since there's nothing to do about the
situation, I'm not sure what the point would be.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 27th, 2013, 2:53 p.m. CDT, <b>Tilghman Lesher</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/2797/diff/1/?file=45202#file45202line105" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/optional_api.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">105</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/* safe strcpy */</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">106</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">strcpy</span><span class="p">(</span><span class="n">user</span><span class="o">-></span><span class="n">module</span><span class="p">,</span> <span class="n">module</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;">By convention, put the comment phrase "/* SAFE */" after the function, on the same line, so that when code auditors check for unsafe functions, they see the phrase and know that the use has been audited and can eliminate the instance from their report.</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;">Sure, but of the 616 other strcpy()'s in Asterisk, only 27 follow this
convention.</pre>
<br />
<p>- David</p>
<br />
<p>On August 27th, 2013, 12:17 p.m. CDT, David Lee 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.</div>
<div>By David Lee.</div>
<p style="color: grey;"><i>Updated Aug. 27, 2013, 12:17 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-22296">ASTERISK-22296</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;">With the new work in Asterisk 12, there are some uses of the
optional_api that are prone to failure. The details are rather involved,
and captured on [the wiki][1].
This patch addresses the issue by removing almost all of the magic from
the optional API implementation. Instead of relying on weak symbol
resolution, a new optional_api.c module was added to Asterisk core.
For modules providing an optional API, the pointer to the implementation
function is registered with the core. For modules that use an optional
API, a pointer to a stub function, along with a optional_ref function
pointer are registered with the core. The optional_ref function pointers
is set to the implementation function when it's provided, or the stub
function when it's now.
Since the implementation no longer relies on magic, it is now supported
on all platforms. In the spirit of choice, an OPTIONAL_API flag was
added, so we can disable the optional_api if needed (maybe it's buggy on
some bizarre platform I haven't tested on)
The AST_OPTIONAL_API*() macros themselves remained unchanged, so
existing code could remain unchanged. But to help with debugging the
optional_api, the patch limits the #include of optional API's to just
the modules using the API. This also reduces resource waste maintaining
optional_ref pointers that aren't used.
Other changes made as a part of this patch:
* The stubs for http_websocket that wrap system calls set errno to
ENOSYS.
* res_http_websocket now properly increments module use count.
* In loader.c, the while() wrappers around dlclose() were removed. The
while(!dlclose()) is actually an anti-pattern, which can lead to
infinite loops if the module you're attempting to unload exports a
symbol that was directly linked to.
* The special handling of nonoptreq on systems without weak symbol
support was removed, since we no longer rely on weak symbols for
optional_api.
[1]: https://wiki.asterisk.org/wiki/x/wACUAQ
</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;">New optional_api tests pass.
The config below consistently fails when attempting to connect to the
ARI WebSocket without this patch; consistently passes with this patch.
modules.conf:
[modules]
load => res_stasis.so
load => res_ari.so
load => res_ari_model.so
load => res_ari_events.so
load => res_http_websocket.so
http.conf:
[general]
enabled=yes
bindaddr=127.0.0.1
ari.conf:
[general]
enabled = yes
[ari]
type = user
password = ari
</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/12/tests/test_optional_api.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/12/rest-api-templates/swagger_model.py <span style="color: grey">(397673)</span></li>
<li>/branches/12/rest-api-templates/res_ari_resource.c.mustache <span style="color: grey">(397673)</span></li>
<li>/branches/12/res/res_http_websocket.c <span style="color: grey">(397673)</span></li>
<li>/branches/12/res/res_ari_events.c <span style="color: grey">(397673)</span></li>
<li>/branches/12/res/res_ari.c <span style="color: grey">(397673)</span></li>
<li>/branches/12/res/ari/internal.h <span style="color: grey">(397673)</span></li>
<li>/branches/12/res/ari/ari_websockets.c <span style="color: grey">(397673)</span></li>
<li>/branches/12/main/optional_api.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/12/main/loader.c <span style="color: grey">(397673)</span></li>
<li>/branches/12/main/asterisk.c <span style="color: grey">(397673)</span></li>
<li>/branches/12/include/asterisk/optional_api.h <span style="color: grey">(397673)</span></li>
<li>/branches/12/include/asterisk/http_websocket.h <span style="color: grey">(397673)</span></li>
<li>/branches/12/include/asterisk/ari.h <span style="color: grey">(397673)</span></li>
<li>/branches/12/channels/sip/include/sip.h <span style="color: grey">(397673)</span></li>
<li>/branches/12/channels/chan_sip.c <span style="color: grey">(397673)</span></li>
<li>/branches/12/build_tools/cflags.xml <span style="color: grey">(397673)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2797/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============0188111266255015812==--
More information about the asterisk-dev
mailing list