<p>Patch set 6:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10227">View Change</a></p><p>20 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/CHANGES">File CHANGES:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/CHANGES@15">Patch Set #6, Line 15:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Bridging<br>------------------<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This change isn't quite in the format of the rest of the file.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/UPGRADE.txt">File UPGRADE.txt:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/UPGRADE.txt@30">Patch Set #6, Line 30:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Bridging<br>------------------<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This change isn't quite in the format of the rest of the other UPGRADE files.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/include/asterisk/bridge.h">File include/asterisk/bridge.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/include/asterisk/bridge.h@77">Patch Set #6, Line 77:</a> <code style="font-family:monospace,monospace">#include "asterisk/astobj2.h"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since the only thing this header file needs from astobj2.h is the ao2_container struct name, this include could be replaced by:</p><p style="white-space: pre-wrap; word-wrap: break-word;">struct ao2_container;</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/main/bridge.c">File main/bridge.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/bridge.c@5068">Patch Set #6, Line 5068:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">                ast_cli(a->fd, FORMAT_ROW,<br>                 bridge->uniqueid,<br>                  bridge->num_channels,<br>                      S_OR(bridge->v_table->name, "<unknown>"),<br>                       S_OR(bridge->technology->name, "<unknown>"));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Get the latest snapshot on the bridge to display this information as it is immutable.  ast_bridge_get_snapshot() and check for NULL return.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Accessing things in bridge without locking it is dangerous.  The worst one is getting the bridge->technology->name as that is most likely to change while being accessed.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c">File main/manager_bridges.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@451">Patch Set #6, Line 451:</a> <code style="font-family:monospace,monospace">  struct ast_bridge_snapshot *snapshot = ast_bridge_get_snapshot(bridge);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It is possible for snapshot to be NULL</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@454">Patch Set #6, Line 454:</a> <code style="font-family:monospace,monospace">      RAII_VAR(struct ast_str *, bridge_info, ast_manager_build_bridge_state_string(snapshot), ast_free);</code></p><ul><li>Since the bridge filtering is done in this function, you shouldn't generate the bridge_info string before the bridge is filtered.</li></ul><ul><li>This RAII_VAR can be eliminated after the bridge is filtered.</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@458">Patch Set #6, Line 458:</a> <code style="font-family:monospace,monospace">     ast_log(LOG_ERROR, "f: %s br: %s %s\n", S_OR(list_data->type_filter, "none"), bridge->uniqueid, bridge->technology->name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This ERROR message appears to be left over debugging.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@460">Patch Set #6, Line 460:</a> <code style="font-family:monospace,monospace">    if (!ast_strlen_zero(list_data->type_filter) && strcmp(list_data->type_filter, bridge->technology->name))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use the bridge snapshot not the live bridge object to check the technology.  You could crash without locking the bridge.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@460">Patch Set #6, Line 460:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (!ast_strlen_zero(list_data->type_filter) && strcmp(list_data->type_filter, bridge->technology->name))<br> {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">guidelines: curly<br>guidelines: wrap long line</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@489">Patch Set #6, Line 489:</a> <code style="font-family:monospace,monospace">         list_data.id_text = ast_alloca(strlen(id) + strlen("ActionID: \r\n") + 1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This opens us up to a security issue.  Allocating from the stack a string that comes from the outside world could blow the stack if an attacker sent a very very long string.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@502">Patch Set #6, Line 502:</a> <code style="font-family:monospace,monospace">          list_data.type_filter = ast_strdupa(type_filter);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why allocate this from the stack?  Just put the pointer into list_data.type_filter.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/manager_bridges.c@575">Patch Set #6, Line 575:</a> <code style="font-family:monospace,monospace">       snapshot = ast_bridge_get_snapshot_by_uniqueid(bridge_uniqueid);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We may not find the snapshot if the bridge no longer exists.<br>Check for NULL.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c">File main/stasis_bridges.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1323">Patch Set #6, Line 1323:</a> <code style="font-family:monospace,monospace">  ao2_lock(bridge);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use ast_bridge_lock/ast_bridge_unlock instead</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1324">Patch Set #6, Line 1324:</a> <code style="font-family:monospace,monospace">    snapshot = bridge->current_snapshot;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">bump snapshot here under lock protection</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1338">Patch Set #6, Line 1338:</a> <code style="font-family:monospace,monospace">   ao2_lock(bridge);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use ast_bridge_lock/ast_bridge_unlock instead</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/main/stasis_bridges.c@1339">Patch Set #6, Line 1339:</a> <code style="font-family:monospace,monospace">    snapshot = bridge->current_snapshot;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">bump snapshot under lock protection</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/res/ari/resource_bridges.c">File res/ari/resource_bridges.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/res/ari/resource_bridges.c@907">Patch Set #6, Line 907:</a> <code style="font-family:monospace,monospace">                struct ast_bridge_snapshot *snapshot = ast_bridge_get_snapshot(bridge);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to protect from a NULL snapshot</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/res/stasis/app.c">File res/stasis/app.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/res/stasis/app.c@183">Patch Set #6, Line 183:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">        if (!forwards->topic_forward && bridge) {<br>          forwards_unsubscribe(forwards);<br>               ao2_ref(forwards, -1);<br>                return NULL;<br>  }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missed this change:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!forwards->topic_forward) {<br>    ao2_ref(forwards, -1);<br>        return NULL;<br>}</pre></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10227/6/tests/test_cel.c">File tests/test_cel.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/tests/test_cel.c">Patch Set #6:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">The leaks fixed in this file should be put into their own patch as they apply to v13 too.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10227/6/tests/test_cel.c@2143">Patch Set #6, Line 2143:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ao2_cleanup(cel_expected_events);<br>     ao2_cleanup(cel_received_events);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should set these containers to NULL to better ensure that the unit test can be run again.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10227">change 10227</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/10227"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I7049b80efa88676ce5c4666f818fa18ad1985369 </div>
<div style="display:none"> Gerrit-Change-Number: 10227 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 19 Oct 2018 18:51:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>