Re: [asterisk-users] More issues with Siren14 datalen == 0 packets
On Wed, Apr 12, 2017, at 10:29 AM, Richard Kenner wrote: > > The feed function in slinfactory explicitly does not allow frames > > without a data payload to be added to the queue. It would have prevented > > this crash. > > Ah, so the fix should really be there, righty? Yes, it already takes care of that. > > > I think the underlying issue is that the data pointer is not NULL when > > it sanely should be in the codec implementation. > > Note that that would still leave a bug in func_speex.c, since it checks > for neither case. > > And, of course, that's not open-source, so I can't fix it. Indeed. It'll go into the queue to get looked at. > > > > Can you suggest ways of searching for other possible occurrences of > > > this bug? These crashes are occurring during important conferences and > > > are causing significant issues. > > > > Not really. Frames are used a lot across Asterisk, so you have to follow > > the flow based on the features in use. > > I did some searches and came across one suspicious case: > > In funcs/app_jack.c:queue_voice_frame > > That's all I see. Please comment it on your original issue. -- Joshua Colp Digium, Inc. | Senior Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - US Check us out at: www.digium.com & www.asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- Check out the new Asterisk community forum at: https://community.asterisk.org/ New to Asterisk? Start here: https://wiki.asterisk.org/wiki/display/AST/Getting+Started asterisk-users mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-users
Re: [asterisk-users] More issues with Siren14 datalen == 0 packets
> The feed function in slinfactory explicitly does not allow frames > without a data payload to be added to the queue. It would have prevented > this crash. Ah, so the fix should really be there, righty? > I think the underlying issue is that the data pointer is not NULL when > it sanely should be in the codec implementation. Note that that would still leave a bug in func_speex.c, since it checks for neither case. And, of course, that's not open-source, so I can't fix it. > > Can you suggest ways of searching for other possible occurrences of > > this bug? These crashes are occurring during important conferences and > > are causing significant issues. > > Not really. Frames are used a lot across Asterisk, so you have to follow > the flow based on the features in use. I did some searches and came across one suspicious case: In funcs/app_jack.c:queue_voice_frame That's all I see. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- Check out the new Asterisk community forum at: https://community.asterisk.org/ New to Asterisk? Start here: https://wiki.asterisk.org/wiki/display/AST/Getting+Started asterisk-users mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-users
Re: [asterisk-users] More issues with Siren14 datalen == 0 packets
On Wed, Apr 12, 2017, at 10:09 AM, Richard Kenner wrote: > > All patches need to go into JIRA with a license agreement to be > > accepted. > > Understood, but I was using it as an illustration. Note, however, that, > from a legal perspective, a patch such as this has no protectable IP (you > can't copyright the only way of doing something) and the GNU projects > have > a formal rule that sufficiently-small patches need no assignments for > that > reason, which I suggest you may want to adopt as well. Matt Fredrickson can pass this on and see. > > > > Why is samples being used as a length instead of datalen? > > > > Internally a signed linear factory operates in terms of samples, not the > > data payload itself. I've also commented on your original issue in > > regards to the siren codecs that it should NULL out the data pointer > > itself. That is more commonly used. > > But I don't think that it would have helped in either case, this one > or in func_speex.c, because neither tests for a null data pointer either. The feed function in slinfactory explicitly does not allow frames without a data payload to be added to the queue. It would have prevented this crash. > > Can you explain the difference between "datalen" and "samples" in this > context, shouldn't they always be related by a (small) linear factor? Nope. The feed actually has a comment detailing a situation which explains why we check: /* In some cases, we can be passed a frame which has no data in it, but * which has a positive number of samples defined. Once such situation is * when a jitter buffer is in use and the jitter buffer interpolates a frame. * The frame it produces has data set to NULL, datalen set to 0, and samples * set to either 160 or 240. > Should I open a JIRA issue for this as well? I think the underlying issue is that the data pointer is not NULL when it sanely should be in the codec implementation. > Can you suggest ways of searching for other possible occurrences of > this bug? These crashes are occurring during important conferences and > are causing significant issues. Not really. Frames are used a lot across Asterisk, so you have to follow the flow based on the features in use. -- Joshua Colp Digium, Inc. | Senior Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - US Check us out at: www.digium.com & www.asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- Check out the new Asterisk community forum at: https://community.asterisk.org/ New to Asterisk? Start here: https://wiki.asterisk.org/wiki/display/AST/Getting+Started asterisk-users mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-users
Re: [asterisk-users] More issues with Siren14 datalen == 0 packets
> All patches need to go into JIRA with a license agreement to be > accepted. Understood, but I was using it as an illustration. Note, however, that, from a legal perspective, a patch such as this has no protectable IP (you can't copyright the only way of doing something) and the GNU projects have a formal rule that sufficiently-small patches need no assignments for that reason, which I suggest you may want to adopt as well. > > Why is samples being used as a length instead of datalen? > > Internally a signed linear factory operates in terms of samples, not the > data payload itself. I've also commented on your original issue in > regards to the siren codecs that it should NULL out the data pointer > itself. That is more commonly used. But I don't think that it would have helped in either case, this one or in func_speex.c, because neither tests for a null data pointer either. Can you explain the difference between "datalen" and "samples" in this context, shouldn't they always be related by a (small) linear factor? Should I open a JIRA issue for this as well? Can you suggest ways of searching for other possible occurrences of this bug? These crashes are occurring during important conferences and are causing significant issues. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- Check out the new Asterisk community forum at: https://community.asterisk.org/ New to Asterisk? Start here: https://wiki.asterisk.org/wiki/display/AST/Getting+Started asterisk-users mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-users
Re: [asterisk-users] More issues with Siren14 datalen == 0 packets
On Wed, Apr 12, 2017, at 09:50 AM, Richard Kenner wrote:
> Another crash with a packet:
>
> $10 = {frametype = AST_FRAME_VOICE, subclass = {integer = 0,
> format = 0x12c62170, frame_ending = 0}, datalen = 0, samples = 640,
> mallocd = 1, mallocd_hdr_len = 324, offset = 64,
> src = 0x2ad290064a08 "siren14tolin32/speex", data = {ptr = 0x80893318,
> uint32 = 2156475160, pad = "\030\063\211\200\000\000\000"}, delivery
> = {
> tv_sec = 1492000520, tv_usec = 225198}, frame_list = {next = 0x0},
> flags = 0, ts = 0, len = 0, seqno = 0}
>
> Note that datalen is zero, but samples aren't.
>
> main/slinfactory.c near line 177 doesn't check for datalen of zero,
> but copies using samples.
>
> Fixed thusly:
All patches need to go into JIRA with a license agreement to be
accepted.
>
> How many more of these cases are there going to be?
It's not a common thing that codecs use, so older code may not handle
it.
> Why is samples being used as a length instead of datalen?
Internally a signed linear factory operates in terms of samples, not the
data payload itself. I've also commented on your original issue in
regards to the siren codecs that it should NULL out the data pointer
itself. That is more commonly used.
--
Joshua Colp
Digium, Inc. | Senior Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - US
Check us out at: www.digium.com & www.asterisk.org
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
Check out the new Asterisk community forum at: https://community.asterisk.org/
New to Asterisk? Start here:
https://wiki.asterisk.org/wiki/display/AST/Getting+Started
asterisk-users mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-users
