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

Mark Michelson mmichelson at digium.com
Tue Nov 11 13:39:50 CST 2008

This is an automatically generated e-mail. To reply, visit:

(Updated 2008-11-11 13:39:50.616051)

Review request for Asterisk Developers and Russell Bryant.


I have made updates as Russell has specified. ast_filestream structures are now refcounted structures.

Most of the code changes are straightforward, but I felt that the logic inside of ast_closestream needed further explanation since it does some odd operations with the refcounts. I have commented this function heavily so that it hopefully is clear how the refcounts work for ast_filestream structures.

I have tested this with a series of playbacks. I compiled with MALLOC_DEBUG turned on and monitored the memory footprint of main/astobj2.c to be sure that the refcounts are handled properly. They were in all tests I ran.


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 (updated)

  branches/1.4/include/asterisk/file.h 155768 
  branches/1.4/include/asterisk/frame.h 155768 
  branches/1.4/main/file.c 155768 
  branches/1.4/main/frame.c 155768 

Diff: http://reviewboard.digium.com/r/46/diff


I have done some simple file playbacks just to be sure that there is no horrible breakage.



More information about the asterisk-dev mailing list