Merged revisions 98943 via svnmerge from

https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r98943 | russell | 2008-01-15 17:26:52 -0600 (Tue, 15 Jan 2008) | 25 lines

Commit a fix for some memory access errors pointed out by the valgrind2.txt
output on issue #11698.

The issue here is that it is possible for an instance of a translator to get
destroyed while the frame allocated as a part of the translator is still being
processed.  Specifically, this is possible anywhere between a call to ast_read()
and ast_frame_free(), which is _a lot_ of places in the code.  The reason this
happens is that the channel might get masqueraded during this time.  During a
masquerade, existing translation paths get destroyed.

So, this patch fixes the issue in an API and ABI compatible way.  (This one is
 for you, paravoid!)

It changes an int in ast_frame to be used as flag bits.  The 1 bit is still used
to indicate that the frame contains timing information.  Also, a second flag has
been added to indicate that the frame came from a translator.  When a frame with
this flag gets released and has this flag, a function is called in translate.c to
let it know that this frame is doing being processed.  At this point, the flag gets
cleared.  Also, if the translator was requested to be destroyed while its internal
frame still had this flag set, its destruction has been deffered until it finds out
that the frame is no longer being processed.

Admittedly, this feels like a hack.  But, it does fix the issue, and I was not able
to think of a better solution ...

........

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

diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index 2ad6d8c9fe0ae415186b89bffadfedea9926f40a..e1238caeaf45060e9b08474327d84a741d9dab38 100644 (file)
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -1935,7 +1935,7 @@ static int __do_deliver(void *data)
          the IAX thread with the iaxsl lock held. */
        struct iax_frame *fr = data;
        fr->retrans = -1;
-       fr->af.has_timing_info = 0;
+       ast_clear_flag(&fr->af, AST_FRFLAG_HAS_TIMING_INFO);
        if (iaxs[fr->callno] && !ast_test_flag(iaxs[fr->callno], IAX_ALREADYGONE))
                iax2_queue_frame(fr->callno, &fr->af);
        /* Free our iax frame */

diff --git a/codecs/codec_zap.c b/codecs/codec_zap.c
index ee7214613b6b906986ebfaa1acfabc627331b5ae..01d9a1ed1e8c3b941e210dea3e8199b0b8fe8b83 100644 (file)
--- a/codecs/codec_zap.c
+++ b/codecs/codec_zap.c
@@ -87,7 +87,6 @@ struct pvt {
        int lasttotalms;
 #endif
        struct zt_transcode_header *hdr;
-       struct ast_frame f;
 };
 
 static char *handle_cli_transcoder_show(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
@@ -160,13 +159,14 @@ static struct ast_frame *zap_frameout(struct ast_trans_pvt *pvt)
 
        if (ztp->fake == 2) {
                ztp->fake = 1;
-               ztp->f.frametype = AST_FRAME_VOICE;
-               ztp->f.subclass = 0;
-               ztp->f.samples = 160;
-               ztp->f.data = NULL;
-               ztp->f.offset = 0;
-               ztp->f.datalen = 0;
-               ztp->f.mallocd = 0;
+               pvt->f.frametype = AST_FRAME_VOICE;
+               pvt->f.subclass = 0;
+               pvt->f.samples = 160;
+               pvt->f.data = NULL;
+               pvt->f.offset = 0;
+               pvt->f.datalen = 0;
+               pvt->f.mallocd = 0;
+               ast_set_flag(&pvt->f, AST_FRFLAG_FROM_TRANSLATOR);
                pvt->samples = 0;
        } else if (ztp->fake == 1) {
                return NULL;
@@ -179,14 +179,15 @@ static struct ast_frame *zap_frameout(struct ast_trans_pvt *pvt)
                                ztp->lasttotalms = ztp->totalms;
                        }
 #endif
-                       ztp->f.frametype = AST_FRAME_VOICE;
-                       ztp->f.subclass = hdr->dstfmt;
-                       ztp->f.samples = hdr->dstsamples;
-                       ztp->f.data = hdr->dstdata + hdr->dstoffset;
-                       ztp->f.offset = hdr->dstoffset;
-                       ztp->f.datalen = hdr->dstlen;
-                       ztp->f.mallocd = 0;
-                       pvt->samples -= ztp->f.samples;
+                       pvt->f.frametype = AST_FRAME_VOICE;
+                       pvt->f.subclass = hdr->dstfmt;
+                       pvt->f.samples = hdr->dstsamples;
+                       pvt->f.data = hdr->dstdata + hdr->dstoffset;
+                       pvt->f.offset = hdr->dstoffset;
+                       pvt->f.datalen = hdr->dstlen;
+                       pvt->f.mallocd = 0;
+                       ast_set_flag(&pvt->f, AST_FRFLAG_FROM_TRANSLATOR);
+                       pvt->samples -= pvt->f.samples;
                        hdr->dstlen = 0;
                        
                } else {
@@ -200,7 +201,7 @@ static struct ast_frame *zap_frameout(struct ast_trans_pvt *pvt)
                }
        }
 
-       return &ztp->f;
+       return &pvt->f;
 }
 
 static void zap_destroy(struct ast_trans_pvt *pvt)

diff --git a/include/asterisk/frame.h b/include/asterisk/frame.h
index 1a66068c3ba3009d0cbfc439ab845cdecb75481f..c4d5832f1ec3cc778df620cac995851c7dd53fcd 100644 (file)
--- a/include/asterisk/frame.h
+++ b/include/asterisk/frame.h
@@ -122,6 +122,15 @@ enum ast_frame_type {
 };
 #define AST_FRAME_DTMF AST_FRAME_DTMF_END
 
+enum {
+       /*! This frame contains valid timing information */
+       AST_FRFLAG_HAS_TIMING_INFO = (1 << 0),
+       /*! This frame came from a translator and is still the original frame.
+        *  The translator can not be free'd if the frame inside of it still has
+        *  this flag set. */
+       AST_FRFLAG_FROM_TRANSLATOR = (1 << 1),
+};
+
 /*! \brief Data structure associated with a single frame of data
  */
 struct ast_frame {
@@ -147,8 +156,8 @@ struct ast_frame {
        struct timeval delivery;
        /*! For placing in a linked list */
        AST_LIST_ENTRY(ast_frame) frame_list;
-       /*! Timing data flag */
-       int has_timing_info;
+       /*! Misc. frame flags */
+       unsigned int flags;
        /*! Timestamp in milliseconds */
        long ts;
        /*! Length in milliseconds */

diff --git a/include/asterisk/translate.h b/include/asterisk/translate.h
index 48b5bf09160d0559355cf6dfb44d396c263df5db..b1f615f7ff11be07fd5b4f0b7fa1eedebe33980f 100644 (file)
--- a/include/asterisk/translate.h
+++ b/include/asterisk/translate.h
@@ -138,7 +138,14 @@ struct ast_trans_pvt {
        struct ast_translator *t;
        struct ast_frame f;     /*!< used in frameout */
        int samples;            /*!< samples available in outbuf */
-       int datalen;            /*!< actual space used in outbuf */
+       /*! 
+        * \brief actual space used in outbuf
+        *
+        * Also, for the sake of ABI compatability, a magic value of -1 in this
+        * field means that the pvt has been requested to be destroyed, but is
+        * pending destruction until ast_translate_frame_freed() gets called. 
+        */
+       int datalen;
        void *pvt;              /*!< more private data, if any */
        char *outbuf;           /*!< the useful portion of the buffer */
        plc_state_t *plc;       /*!< optional plc pointer */
@@ -250,6 +257,20 @@ unsigned int ast_translate_path_steps(unsigned int dest, unsigned int src);
  */
 unsigned int ast_translate_available_formats(unsigned int dest, unsigned int src);
 
+/*!
+ * \brief Hint that a frame from a translator has been freed
+ *
+ * This is sort of a hack.  This function gets called when ast_frame_free() gets
+ * called on a frame that has the AST_FRFLAG_FROM_TRANSLATOR flag set.  This is
+ * because it is possible for a translation path to be destroyed while a frame
+ * from a translator is still in use.  Specifically, this happens if a masquerade
+ * happens after a call to ast_read() but before the frame is done being processed, 
+ * since the frame processing is generally done without the channel lock held.
+ *
+ * \return nothing
+ */
+void ast_translate_frame_freed(struct ast_frame *fr);
+
 #if defined(__cplusplus) || defined(c_plusplus)
 }
 #endif

diff --git a/main/abstract_jb.c b/main/abstract_jb.c
index afc6bd182e0cedfc79303c5983f2df63b1571a87..4f464796dd7836fae5d0031ecbfb61f1cd471479 100644 (file)
--- a/main/abstract_jb.c
+++ b/main/abstract_jb.c
@@ -312,10 +312,10 @@ int ast_jb_put(struct ast_channel *chan, struct ast_frame *f)
        }
 
        /* We consider an enabled jitterbuffer should receive frames with valid timing info. */
-       if (!f->has_timing_info || f->len < 2 || f->ts < 0) {
+       if (!ast_test_flag(f, AST_FRFLAG_HAS_TIMING_INFO) || f->len < 2 || f->ts < 0) {
                ast_log(LOG_WARNING, "%s received frame with invalid timing info: "
                        "has_timing_info=%d, len=%ld, ts=%ld, src=%s\n",
-                       chan->name, f->has_timing_info, f->len, f->ts, f->src);
+                       chan->name, ast_test_flag(f, AST_FRFLAG_HAS_TIMING_INFO), f->len, f->ts, f->src);
                return -1;
        }
 

diff --git a/main/frame.c b/main/frame.c
index bb386df55774022bf198763077e66cd9304d9d80..9b7d150877412b3e7fd6a2f65c70429c02798f2d 100644 (file)
--- a/main/frame.c
+++ b/main/frame.c
@@ -36,6 +36,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include "asterisk/utils.h"
 #include "asterisk/threadstorage.h"
 #include "asterisk/linkedlists.h"
+#include "asterisk/translate.h"
 
 #ifdef TRACE_FRAMES
 static int headers;
@@ -355,6 +356,9 @@ void ast_frame_free(struct ast_frame *fr, int cache)
 #endif                 
                ast_free(fr);
        }
+
+       if (ast_test_flag(fr, AST_FRFLAG_FROM_TRANSLATOR))
+               ast_translate_frame_freed(fr);
 }
 
 /*!
@@ -366,7 +370,9 @@ struct ast_frame *ast_frisolate(struct ast_frame *fr)
 {
        struct ast_frame *out;
        void *newdata;
-       
+
+       ast_clear_flag(fr, AST_FRFLAG_FROM_TRANSLATOR);
+
        if (!(fr->mallocd & AST_MALLOCD_HDR)) {
                /* Allocate a new header if needed */
                if (!(out = ast_frame_header_new()))
@@ -378,8 +384,8 @@ struct ast_frame *ast_frisolate(struct ast_frame *fr)
                out->offset = fr->offset;
                out->data = fr->data;
                /* Copy the timing data */
-               out->has_timing_info = fr->has_timing_info;
-               if (fr->has_timing_info) {
+               ast_copy_flags(out, fr, AST_FRFLAG_HAS_TIMING_INFO);
+               if (ast_test_flag(fr, AST_FRFLAG_HAS_TIMING_INFO)) {
                        out->ts = fr->ts;
                        out->len = fr->len;
                        out->seqno = fr->seqno;
@@ -483,7 +489,7 @@ struct ast_frame *ast_frdup(const struct ast_frame *f)
                /* Must have space since we allocated for it */
                strcpy((char *)out->src, f->src);
        }
-       out->has_timing_info = f->has_timing_info;
+       ast_copy_flags(out, f, AST_FRFLAG_HAS_TIMING_INFO);
        out->ts = f->ts;
        out->len = f->len;
        out->seqno = f->seqno;

diff --git a/main/rtp.c b/main/rtp.c
index cdfab894bd12359095cded917c07e73ecd75a68d..0794fbcde61a5323aa207f1af6ebb580f1ab2d8a 100644 (file)
--- a/main/rtp.c
+++ b/main/rtp.c
@@ -1583,7 +1583,7 @@ struct ast_frame *ast_rtp_read(struct ast_rtp *rtp)
                        ast_frame_byteswap_be(&rtp->f);
                calc_rxstamp(&rtp->f.delivery, rtp, timestamp, mark);
                /* Add timing data to let ast_generic_bridge() put the frame into a jitterbuf */
-               rtp->f.has_timing_info = 1;
+               ast_set_flag(&rtp->f, AST_FRFLAG_HAS_TIMING_INFO);
                rtp->f.ts = timestamp / 8;
                rtp->f.len = rtp->f.samples / 8;
        } else if(rtp->f.subclass & AST_FORMAT_VIDEO_MASK) {
@@ -3028,7 +3028,7 @@ static int ast_rtp_raw_write(struct ast_rtp *rtp, struct ast_frame *f, int codec
        if (rtp->lastts > rtp->lastdigitts)
                rtp->lastdigitts = rtp->lastts;
 
-       if (f->has_timing_info)
+       if (ast_test_flag(f, AST_FRFLAG_HAS_TIMING_INFO))
                rtp->lastts = f->ts * 8;
 
        /* Get a pointer to the header */

diff --git a/main/translate.c b/main/translate.c
index 2e98df92b2b6ee44340b3d0d495bffc3929ca599..8bdaaf082b5dc7a1895606ebfe48d694fabdca4d 100644 (file)
--- a/main/translate.c
+++ b/main/translate.c
@@ -133,6 +133,18 @@ static void destroy(struct ast_trans_pvt *pvt)
 {
        struct ast_translator *t = pvt->t;
 
+       if (ast_test_flag(&pvt->f, AST_FRFLAG_FROM_TRANSLATOR)) {
+               /* If this flag is still set, that means that the translation path has
+                * been torn down, while we still have a frame out there being used.
+                * When ast_frfree() gets called on that frame, this ast_trans_pvt
+                * will get destroyed, too. */
+
+               /* Set the magic hint that this has been requested to be destroyed. */
+               pvt->datalen = -1;
+
+               return;
+       }
+
        if (t->destroy)
                t->destroy(pvt);
        ast_free(pvt);
@@ -147,7 +159,7 @@ static int framein(struct ast_trans_pvt *pvt, struct ast_frame *f)
        int samples = pvt->samples;     /* initial value */
        
        /* Copy the last in jb timing info to the pvt */
-       pvt->f.has_timing_info = f->has_timing_info;
+       ast_copy_flags(&pvt->f, f, AST_FRFLAG_HAS_TIMING_INFO);
        pvt->f.ts = f->ts;
        pvt->f.len = f->len;
        pvt->f.seqno = f->seqno;
@@ -226,6 +238,8 @@ struct ast_frame *ast_trans_frameout(struct ast_trans_pvt *pvt,
        f->src = pvt->t->name;
        f->data = pvt->outbuf;
 
+       ast_set_flag(f, AST_FRFLAG_FROM_TRANSLATOR);
+
        return f;
 }
 
@@ -304,7 +318,7 @@ struct ast_frame *ast_translate(struct ast_trans_pvt *path, struct ast_frame *f,
        long len;
        int seqno;
 
-       has_timing_info = f->has_timing_info;
+       has_timing_info = ast_test_flag(f, AST_FRFLAG_HAS_TIMING_INFO);
        ts = f->ts;
        len = f->len;
        seqno = f->seqno;
@@ -354,7 +368,7 @@ struct ast_frame *ast_translate(struct ast_trans_pvt *path, struct ast_frame *f,
                path->nextout = ast_tvadd(path->nextout, ast_samp2tv(out->samples, format_rate(out->subclass)));
        } else {
                out->delivery = ast_tv(0, 0);
-               out->has_timing_info = has_timing_info;
+               ast_set2_flag(out, has_timing_info, AST_FRFLAG_HAS_TIMING_INFO);
                if (has_timing_info) {
                        out->ts = ts;
                        out->len = len;
@@ -875,3 +889,17 @@ unsigned int ast_translate_available_formats(unsigned int dest, unsigned int src
 
        return res;
 }
+
+void ast_translate_frame_freed(struct ast_frame *fr)
+{
+       struct ast_trans_pvt *pvt;
+
+       ast_clear_flag(fr, AST_FRFLAG_FROM_TRANSLATOR);
+
+       pvt = (struct ast_trans_pvt *) (((char *) fr) - offsetof(struct ast_trans_pvt, f));
+
+       if (pvt->datalen != -1)
+               return;
+       
+       destroy(pvt);
+}