<p> Attention is currently required from: Joshua Colp, Mark Murawski. </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/85d31de8_674439d3">Patch Set #4, Line 160:</a> <code style="font-family:monospace,monospace"> }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This hasn't called ast_mutex_destroy</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/291158be_611c08a1">Patch Set #4, Line 171:</a> <code style="font-family:monospace,monospace">                ast_mutex_lock(&s->lock);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why lock and then unlock?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/72629d62_74298b26">Patch Set #4, Line 278:</a> <code style="font-family:monospace,monospace">                remove_signal(signame);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This has a race condition where another thread could be finding this signal, while it is being remov […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this could be fixed by simply calling remove_signal before ast_mutex_unlock, i.e. moving it inside the lock. Given that ast_mutex_destroy is called though, is it problematic to destroy objects with locks held? If not, then this would be the cleanest way to do it I think.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Alternately, s->watchers = s->watchers - 1; is executed with the lock held. So as soon as the signal is unlocked, if it's about to be destroyed, s->watchers == 0.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So on both the wait and send side, we could also check for 0 and recognize it's about to be destroyed and just bail early in that case.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Alternately, I could WRLOCK the list right before unlocking the signal, which should just guarantee the signal will be destroyed before anyone else can grab it. I'm thinking that may be the cleanest and simplest option. This is the one I've taken in the updated review.</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/122e476d_6907a855">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What does this actually mean for a user? You've removed all of the signals from the list, so will th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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: 5 </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: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 22 Dec 2022 12:47:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>