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

[Mark Michelson]

> On 2010-04-22 16:53:32, Tilghman Lesher wrote:
> > /trunk/main/audiohook.c, lines 707-709
> > <https://reviewboard.asterisk.org/r/623/diff/1/?file=9492#file9492line707>
> >
> >     By your previous logic, should we be blindly freeing middle_frame here?  After all, it could be the same as the initial frame.  All we know at this point is that it's not the same as end_frame.
> 
> David Vossel wrote:
>     Yep, Thought about this as well. Even had it in my original patch, but i don't think it is necessary.  At the beginning of the function middle_frame == initial frame and end_frame == initial frame.   If middle_frame is not equal to end_frame in the end, then translation occurred and no manipulation took place, so we just return end_frame which equal to the initial frame that was never touched while freeing the middle_frame which was translated and never manipulated.

I gave this a double-check to make sure there weren't any situations that could pose a problem. David's got it right. If the middle_frame is not the same as the end_frame, then it also implies that the middle_frame is not the same as the start_frame. I would recommend adding a comment here so it's more clear and doesn't take 30 minutes of combing through the code to be absolutely sure.


> On 2010-04-22 16:53:32, Tilghman Lesher wrote:
> > /trunk/main/audiohook.c, line 670
> > <https://reviewboard.asterisk.org/r/623/diff/1/?file=9492#file9492line670>
> >
> >     It does not seem right that manipulate_callback can return an error, and we fail to recognize that an error occurred or to do nothing about it.  For this reason alone, I'm against a simple revert.
> >     
> >     Certainly, you could add some additional logic to check the conditions that you've enumerated above.
> 
> David Vossel wrote:
>     I agree that it is odd that we don't check for the failure, but I don't see any appropriate action to take if we do detect a failure, which is probably why it wasn't checked to begin with.

There are a few roads you can take here, but the absolute first thing that needs to be done is to document the audiohook manipulate prototype in audiohook.h to state for certain if there is any expectation that upon a failure return, the frame passed in may have been modified.

If there's no chance that the frame has been modified on a failure return, then the only purpose of checking the return value would be to potentially print a warning. Of course, if you ask me, the manipulate callbacks themselves should be doing this since they have better knowledge of the domain, so to speak.

If there is a chance that the frame has been modified on a failure return, then the only thing I can think to do is to duplicate the frame prior to calling the manipulate callback. This way, you have a copy of the frame as it was prior to the callback. Pass the copy to the callback. After the callback, on a failure you can delete the copy and use the original from then on. Otherwise, delete the original since the copy was modified successfully.

If you ask me, I think that manipulate callbacks should have the guarantee in place that the frame has not been modified if a failure is returned. Like I said, though, this is not actually documented, so you can't really expect the callbacks to behave like that at the moment.


- Mark


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


On 2010-04-22 15:56:41, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/623/
> -----------------------------------------------------------
> 
> (Updated 2010-04-22 15:56:41)
> 
> 
> 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/main/audiohook.c 258605 
> 
> Diff: https://reviewboard.asterisk.org/r/623/diff
> 
> 
> Testing
> -------
> 
> chanspy can now work with other audiohook manipulators.  
> 
> 
> Thanks,
> 
> David
> 
>