Richard Kenner
2017-Apr-12 13:09 UTC
[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.
Joshua Colp
2017-Apr-12 13:19 UTC
[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
Richard Kenner
2017-Apr-12 13:29 UTC
[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.