[prev in list] [next in list] [prev in thread] [next in thread] 

List:       asterisk-dev
Subject:    Re: [asterisk-dev] [Code Review] Crash in audiohook_write_list
From:       "David Vossel" <dvossel () digium ! com>
Date:       2010-04-29 14:11:35
Message-ID: 20100429141135.11187.25919 () hotblack ! digium ! internal
[Download RAW message or body]



> On 2010-04-28 18:24:40, Tilghman Lesher wrote:
> > I'm fine with the code, but I think there needs to be a paragraph in here, \
> > possibly with a set of truth tables, to show why each part of the logic is \
> > correct.

I'm not sure if I'll go as far as truth tables, but I agree that a paragraph \
explaining this function in more detail is a good idea.  I'll do this before \
committing it.  Thanks for the review!


- David


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


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


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


[prev in list] [next in list] [prev in thread] [next in thread]