<p> Attention is currently required from: N A, Richard Mudgett. </p>
<p>Patch set 5:<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/c/asterisk/+/16231">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File funcs/func_scramble.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/asterisk/+/16231/comment/a6294142_50fac772">Patch Set #5, Line 205:</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;">      } else if (!strcasecmp(args.direction, "remove")) {<br>         return remove_scrambler(chan);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Check this case first prior even to the datastore find above. Otherwise there is a datastore leak here when removing on first call.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16231/comment/b34975cb_8fbe69aa">Patch Set #5, Line 208:</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_log(LOG_ERROR, "Direction must be either RX, TX or remove\n");<br>          return -1;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">datastore can be leaked here if created, but not added.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16231/comment/f6df5bd1_e84b7e50">Patch Set #5, Line 212:</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 (is_new) {<br>         datastore->data = ni;<br>              ast_channel_lock(chan);<br>               ast_channel_datastore_add(chan, datastore);<br>           ast_channel_unlock(chan);<br>             ast_audiohook_attach(chan, &ni->audiohook);<br>    }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it'd read easier if you moved this to right after you create the datastore above and drop the "is_new".</p><p style="white-space: pre-wrap; word-wrap: break-word;">Doing so would also alleviate a possible threading & datastore creating/add issue here too. As you have it now if two threads using the same channel enter this function then both could create and add the datastore. You probably want to create/add within the same "lock" scope.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16231">change 16231</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/c/asterisk/+/16231"/><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-Change-Id: I01020769d91060a1f56a708eb405f87648d1a67e </div>
<div style="display:none"> Gerrit-Change-Number: 16231 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </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-Attention: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Attention: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 09 Aug 2021 22:49:25 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>