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: https://origsvn.digium.com/svn/asterisk/branches/1.4@222878 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/include/asterisk/file.h b/include/asterisk/file.h
index 52b0a9f18beb066de150fa47d6b3985011004c59..636309bc4ef94540b66369f2a06bbbf7f99b43b1 100644 (file)
--- a/include/asterisk/file.h
+++ b/include/asterisk/file.h
@@ -402,21 +402,6 @@ 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);
-
 /*! Initialize file stuff */
 /*!
  * Initializes all the various file stuff.  Basically just registers the cli stuff

diff --git a/include/asterisk/frame.h b/include/asterisk/frame.h
index 608e9516881a1c7ceeabe1c441ac04bfca2fe665..2d9fa69aab65d56063843624545516efdfd02950 100644 (file)
--- a/include/asterisk/frame.h
+++ b/include/asterisk/frame.h
@@ -135,10 +135,6 @@ 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

diff --git a/main/file.c b/main/file.c
index d110d72f9c16b632093fe76844dcc0c30a7367be..57282c3169672065930fe25a13ddae68aea6f020 100644 (file)
--- a/main/file.c
+++ b/main/file.c
@@ -706,17 +706,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 {
@@ -733,15 +752,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");
@@ -749,10 +766,12 @@ static enum fsread_res ast_readaudio_callback(struct ast_filestream *s)
                        }
                        goto return_failure;
                }
+
                if (fr) {
                        ast_frfree(fr);
                }
        }
+
        if (whennext != s->lasttimeout) {
 #ifdef HAVE_DAHDI
                if (s->owner->timingfd > -1) {
@@ -803,11 +822,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");
@@ -816,6 +832,7 @@ static enum fsread_res ast_readvideo_callback(struct ast_filestream *s)
                        s->owner->vstreamid = -1;
                        return FSREAD_FAILURE;
                }
+
                if (fr) {
                        ast_frfree(fr);
                }
@@ -907,20 +924,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;
 }
@@ -1338,17 +1341,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 9de8a700a008eb017f5462d61b9128e5c2b18aa1..6cd886123ee9aa9f1be0ba4ca49ef5cb237ddf90 100644 (file)
--- a/main/frame.c
+++ b/main/frame.c
@@ -346,8 +346,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)
@@ -436,7 +434,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;
        }