[asterisk-dev] res_fax_spandsp segfaults during fax detection - FIXED?

[Pavel Troller]

Hello Michal,
> Dear Pavel,
>
> On 01/30/2014 06:55 AM, Pavel Troller wrote:
>>> I probably found what causes random segfaults during fax detection with
>>> spandsp... Many thanks for your contributions and brainstorming about this,
>>> it helped a lot. If you have such issue, please test this possible fix and
>>> let me know...
>>>
>>> At res_fax.c, somewhere around line 3070, you'll find (at least for
>>> Asterisk 11)
>>>       case AST_FORMAT_ALAW:
>>>       case AST_FORMAT_ULAW:
>>> Comment out these two lines, recompile, reload and test.
>> As I understand the code, by commenting out these lines, we'll just abandon
>> the frame hook for A/ULAW samples, so we'll never gateway them then... Yes,
>> it will probabaly stop segfaulting, but as a side-effect, it will stop
>> gatewaying as well :-). Of course I may be wrong, I just had a little look
>> there...
>
> I tested this fix before posting to the list, and T38 gatewaying is working 
> with it, at least in my setup (between DAHDI and SIP). I tested both ways 
> (SIP->DAHDI, DAHDI->SIP) and fax receipt was successful. Gateway is enabled 
> in sip.conf, in peer section:
> setvar=FAXOPT(t38gateway)=yes,20

OK, but it's really strange, because by omitting these codecs from the switch
the code will return from the audiohook, doing nothing more.

>
>>    However, it seems that there is really a bug in the hook.
>>    - On line 3130, we are attempting to translate the frames before we write
>> them to the faxing engine (on line 3141). It's OK.
>>    - BUT: On line 3122, we are calling fax_gateway_detect_v21() with
>> UNTRANSLATED frames, which is FATAL - this detection can handle only linear
>> format similarly as real faxing, so we have to translate non-linear formats
>> (A/ULAW) for it as well! By not doing this, we probably
>> 1) prevent successful detection of V.21, so it doesn't work either
>> 2) In rare cases, crash the whole thing, because the detection code always
>> tries to read 320 bytes from 160byte buffer (as Michal pointed out below).
>>    Once again, I may be wrong, but I think it's in accordance with all the
>> symptoms we have already collected.
>
> .... it seems that you really don't like my quick fix :o))  OK, then forget 
> it, and try this instead:

It's not that I don't like your quick fix, I would just like to have a regular
solution :-).

>
> # diff res/res_fax.c.orig res/res_fax.c
> 2963a2964
> >       struct ast_frame *tmpframe = &ast_null_frame;
> 3117a3119,3131
> >
> >               if (f->subclass.format.id != AST_FORMAT_SLINEAR) {
> >
> >                       if (ast_channel_readtrans(active) && (tmpframe = 
> ast_translate(ast_channel_readtrans(active), f, 0)) == NULL) {
> >                               ast_debug(5, "frame translation to slinear 
> failed, v21 detection skipped\n");
> >                               return f; // translation unsuccessful, dont 
> start v21 detection
> >                       }
> >                       ast_debug(5, "frame translated to slinear for v21 
> detection\n");
> >                       fax_gateway_detect_v21(gateway, chan, peer, active, 
> tmpframe);
> >                       ast_frfree(tmpframe);
> >                       return f;
> >               }
> >

Just two small technical notes:
1) It's more common to use diff -u (unified format is more readable)
2) Please prevent your mail client to wrap the lines around, or attach the
   patch as a standalone attachment, the way how it's presented in your
   e-mail renders it just very hardly usable.

>
> This code will translate non-slinear frames to slinear, just before they 
> are sent to libspandsp for v21detection. With this patch applied, v21 
> detection is done also for RTP (SIP) alaw/ulaw frames, so maybe SIP/G711 
> <-> SIP/T38 gateway will work too. I tested DAHDI <-> SIP/T38, gateway 
> works both ways, voice calls too. Is it better now? :o)
>

I fully understand the code, but I'm not trained enough in the Asterisk 
internals to respond to questions, which immediately appeared in my head:
1) In the original code, the result from fax_gateway_detect_v21() is returned.
Now, you are returning the original frame. I quickly looked at the above
routine and it in turn calls fax_gateway_request_t38() and returns its
result (but not always), and in the fax_gateway_request_t38() function
they are also returning different things according to results of the
program flow. So, is it really safe to do this ? Are you sure, that the
real result is really unneccessary ?
2) Are you sure, that ast_translate() will always allocate a new buffer for
tmpframe ? Is it written somewhere ? Isn't it possible that it will just
reallocate the buffer for the original frame to increase its size and return
its pointer, so by doing ast_frfree() you would just deallocate the same
buffer, thus making big troubles ? You would find it by checking that
tmpframe != f...
  As you can see, I'm very careful, or maybe even a bit conservative, with
patching things, unless I really DEEPLY understand, how they are going...
So, I believe, that you really studied the code enough to be sure, that
you can really clear my doubts by your deep knowledge... I didn't have time
to study the code to such extent...

With regards,
  Pavel 

> --
> Michal Rybarik
>
>
> -- 
> _____________________________________________________________________
> -- Bandwidth and Colocation Provided by http://www.api-digital.com --
>
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>   http://lists.digium.com/mailman/listinfo/asterisk-dev