When merging the fix for issue #14118, I found that

the issue didn't affect 1.6.0, but in this case that's
not an especially good thing, because it means that
the fix for issue #13496 was not merged into 1.6.0 in
the first place. This commit kills two birds with one
stone by putting both fixes in the 1.6.0 branch



git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.6.0@166278 65c4cc65-6c06-0410-ace0-fbb531ad65f3

include/asterisk/file.h

@@ -315,6 +315,21 @@ off_t ast_tellstream(struct ast_filestream *fs);
  */ 
 struct ast_frame *ast_readframe(struct ast_filestream *s);
 
+/*!\brief destroy a filestream using an ast_frame as input
+ *
+ * This is a hack that is used also by the ast_trans_pvt and
+ * ast_dsp structures. When a structure contains an ast_frame
+ * pointer as one of its fields. It may be that the frame is
+ * still used after the outer structure is freed. This leads to
+ * invalid memory accesses. This function allows for us to hold
+ * off on destroying the ast_filestream until we are done using
+ * the ast_frame pointer that is part of it
+ *
+ * \param fr The ast_frame that is part of an ast_filestream we wish
+ * to free.
+ */
+void ast_filestream_frame_freed(struct ast_frame *fr);
+
 #if defined(__cplusplus) || defined(c_plusplus)
 }
 #endif

include/asterisk/frame.h

@@ -134,6 +134,10 @@ enum {
 	 *  The dsp cannot be free'd if the frame inside of it still has
 	 *  this flag set. */
 	AST_FRFLAG_FROM_DSP = (1 << 2),
+	/*! This frame came from a filestream and is still the original frame.
+	 *  The filestream cannot be free'd if the frame inside of it still has
+	 *  this flag set. */
+	AST_FRFLAG_FROM_FILESTREAM = (1 << 3),
 };
 
 /*! \brief Data structure associated with a single frame of data

main/file.c

@@ -43,6 +43,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include "asterisk/pbx.h"
 #include "asterisk/linkedlists.h"
 #include "asterisk/module.h"
+#include "asterisk/astobj2.h"
 
 /*
  * The following variable controls the layout of localized sound files.
@@ -279,12 +280,57 @@ static int exts_compare(const char *exts, const char *type)
 	return 0;
 }
 
+static void filestream_destructor(void *arg)
+{
+	char *cmd = NULL;
+	size_t size = 0;
+	struct ast_filestream *f = arg;
+
+	/* Stop a running stream if there is one */
+	if (f->owner) {
+		if (f->fmt->format < AST_FORMAT_AUDIO_MASK) {
+			f->owner->stream = NULL;
+			AST_SCHED_DEL(f->owner->sched, f->owner->streamid);
+#ifdef HAVE_DAHDI
+			ast_settimeout(f->owner, 0, NULL, NULL);
+#endif			
+		} else {
+			f->owner->vstream = NULL;
+			AST_SCHED_DEL(f->owner->sched, f->owner->vstreamid);
+		}
+	}
+	/* destroy the translator on exit */
+	if (f->trans)
+		ast_translator_free_path(f->trans);
+
+	if (f->realfilename && f->filename) {
+			size = strlen(f->filename) + strlen(f->realfilename) + 15;
+			cmd = alloca(size);
+			memset(cmd,0,size);
+			snprintf(cmd,size,"/bin/mv -f %s %s",f->filename,f->realfilename);
+			ast_safe_system(cmd);
+	}
+
+	if (f->filename)
+		free(f->filename);
+	if (f->realfilename)
+		free(f->realfilename);
+	if (f->fmt->close)
+		f->fmt->close(f);
+	fclose(f->f);
+	if (f->vfs)
+		ast_closestream(f->vfs);
+	if (f->orig_chan_name)
+		free((void *) f->orig_chan_name);
+	ast_module_unref(f->fmt->module);
+}
+
 static struct ast_filestream *get_filestream(struct ast_format *fmt, FILE *bfile)
 {
 	struct ast_filestream *s;
 
 	int l = sizeof(*s) + fmt->buf_size + fmt->desc_size;	/* total allocation size */
-	if ( (s = ast_calloc(1, l)) == NULL)
+	if ( (s = ao2_alloc(l, filestream_destructor)) == NULL)
 		return NULL;
 	s->fmt = fmt;
 	s->f = bfile;
@@ -637,6 +683,10 @@ struct ast_frame *ast_readframe(struct ast_filestream *s)
 	int whennext = 0;	
 	if (s && s->fmt)
 		f = s->fmt->read(s, &whennext);
+	if (f) {
+		ast_set_flag(f, AST_FRFLAG_FROM_FILESTREAM);
+		ao2_ref(s, +1);
+	}
 	return f;
 }
 
@@ -659,6 +709,10 @@ static enum fsread_res ast_readaudio_callback(struct ast_filestream *s)
 			goto return_failure;
 		
 		fr = s->fmt->read(s, &whennext);
+		if (fr) {
+			ast_set_flag(fr, AST_FRFLAG_FROM_FILESTREAM);
+			ao2_ref(s, +1);
+		}
 		if (!fr /* stream complete */ || ast_write(s->owner, fr) /* error writing */) {
 			if (fr)
 				ast_log(LOG_WARNING, "Failed to write frame\n");
@@ -715,6 +769,10 @@ static enum fsread_res ast_readvideo_callback(struct ast_filestream *s)
 
 	while (!whennext) {
 		struct ast_frame *fr = s->fmt->read(s, &whennext);
+		if (fr) {
+			ast_set_flag(fr, AST_FRFLAG_FROM_FILESTREAM);
+			ao2_ref(s, +1);
+		}
 		if (!fr || ast_write(s->owner, fr)) { /* no stream or error, as above */
 			if (fr)
 				ast_log(LOG_WARNING, "Failed to write frame\n");
@@ -792,46 +850,21 @@ int ast_stream_rewind(struct ast_filestream *fs, off_t ms)
 
 int ast_closestream(struct ast_filestream *f)
 {
-	char *cmd = NULL;
-	size_t size = 0;
-	/* Stop a running stream if there is one */
-	if (f->owner) {
-		if (f->fmt->format & AST_FORMAT_AUDIO_MASK) {
-			f->owner->stream = NULL;
-			AST_SCHED_DEL(f->owner->sched, f->owner->streamid);
-#ifdef HAVE_DAHDI
-			ast_settimeout(f->owner, 0, NULL, NULL);
-#endif			
-		} else {
-			f->owner->vstream = NULL;
-			AST_SCHED_DEL(f->owner->sched, f->owner->vstreamid);
-		}
+	if (ast_test_flag(&f->fr, AST_FRFLAG_FROM_FILESTREAM)) {
+		/* If this flag is still set, it essentially means that the reference
+		 * count of f is non-zero. We can't destroy this filestream until
+		 * whatever is using the filestream's frame has finished.
+		 *
+		 * Since this was called, however, we need to remove the reference from
+		 * when this filestream was first allocated. That way, when the embedded
+		 * frame is freed, the refcount will reach 0 and we can finish destroying
+		 * this filestream properly.
+		 */
+		ao2_ref(f, -1);
+		return 0;
 	}
-	/* destroy the translator on exit */
-	if (f->trans)
-		ast_translator_free_path(f->trans);
-
-	if (f->realfilename && f->filename) {
-			size = strlen(f->filename) + strlen(f->realfilename) + 15;
-			cmd = alloca(size);
-			memset(cmd, 0, size);
-			snprintf(cmd, size, "/bin/mv -f %s %s", f->filename, f->realfilename);
-			ast_safe_system(cmd);
-	}
-
-	if (f->filename)
-		ast_free(f->filename);
-	if (f->realfilename)
-		ast_free(f->realfilename);
-	if (f->fmt->close)
-		f->fmt->close(f);
-	fclose(f->f);
-	if (f->vfs)
-		ast_closestream(f->vfs);
-	if (f->orig_chan_name)
-		free((void *) f->orig_chan_name);
-	ast_module_unref(f->fmt->module);
-	ast_free(f);
+	
+	ao2_ref(f, -1);
 	return 0;
 }
 
@@ -1237,6 +1270,17 @@ int ast_waitstream_exten(struct ast_channel *c, const char *context)
 		-1, -1, context);
 }
 
+void ast_filestream_frame_freed(struct ast_frame *fr)
+{
+	struct ast_filestream *fs;
+
+	ast_clear_flag(fr, AST_FRFLAG_FROM_FILESTREAM);
+
+	fs = (struct ast_filestream *) (((char *) fr) - offsetof(struct ast_filestream, fr));
+
+	ao2_ref(fs, -1);
+}
+
 /*
  * if the file name is non-empty, try to play it.
  * Return 0 if success, -1 if error, digit if interrupted by a digit.

main/frame.c

@@ -38,6 +38,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include "asterisk/linkedlists.h"
 #include "asterisk/translate.h"
 #include "asterisk/dsp.h"
+#include "asterisk/file.h"
 
 #ifdef TRACE_FRAMES
 static int headers;
@@ -307,10 +308,13 @@ static void frame_cache_cleanup(void *data)
 
 void ast_frame_free(struct ast_frame *fr, int cache)
 {
-	if (ast_test_flag(fr, AST_FRFLAG_FROM_TRANSLATOR))
+	if (ast_test_flag(fr, AST_FRFLAG_FROM_TRANSLATOR)) {
 		ast_translate_frame_freed(fr);
-	else if (ast_test_flag(fr, AST_FRFLAG_FROM_DSP))
+	} else if (ast_test_flag(fr, AST_FRFLAG_FROM_DSP)) {
 		ast_dsp_frame_freed(fr);
+	} else if (ast_test_flag(fr, AST_FRFLAG_FROM_FILESTREAM)) {
+		ast_filestream_frame_freed(fr);
+	}
 
 	if (!fr->mallocd)
 		return;