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

[Kaloyan Kovachev]

On Fri, 23 Apr 2010 10:19:12 -0500, Tilghman Lesher wrote
> On Friday 23 April 2010 05:18:14 Kaloyan Kovachev wrote:
> > On Thu, 22 Apr 2010 20:56:41 -0000, "David Vossel" wrote:
> > > 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)
> >
> > 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.
> 
> Because the manipulate callback doesn't produce a new frame; it merely
> changes the audio in the frame passed.  The return value is what indicates
> whether the audio in the frame was successfully changed or not.

Yes, but in case of error we do not know what is the content of (possibly
partially) modified frame, so either:
 the manipulate callback should restore the original frame before returning
and later not to free the returned frame on error
OR
 to set the frame to null and then after return from the callback on error or
null frame to restore the middle frame (from a copy) instead of freeing it
OR
 to free it and stop processing callbacks (as proposed above), then to return
the completely unmodified frame

in either case freeing the middle frame on error is wrong on my opinion, as it
will cause the cascaded manipulations to fail for sure (so no need to call
them in first place) and if they are not checking for null frame to crash

> 
> -- 
> Tilghman Lesher
> Digium, Inc. | Senior Software Developer
> twitter: Corydon76 | IRC: Corydon76-dig (Freenode)
> Check us out at: www.digium.com & www.asterisk.org
> 
> -- 
> _____________________________________________________________________
> -- 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