<p> Attention is currently required from: Jaco Kroon. </p>
<p>Patch set 1:<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/+/18689">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">Commit Message:</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/+/18689/comment/7c8bb2b1_9b87e716">Patch Set #1, Line 16:</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>This fixes my immediate issue, however may re-introduce the issue. Is<br>there a reliable test case for the crash which can be used to validate<br>that this doesn't re-introduce the problem?<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Unfortunately I don't think there is a testsuite test for this. It's been a while, so can't fully remember but you might just be able to add an unsupported format like vp9 to the codecs allow list, and then test through an IAX loopback configuration setup. For example,</p><p style="white-space: pre-wrap; word-wrap: break-word;">iax.conf:<br>[loopback]<br>type=peer<br>host=127.0.0.1<br>username=loopback-inbound<br>secret=0000</p><p style="white-space: pre-wrap; word-wrap: break-word;">[loopback-inbound]<br>type=user<br>context=loopback<br>secret=0000</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">extensions.conf:<br>[default]<br>exten => loopback,1,NoOp()<br> same => n,Answer()<br> same => n,Dial(IAX2/${EXTEN})<br> same => n,Hangup()</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">[loopback]<br>exten => s,1,NoOp()<br> same => n,Echo()<br> same => n,Hangup()</pre><p style="white-space: pre-wrap; word-wrap: break-word;">pjsip.conf:<br>Define your pjsip endpoint with vp9</p><p style="white-space: pre-wrap; word-wrap: break-word;">Then place a call from your pjsip endpoint to the loopback extension.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File channels/chan_iax2.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/+/18689/comment/df7ad139_a1d55df1">Patch Set #1, Line 4152:</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 (ms >= (next = jb_next(pvt->jb))) {<br> voicefmt = ast_format_compatibility_bitfield2format(pvt->voiceformat);<br> ret = jb_get(pvt->jb, &frame, ms, voicefmt ? ast_format_get_default_ms(voicefmt) : 20);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I believe this was initially changed to bypass having to do multiple NULL checks on voicefmt, and possible side effects of a NULL subclass.format.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So yes I think this reversion is "okay" for this check, but 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/asterisk/+/18689/comment/fa0f32b8_719fd18c">Patch Set #1, Line 4168:</a> <code style="font-family:monospace,monospace"> af.subclass.format = voicefmt;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if af.subclass.format == NULL is it possible for this to cause a crash later someplace else in the Asterisk code (e.g. any current or future module that might attempt to reference it).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe a better way to ask it is, is it okay for the subclass.format on a frame to be NULL? I'm not sure.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18689/comment/a3ec13a3_7c49f3ec">Patch Set #1, Line 4169:</a> <code style="font-family:monospace,monospace"> af.samples = frame.ms * (ast_format_get_sample_rate(voicefmt) / 1000);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">With the above revert this will also require a NULL check and a default used.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18689">change 18689</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/+/18689"/><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: I80f9c1af4b2cd8281dbd74cb7cbc01b8deccdf96 </div>
<div style="display:none"> Gerrit-Change-Number: 18689 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Jaco Kroon <jaco@uls.co.za> </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-Attention: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 01 Jul 2022 18:05:57 +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>