<p> Attention is currently required from: N A, Mark Murawski. </p>
<p>Patch set 4:<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/+/17786">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File apps/app_signal.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/+/17786/comment/900e79f3_24009fef">Patch Set #4, Line 160:</a> <code style="font-family:monospace,monospace">       }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This hasn't called ast_mutex_destroy</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17786/comment/b56d8018_c6639b90">Patch Set #4, Line 171:</a> <code style="font-family:monospace,monospace">            ast_mutex_lock(&s->lock);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why lock and then unlock?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17786/comment/5e352a34_db01c517">Patch Set #4, Line 278:</a> <code style="font-family:monospace,monospace">            remove_signal(signame);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This has a race condition where another thread could be finding this signal, while it is being removed. The signals list is not locked during this. Due to the fact that watchers is also not touched as part of the get process, that other thread could end up using freed memory.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17786/comment/e5c363e0_c79522bb">Patch Set #4, Line 422:</a> <code style="font-family:monospace,monospace">          ast_log(LOG_WARNING, "One or more signals is currently in use. Unload failed.\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What does this actually mean for a user? You've removed all of the signals from the list, so will those just go away - will they stick around?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/17786">change 17786</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/+/17786"/><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: Ic34439de3d60f8609357666a465c354d81f5fef3 </div>
<div style="display:none"> Gerrit-Change-Number: 17786 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Reviewer: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Attention: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Attention: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 22 Dec 2022 10:25:59 +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>