Review Board 1.7.16


Delay the destruction of an ast_filestream structure until the frame embedded within is freed.

Review Request #46 - Created Nov. 10, 2008 and submitted

Mark Michelson
1.4
Reviewers
asterisk-dev
russell
Asterisk
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.
I have done some simple file playbacks just to be sure that there is no horrible breakage.
Review request changed
Updated (Nov. 11, 2008, 12:42 p.m.)
I've done as Russell instructed in his latest review. As far as the comment for the doxygen in file.h is concerned, I assume that by \arg, he meant \param. I also cleared up ast_closestream to just unref the filestream by 1 unconditionally. Once again, I ran some file playback tests, and all seemed well.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.