There was an issue when attempting to reference an embedded

frame in a freed ast_filestream. This patch makes use of the
ao2 functions to make sure that we do not free an ast_filestream
structure until the embedded ast_frame has been "freed" as well.

(closes issue #13496)
Reported by: fst-onge
Patches:
      filestream_frame_1_4.diff uploaded by putnopvut (license 60)
Tested by: putnopvut

Closes AST-89

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

diff --git a/include/asterisk/file.h b/include/asterisk/file.h
index 636309bc4ef94540b66369f2a06bbbf7f99b43b1..52b0a9f18beb066de150fa47d6b3985011004c59 100644 (file)
--- a/include/asterisk/file.h
+++ b/include/asterisk/file.h
@@ -402,6 +402,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);
+
 /*! 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 30686efdea341aa693ded86da72f0ba5841db472..6ee131eb2ec50ad36a8d26699004e15666846311 100644 (file)
--- a/include/asterisk/frame.h
+++ b/include/asterisk/frame.h
@@ -135,6 +135,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

diff --git a/main/file.c b/main/file.c
index d891000f7d379984794c5a29ad723c9f30dbabf8..cfa5bf3304ab7b11a316496271d24d6afdae0a5f 100644 (file)
--- a/main/file.c
+++ b/main/file.c
@@ -52,6 +52,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.
@@ -296,12 +297,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_MAX_AUDIO) {
+                       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;
@@ -657,6 +703,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;
 }
 
@@ -813,46 +863,22 @@ 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_MAX_AUDIO) {
-                       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 (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;
        }
-
-       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);
-       free(f);
+       
+       ao2_ref(f, -1);
        return 0;
 }
 
@@ -1261,6 +1287,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.

diff --git a/main/frame.c b/main/frame.c
index d64fa09a7eae3f9b57ac02aedfcbf7ebb05e070c..6768d0a98858a986f3d198a776e98ea1b319ce1d 100644 (file)
--- a/main/frame.c
+++ b/main/frame.c
@@ -45,6 +45,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;
@@ -320,10 +321,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;