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

[Jaco Kroon]

Attention is currently required from: Sean Bright, Kevin Harwell.

Jaco Kroon 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:

(5 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18689/comment/d91df499_6462e80b 
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. […]
This patch only affects frames going through the JB, and the JB can only create extra frames from JB_INTERP, which (as far as I can determine) can ONLY happen once voicefmt has been established.  So if this issue is also fixed for the non-JB case (which it should be), then we are having the wrong discussion.  If it's not fixed, we need to address subclass.format being NULL in those locations where they could cause problems.


Patchset:

PS1: 
> Lack of indications. […]
Done


File channels/chan_iax2.c:

https://gerrit.asterisk.org/c/asterisk/+/18689/comment/b606d097_d7a8c376 
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 po […]
Done


https://gerrit.asterisk.org/c/asterisk/+/18689/comment/1fadca7e_88f11fea 
PS1, Line 4168: 			af.subclass.format = voicefmt;
> if af.subclass. […]
Looking at the JB code, it's impossible for voicefmt to be NULL here as we can only get JB_INTERP frames once we've had at least one voice frame, at which point voicefmt will be set to a non-NULL value since the voice format will have been negotiated.


https://gerrit.asterisk.org/c/asterisk/+/18689/comment/2f99b24a_a1bf4f5c 
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.
See above explanation.  I don't think this is the case.



-- 
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 <default.enum at gmail.com>
Gerrit-CC: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Kevin Harwell <default.enum at gmail.com>
Gerrit-Comment-Date: Thu, 15 Dec 2022 09:12:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean at seanbright.com>
Comment-In-Reply-To: Kevin Harwell <default.enum at gmail.com>
Comment-In-Reply-To: Jaco Kroon <jaco at uls.co.za>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20221215/9145e361/attachment.html>