[asterisk-dev] [Code Review] Delay the destruction of an ast_filestream structure until the frame embedded within is freed.

[Russell Bryant]

> On 2008-11-10 19:43:12, Russell Bryant wrote:
> >
> 
> Mark Michelson wrote:
>     Yes, I agree that this is a far more "correct" solution to the issue. I'll see what I can do in this regard. The one part of your explanation that throws me off is this: "Then, when you get notified that the frame has been free'd, you decrease the reference count on the filestream." The concept of when you get notified is a bit vague here. I assume that what you mean is that in addition to increasing the reference count on the filestream when a frame is returned, you also set the AST_FRFLAG_FROM_FILESTREAM flag. If this is what you mean, then it sounds like the only changes I need to make are
>     
>     * Don't use the lastwriteformat as the indicator to destroy the filestream. Let the reference counting do this for me.
>     * Change the allocation of the filestream from ast_calloc to ao2_alloc, and set ast_filestream_close as the destructor callback.
>     * Modify ast_filestream_close to not actually free the filestream itself since astobj2 will take care of this for me.
>     
>     Also, I noticed after looking through this diff that I never actually set the AST_FRFLAG_FROM_FILESTREAM flag anywhere, so this code would have been useless.

When I say "get notified", I mean when ast_frame_free() calls your function because the frame had the FROM_FILESTREAM flag set.  So yeah, it sounds like we're on the same page.

In addition to your list of changes:

* When the filestream code returns the frame, increase the reference count on the filestream
* When the frame free code calls your function since the FROM_FILESTREAM flag was set, decrease the filestream ref count


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/46/#review115
-----------------------------------------------------------


On 2008-11-10 19:35:21, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/46/
> -----------------------------------------------------------
> 
> (Updated 2008-11-10 19:35:21)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> This patch is to alleviate a memory corruption problem observed on a production system. Valgrind showed that there were invalid accesses to frames in the function ast_frame_free. It appears that the problem had to do with the fact that the frames were allocated as part of an ast_filestream structure. After the ast_filestream is freed, we still attempt to operate on the frame that was part of it.
> 
> This patch solves the problem by marking frames which are part of an ast_filestream structure with a flag, AST_FRFLAG_FROM_FILESTREAM. If ast_filestream_close is called on a filestream whose frame is still flagged, then we will delay the destruction of the ast_filestream until ast_frfree is called on the frame.
> 
> I'm confident that this patch is correct, especially since similar measures have been taken with frames embedded in ast_dsp structures and ast_trans_pvt structures. The main concern I have is that I needed to repurpose a field in the ast_filestream structure to be used as a means of indicating that the ast_filestream had been requested to be destroyed. I chose the 'lastwriteformat' field for this and set it to -1 in the case that ast_filestream_close is called prior to the AST_FRFLAG_FROM_FILESTREAM being cleared from the embedded frame. It is important that we are sure that the lastwriteformat variable has no other legitimate reason to be set to -1. Grepping the source shows that the only time this variable is used is in the function ast_writestream. It gets set to the subclass of a voice frame if the frame's format is not one for which we have set up a translator. All voice frame subclasses are non-negative to the best of my knowledge.
> 
> 
> Diffs
> -----
> 
>   branches/1.4/include/asterisk/file.h 152927 
>   branches/1.4/include/asterisk/frame.h 152927 
>   branches/1.4/main/file.c 152927 
>   branches/1.4/main/frame.c 152927 
> 
> Diff: http://reviewboard.digium.com/r/46/diff
> 
> 
> Testing
> -------
> 
> I have done some simple file playbacks just to be sure that there is no horrible breakage.
> 
> 
> Thanks,
> 
> Mark
> 
>