Merged revisions 222880 via svnmerge from

https://origsvn.digium.com/svn/asterisk/trunk

................
  r222880 | russell | 2009-10-08 14:52:03 -0500 (Thu, 08 Oct 2009) | 51 lines
  
  Merged revisions 222878 via svnmerge from 
  https://origsvn.digium.com/svn/asterisk/branches/1.4
  
  ........
    r222878 | russell | 2009-10-08 14:45:47 -0500 (Thu, 08 Oct 2009) | 44 lines
    
    Make filestream frame handling safer by isolating frames before returning them.
    
    This patch is related to a number of issues on the bug tracker that show
    crashes related to freeing frames that came from a filestream.  A number of
    fixes have been made over time while trying to figure out these problems, but
    there re still people seeing the crash.  (Note that some of these bug reports
    include information about other problems.  I am specifically addressing
    the filestream frame crash here.)
    
    I'm still not clear on what the exact problem is.  However, what is _very_
    clear is that we have seen quite a few problems over time related to unexpected
    behavior when we try to use embedded frames as an optimization.  In some cases,
    this optimization doesn't really provide much due to improvements made in other
    areas.
    
    In this case, the patch modifies filestream handling such that the embedded frame
    will not be returned.  ast_frisolate() is used to ensure that we end up with a
    completely mallocd frame.  In reality, though, we will not actually have to malloc
    every time.  For filestreams, the frame will almost always be allocated and freed
    in the same thread.  That means that the thread local frame cache will be used.
    So, going this route doesn't hurt.
    
    With this patch in place, some people have reported success in not seeing the
    crash anymore.
    
    (SWP-150)
    (AST-208)
    (ABE-1834)
    
    (issue #15609)
    Reported by: aragon
    Patches:
          filestream_frisolate-1.4.diff2.txt uploaded by russell (license 2)
    Tested by: aragon, russell
    
    (closes issue #15817)
    Reported by: zerohalo
    Tested by: zerohalo
    
    (closes issue #15845)
    Reported by: marhbere
    
    Review: https://reviewboard.asterisk.org/r/386/
  ........
................


git-svn-id: http://svn.digium.com/svn/asterisk/branches/1.6.2@222883 f38db490-d61c-443f-a65b-d21fe96a405b

2 files changed, 38 insertions, 49 deletions

diff --git a/main/file.c b/main/file.c
index 26c6b0be6..c20e64f14 100644
--- a/main/file.c
+++ b/main/file.c
@@ -692,17 +692,36 @@ struct ast_filestream *ast_openvstream(struct ast_channel *chan, const char *fil
 	return NULL;
 }
 
-struct ast_frame *ast_readframe(struct ast_filestream *s)
+static struct ast_frame *read_frame(struct ast_filestream *s, int *whennext)
 {
-	struct ast_frame *f = NULL;
-	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);
+	struct ast_frame *fr, *new_fr;
+
+	if (!s || !s->fmt) {
+		return NULL;
+	}
+
+	if (!(fr = s->fmt->read(s, whennext))) {
+		return NULL;
+	}
+
+	if (!(new_fr = ast_frisolate(fr))) {
+		ast_frfree(fr);
+		return NULL;
+	}
+
+	if (new_fr != fr) {
+		ast_frfree(fr);
+		fr = new_fr;
 	}
-	return f;
+
+	return fr;
+}
+
+struct ast_frame *ast_readframe(struct ast_filestream *s)
+{
+	int whennext = 0;
+
+	return read_frame(s, &whennext);
 }
 
 enum fsread_res {
@@ -719,15 +738,13 @@ static enum fsread_res ast_readaudio_callback(struct ast_filestream *s)
 
 	while (!whennext) {
 		struct ast_frame *fr;
-		
-		if (s->orig_chan_name && strcasecmp(s->owner->name, s->orig_chan_name))
+
+		if (s->orig_chan_name && strcasecmp(s->owner->name, s->orig_chan_name)) {
 			goto return_failure;
-		
-		fr = s->fmt->read(s, &whennext);
-		if (fr) {
-			ast_set_flag(fr, AST_FRFLAG_FROM_FILESTREAM);
-			ao2_ref(s, +1);
 		}
+
+		fr = read_frame(s, &whennext);
+
 		if (!fr /* stream complete */ || ast_write(s->owner, fr) /* error writing */) {
 			if (fr) {
 				ast_log(LOG_WARNING, "Failed to write frame\n");
@@ -735,10 +752,12 @@ static enum fsread_res ast_readaudio_callback(struct ast_filestream *s)
 			}
 			goto return_failure;
 		} 
+
 		if (fr) {
 			ast_frfree(fr);
 		}
 	}
+
 	if (whennext != s->lasttimeout) {
 		if (s->owner->timingfd > -1) {
 			float samp_rate = (float) ast_format_rate(s->fmt->format);
@@ -782,11 +801,8 @@ static enum fsread_res ast_readvideo_callback(struct ast_filestream *s)
 	int whennext = 0;
 
 	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);
-		}
+		struct ast_frame *fr = read_frame(s, &whennext);
+
 		if (!fr /* stream complete */ || ast_write(s->owner, fr) /* error writing */) {
 			if (fr) {
 				ast_log(LOG_WARNING, "Failed to write frame\n");
@@ -795,6 +811,7 @@ static enum fsread_res ast_readvideo_callback(struct ast_filestream *s)
 			s->owner->vstreamid = -1;
 			return FSREAD_FAILURE;
 		}
+
 		if (fr) {
 			ast_frfree(fr);
 		}
@@ -886,20 +903,6 @@ int ast_closestream(struct ast_filestream *f)
 		}
 	}
 
-	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;
-	}
-	
 	ao2_ref(f, -1);
 	return 0;
 }
@@ -1331,17 +1334,6 @@ 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.
diff --git a/main/frame.c b/main/frame.c
index 496da920c..338cf0388 100644
--- a/main/frame.c
+++ b/main/frame.c
@@ -336,8 +336,6 @@ static void __frame_free(struct ast_frame *fr, int cache)
 		ast_translate_frame_freed(fr);
 	} 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)
@@ -426,7 +424,6 @@ struct ast_frame *ast_frisolate(struct ast_frame *fr)
 	} else {
 		ast_clear_flag(fr, AST_FRFLAG_FROM_TRANSLATOR);
 		ast_clear_flag(fr, AST_FRFLAG_FROM_DSP);
-		ast_clear_flag(fr, AST_FRFLAG_FROM_FILESTREAM);
 		out = fr;
 	}