asterisk users - Apr 2017 - More issues with Siren14 datalen == 0 packets

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:

*** slinfactory.c.orig  2017-02-13 15:00:19.000000000 -0500
--- slinfactory.c       2017-04-12 08:48:16.000000000 -0400
***************
*** 174,178 ****
                        frame_data = frame_ptr->data.ptr;
  
!                       if (frame_ptr->samples <= ineed) {
                                memcpy(offset, frame_data, frame_ptr->samples
* sizeof(*offset));
                                sofar += frame_ptr->samples;
--- 174,180 ----
                        frame_data = frame_ptr->data.ptr;
  
!                       if (frame_ptr->datalen == 0)
!                         ;
!                       else if (frame_ptr->samples <= ineed) {
                                memcpy(offset, frame_data, frame_ptr->samples
* sizeof(*offset));
                                sofar += frame_ptr->samples;

How many more of these cases are there going to be?

Why is samples being used as a length instead of datalen?

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:
<snip>

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

> 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.