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

[David Vossel]

> On 2010-04-28 17:37:57, Tilghman Lesher wrote:
> > I'd like some additional comments sprinkled throughout, identifying the current state of middle_frame and end_frame (middle_frame => translated, manipulated, end_frame == middle_frame or start_frame).  This should help verify my suggestion, below, and also help with future debugging.

That isn't possible, end_frame is == to middle frame depending on what manipulation happened so there is no definite place say that what is what.  The state of end_frame and middle_frame is dynamic depending on what manipulation callbacks and whispering is done.


> On 2010-04-28 17:37:57, Tilghman Lesher wrote:
> > /trunk/main/audiohook.c, lines 710-712
> > <https://reviewboard.asterisk.org/r/623/diff/2/?file=9672#file9672line710>
> >
> >     I ran through the logic, and what I found is that you do want a conditional here, but what you want it to be is "if (middle_frame != start_frame)".  This can occur if the frame is translated (in most cases, yes), but is neither whispered nor manipulated (this could occur if there are spies).  Of course, if the middle_frame is NOT translated, it's the same as the start_frame, and you definitely do not want to free it here, as it will be freed later.

We don't need this.  Isn't this the same thing we discussed earlier in the review. If middle_frame is the same as start_frame it will be the same as end_frame... which is what the check looks for right above this.  If middle frame was translated but never manipulated, end_frame will not be updated, so middle_frame will not equal end_frame because end_frame still points to the start frame.... in that case this should be freed like it is.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/623/#review1927
-----------------------------------------------------------


On 2010-04-28 11:43:26, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/623/
> -----------------------------------------------------------
> 
> (Updated 2010-04-28 11:43:26)
> 
> 
> 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.
> 
> 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/include/asterisk/audiohook.h 259663 
>   /trunk/main/audiohook.c 259663 
> 
> Diff: https://reviewboard.asterisk.org/r/623/diff
> 
> 
> Testing
> -------
> 
> chanspy can now work with other audiohook manipulators.  
> 
> 
> Thanks,
> 
> David
> 
>