<p>Patch set 6:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/10937">View Change</a></p><p>18 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c">File res/res_stasis.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/10937/6/res/res_stasis.c@1345">Patch Set #6, Line 1345:</a> <code style="font-family:monospace,monospace"> break;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ref leak on next_app</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1375">Patch Set #6, Line 1375:</a> <code style="font-family:monospace,monospace"> break;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">next_app being ref leaked here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1416">Patch Set #6, Line 1416:</a> <code style="font-family:monospace,monospace"> break;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">msg and next_app are ref leaked here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1419">Patch Set #6, Line 1419:</a> <code style="font-family:monospace,monospace"> if (r == 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think you need this check since if "r" is ever non-zero it breaks out of the loop above.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/res_stasis.c@1427">Patch Set #6, Line 1427:</a> <code style="font-family:monospace,monospace"> }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">next_app is being ref leaked</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h">File res/stasis/control.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/10937/6/res/stasis/control.h@40">Patch Set #6, Line 40:</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;"> *<br> * \note This function inherits app's ref rather than bumping app!<br> * If something goes wrong, ao2_cleanup is called on the app.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Any particular reason for this change? To me it breaks the usual expectations a bit as we typically don't ref steal in cases like this.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Usually a particular scope of code is responsible for its own incrementing/decrementing the ref.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h@119">Patch Set #6, Line 119:</a> <code style="font-family:monospace,monospace"> * \note This will unref the previous app by 1, and bump app by 1</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd add the word "control's" in this statement, so it is clear it's the control's app ref being removed. As in, "...control's previous app by 1, ...."</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h@123">Patch Set #6, Line 123:</a> <code style="font-family:monospace,monospace">char *control_next_app(struct stasis_app_control *control);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing documentation.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.h@132">Patch Set #6, Line 132:</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;"><br>char **control_argv(struct stasis_app_control *control);<br><br>int control_argc(struct stasis_app_control *control);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Both of these are also missing docs.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c">File res/stasis/control.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/10937/6/res/stasis/control.c@86">Patch Set #6, Line 86:</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;"> /*!<br> * The list of arguments to pass to StasisStart when moving to another app.<br> */<br> char **argv;<br> /*!<br> * The number of arguments argv contains.<br> */<br> int argc;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm thinking you can just use a string vector here instead. It might simplify some of the other code or make it easier to read.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@129">Patch Set #6, Line 129:</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;"> res = ast_cond_init(&control->wait_cond, NULL);<br> if (res != 0) {<br> ast_log(LOG_ERROR, "Error initializing ast_cond_t: %s\n",<br> strerror(errno));<br> ao2_cleanup(app);<br> ao2_ref(control, -1);<br> return NULL;<br> }<br><br> control->app = app;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not a fan of stealing the ref here, but if you do it this way you could move this just before the condition init, so if it fails the control unref will clean it up without having to call ao2_cleanup directly here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@541">Patch Set #6, Line 541:</a> <code style="font-family:monospace,monospace"> if (!(move_data = ast_calloc(1, sizeof(*move_data)))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The move_data structure eventually gets free'd by the ast_free_ptr callback, but some of the objects it "owns" do not like app_name and argv. See below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@546">Patch Set #6, Line 546:</a> <code style="font-family:monospace,monospace"> if (!(move_data->app_name = ast_calloc(1, size))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">app_name is leaked. At least I did not see where it was ever free'd</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@554">Patch Set #6, Line 554:</a> <code style="font-family:monospace,monospace"> if (!(iter = ast_calloc(1, size))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It doesn't appear "iter" is free'd. You could probably just use a stack variable here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@560">Patch Set #6, Line 560:</a> <code style="font-family:monospace,monospace"> if (!(move_data->argv = ast_calloc(1, sizeof(*move_data->argv)))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is never free'd</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@564">Patch Set #6, Line 564:</a> <code style="font-family:monospace,monospace"> while ((token = strtok_r(iter, ",", &iter))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not 100% sure, but I don't think you want strtok_r's saveptr (last parameter) to be the same as the first parameter. Although it might not be a problem here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@569">Patch Set #6, Line 569:</a> <code style="font-family:monospace,monospace"> move_data->argv[argc] = ast_calloc(1, size);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Each element is leaked as it is copied again later and never free'd</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10937/6/res/stasis/control.c@1413">Patch Set #6, Line 1413:</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 (control->next_app) {<br> ast_free(control->next_app);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can just use ao2_cleanup, or move the setting to NULL under here.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/10937">change 10937</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/10937"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I43d12b10045a98a8d42541889b85695be26f288a </div>
<div style="display:none"> Gerrit-Change-Number: 10937 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua C. Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 05 Feb 2019 21:27:27 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>