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

[Russell Bryant]

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



branches/1.4/include/asterisk/file.h
<http://reviewboard.digium.com/r/46/#comment205>

    I have a proposal for a slightly different way to approach this problem.
    
    What if instead of going this route, we made the ast_filestream a reference counted object, using astobj2.  When an ast_filestream gets allocated, it is done with ao2_alloc, and instead of being directly free'd in ast_closestream(), it would be unreferenced.
    
    Now, to deal with this specific bug, the reference count on the filestream would be increased every time the frame gets returned.  Then, when you get notified that the frame has been free'd, you decrease the reference count on the filestream.
    
    This really wouldn't change the code much at all.  However, it does relieve us from having to overload a field in the filestream to determine when to free it, and does it in a more explicit, and probably safer way.
    
    What I would really like to come up with is a solution that doesn't require the special frame flag and API call, but I haven't thought of anything yet.


- Russell


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