[Asterisk-code-review] chan_iax2: permit frames without voice format. (asterisk[master])

[Kevin Harwell]

Attention is currently required from: Jaco Kroon.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18689 )

Change subject: chan_iax2: permit frames without voice format.
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18689/comment/7c8bb2b1_9b87e716 
PS1, Line 16: 
            : This fixes my immediate issue, however may re-introduce the issue.  Is
            : there a reliable test case for the crash which can be used to validate
            : that this doesn't re-introduce the problem?
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,

iax.conf:
[loopback]
type=peer
host=127.0.0.1
username=loopback-inbound
secret=0000

[loopback-inbound]
type=user
context=loopback
secret=0000

extensions.conf:
[default]
exten => loopback,1,NoOp()
	same => n,Answer()
	same => n,Dial(IAX2/${EXTEN})
	same => n,Hangup()

[loopback]
exten => s,1,NoOp()
	same => n,Echo()
	same => n,Hangup()

pjsip.conf:
Define your pjsip endpoint with vp9

Then place a call from your pjsip endpoint to the loopback extension.


File channels/chan_iax2.c:

https://gerrit.asterisk.org/c/asterisk/+/18689/comment/df7ad139_a1d55df1 
PS1, Line 4152: 	if (ms >= (next = jb_next(pvt->jb))) {
              : 		voicefmt = ast_format_compatibility_bitfield2format(pvt->voiceformat);
              : 		ret = jb_get(pvt->jb, &frame, ms, voicefmt ? ast_format_get_default_ms(voicefmt) : 20);
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.

So yes I think this reversion is "okay" for this check, but see below...


https://gerrit.asterisk.org/c/asterisk/+/18689/comment/fa0f32b8_719fd18c 
PS1, Line 4168: 			af.subclass.format = voicefmt;
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).

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.


https://gerrit.asterisk.org/c/asterisk/+/18689/comment/a3ec13a3_7c49f3ec 
PS1, Line 4169: 			af.samples  = frame.ms * (ast_format_get_sample_rate(voicefmt) / 1000);
With the above revert this will also require a NULL check and a default used.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18689
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I80f9c1af4b2cd8281dbd74cb7cbc01b8deccdf96
Gerrit-Change-Number: 18689
Gerrit-PatchSet: 1
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Jaco Kroon <jaco at uls.co.za>
Gerrit-Comment-Date: Fri, 01 Jul 2022 18:05:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220701/cb908340/attachment.html>