[asterisk-dev] [Code Review] Crash in audiohook_write_list

[Kaloyan Kovachev]

On Thu, 22 Apr 2010 20:56:41 -0000, "David Vossel" <dvossel at digium.com>
wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/623/
> -----------------------------------------------------------
> 
> Review request for Asterisk Developers and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> In audio_audiohook_write_list(), middle_frame represents the translated
> SLINEAR version of the input frame (that is if the input frame was not
> already SLINEAR to begin with, If that is the case, middle_frame is the
> input frame)
> 
> The crash that is occurring is caused by r224856 in trunk which frees
the
> middle_frame if an audiohook manipulation callback fails.  This is not
good
> for several reasons.

This it the crash mentioned in note 113581 for issue 14621
https://issues.asterisk.org/view.php?id=14621
Why not just check that there is a frame before proceeding with the
callback? This way in case of failure no manipulations will be made after
an error and the original frame will be passed unmodified.

> 
> 1. middle_frame could be equal to the input frame, in that case whatever
> we are returning in this function is a pointer to freed memory
> 2. there may be additional audiohook manipulators in the list that could
> process the data, an audiohook manipulation callback failure can be as
> simple as the manipulator not caring about altering the data for that
> direction (example func_speex which was was altered to do that very
thing
> in the same revision), that shouldn't prevent other manipulators in the
> list from doing their thing.
> 3. middle_frame is set to end_frame before manipulation if chan_spy is
> used.  This means it is possible for middle_frame to be freed while
> end_frame still points to the freed data which is later returned at the
end
> of the function.  This could be avoided, but would add even additional
> complication to an already exceedingly complex function for no apparent
> benefit.
> 
> 
> This revision was added to fix something, but I can't figure out what it
> is.  I'm suggesting we revert the audiohook.c part of it.
> 
> 
> This addresses bug 17052.
>     https://issues.asterisk.org/view.php?id=17052
> 
> 
> Diffs
> -----
> 
>   /trunk/main/audiohook.c 258605 
> 
> Diff: https://reviewboard.asterisk.org/r/623/diff
> 
> 
> Testing
> -------
> 
> chanspy can now work with other audiohook manipulators.  
> 
> 
> Thanks,
> 
> David
> 
> 
> -- 
> _____________________________________________________________________
> -- 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