Merged revisions 119156 via svnmerge from
authorRussell Bryant <russell@russellbryant.com>
Thu, 29 May 2008 22:28:50 +0000 (22:28 +0000)
committerRussell Bryant <russell@russellbryant.com>
Thu, 29 May 2008 22:28:50 +0000 (22:28 +0000)
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r119156 | russell | 2008-05-29 17:24:29 -0500 (Thu, 29 May 2008) | 10 lines

Fix a race condition in channel autoservice.  There was still a small window of opportunity
for a DTMF frame, or some other deferred frame type, to come in and get dropped.

(closes issue #12656)
(closes issue #12656)
Reported by: dimas
Patches:
      v3-12656.patch uploaded by dimas (license 88)
  -- with some modifications by me

........

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

main/autoservice.c

index c255091..df475fa 100644 (file)
@@ -56,7 +56,7 @@ struct asent {
         *  it gets stopped for the last time. */
        unsigned int use_count;
        unsigned int orig_end_dtmf_flag:1;
         *  it gets stopped for the last time. */
        unsigned int use_count;
        unsigned int orig_end_dtmf_flag:1;
-       AST_LIST_HEAD_NOLOCK(, ast_frame) dtmf_frames;
+       AST_LIST_HEAD_NOLOCK(, ast_frame) deferred_frames;
        AST_LIST_ENTRY(asent) list;
 };
 
        AST_LIST_ENTRY(asent) list;
 };
 
@@ -67,27 +67,16 @@ static pthread_t asthread = AST_PTHREADT_NULL;
 
 static int as_chan_list_state;
 
 
 static int as_chan_list_state;
 
-static void defer_frame(struct ast_channel *chan, struct ast_frame *f)
-{
-       struct ast_frame *dup_f;
-       struct asent *as;
-
-       AST_LIST_LOCK(&aslist);
-       AST_LIST_TRAVERSE(&aslist, as, list) {
-               if (as->chan != chan)
-                       continue;
-               if ((dup_f = ast_frdup(f)))
-                       AST_LIST_INSERT_TAIL(&as->dtmf_frames, dup_f, frame_list);
-       }
-       AST_LIST_UNLOCK(&aslist);
-}
-
 static void *autoservice_run(void *ign)
 {
        for (;;) {
 static void *autoservice_run(void *ign)
 {
        for (;;) {
-               struct ast_channel *mons[MAX_AUTOMONS], *chan;
+               struct ast_channel *mons[MAX_AUTOMONS];
+               struct asent *ents[MAX_AUTOMONS];
+               struct ast_channel *chan;
                struct asent *as;
                struct asent *as;
-               int x = 0, ms = 50;
+               int i, x = 0, ms = 50;
+               struct ast_frame *f = NULL;
+               struct ast_frame *defer_frame = NULL;
 
                AST_LIST_LOCK(&aslist);
 
 
                AST_LIST_LOCK(&aslist);
 
@@ -95,42 +84,48 @@ static void *autoservice_run(void *ign)
                 * to get used again. */
                as_chan_list_state++;
 
                 * to get used again. */
                as_chan_list_state++;
 
-               if (AST_LIST_EMPTY(&aslist))
+               if (AST_LIST_EMPTY(&aslist)) {
                        ast_cond_wait(&as_cond, &aslist.lock);
                        ast_cond_wait(&as_cond, &aslist.lock);
+               }
 
                AST_LIST_TRAVERSE(&aslist, as, list) {
                        if (!ast_check_hangup(as->chan)) {
 
                AST_LIST_TRAVERSE(&aslist, as, list) {
                        if (!ast_check_hangup(as->chan)) {
-                               if (x < MAX_AUTOMONS)
+                               if (x < MAX_AUTOMONS) {
+                                       ents[x] = as;
                                        mons[x++] = as->chan;
                                        mons[x++] = as->chan;
-                               else
+                               } else {
                                        ast_log(LOG_WARNING, "Exceeded maximum number of automatic monitoring events.  Fix autoservice.c\n");
                                        ast_log(LOG_WARNING, "Exceeded maximum number of automatic monitoring events.  Fix autoservice.c\n");
+                               }
                        }
                }
 
                AST_LIST_UNLOCK(&aslist);
 
                        }
                }
 
                AST_LIST_UNLOCK(&aslist);
 
-               if ((chan = ast_waitfor_n(mons, x, &ms))) {
-                       struct ast_frame *f = ast_read(chan);
-       
-                       if (!f) {
-                               struct ast_frame hangup_frame = { 0, };
-                               /* No frame means the channel has been hung up.
-                                * A hangup frame needs to be queued here as ast_waitfor() may
-                                * never return again for the condition to be detected outside
-                                * of autoservice.  So, we'll leave a HANGUP queued up so the
-                                * thread in charge of this channel will know. */
-
-                               hangup_frame.frametype = AST_FRAME_CONTROL;
-                               hangup_frame.subclass = AST_CONTROL_HANGUP;
+               chan = ast_waitfor_n(mons, x, &ms);
+               if (!chan) {
+                       continue;
+               }
 
 
-                               defer_frame(chan, &hangup_frame);
+               f = ast_read(chan);
+       
+               if (!f) {
+                       struct ast_frame hangup_frame = { 0, };
+                       /* No frame means the channel has been hung up.
+                        * A hangup frame needs to be queued here as ast_waitfor() may
+                        * never return again for the condition to be detected outside
+                        * of autoservice.  So, we'll leave a HANGUP queued up so the
+                        * thread in charge of this channel will know. */
+
+                       hangup_frame.frametype = AST_FRAME_CONTROL;
+                       hangup_frame.subclass = AST_CONTROL_HANGUP;
+
+                       defer_frame = &hangup_frame;
+               } else {
 
 
-                               continue;
-                       }
-                       
                        /* Do not add a default entry in this switch statement.  Each new
                         * frame type should be addressed directly as to whether it should
                         * be queued up or not. */
                        /* Do not add a default entry in this switch statement.  Each new
                         * frame type should be addressed directly as to whether it should
                         * be queued up or not. */
+
                        switch (f->frametype) {
                        /* Save these frames */
                        case AST_FRAME_DTMF_END:
                        switch (f->frametype) {
                        /* Save these frames */
                        case AST_FRAME_DTMF_END:
@@ -138,7 +133,7 @@ static void *autoservice_run(void *ign)
                        case AST_FRAME_TEXT:
                        case AST_FRAME_IMAGE:
                        case AST_FRAME_HTML:
                        case AST_FRAME_TEXT:
                        case AST_FRAME_IMAGE:
                        case AST_FRAME_HTML:
-                               defer_frame(chan, f);
+                               defer_frame = f;
                                break;
 
                        /* Throw these frames away */
                                break;
 
                        /* Throw these frames away */
@@ -151,9 +146,31 @@ static void *autoservice_run(void *ign)
                        case AST_FRAME_MODEM:
                                break;
                        }
                        case AST_FRAME_MODEM:
                                break;
                        }
+               }
 
 
-                       if (f)
+               if (!defer_frame) {
+                       if (f) {
                                ast_frfree(f);
                                ast_frfree(f);
+                       }
+                       continue;
+               }
+
+               for (i = 0; i < x; i++) {
+                       struct ast_frame *dup_f;
+
+                       if (mons[i] != chan) {
+                               continue;
+                       }
+
+                       if ((dup_f = ast_frdup(f))) {
+                               AST_LIST_INSERT_TAIL(&ents[i]->deferred_frames, dup_f, frame_list);
+                       }
+                       
+                       break;
+               }
+
+               if (f) {
+                       ast_frfree(f);
                }
        }
 
                }
        }
 
@@ -224,15 +241,10 @@ int ast_autoservice_start(struct ast_channel *chan)
 int ast_autoservice_stop(struct ast_channel *chan)
 {
        int res = -1;
 int ast_autoservice_stop(struct ast_channel *chan)
 {
        int res = -1;
-       struct asent *as;
-       AST_LIST_HEAD_NOLOCK(, ast_frame) dtmf_frames;
+       struct asent *as, *removed = NULL;
        struct ast_frame *f;
        struct ast_frame *f;
-       int removed = 0;
-       int orig_end_dtmf_flag = 0;
        int chan_list_state;
 
        int chan_list_state;
 
-       AST_LIST_HEAD_INIT_NOLOCK(&dtmf_frames);
-
        AST_LIST_LOCK(&aslist);
 
        /* Save the autoservice channel list state.  We _must_ verify that the channel
        AST_LIST_LOCK(&aslist);
 
        /* Save the autoservice channel list state.  We _must_ verify that the channel
@@ -241,41 +253,52 @@ int ast_autoservice_stop(struct ast_channel *chan)
         * it after its gone! */
        chan_list_state = as_chan_list_state;
 
         * it after its gone! */
        chan_list_state = as_chan_list_state;
 
+       /* Find the entry, but do not free it because it still can be in the
+          autoservice thread array */
        AST_LIST_TRAVERSE_SAFE_BEGIN(&aslist, as, list) {       
                if (as->chan == chan) {
        AST_LIST_TRAVERSE_SAFE_BEGIN(&aslist, as, list) {       
                if (as->chan == chan) {
-                       AST_LIST_REMOVE_CURRENT(list);
                        as->use_count--;
                        as->use_count--;
-                       if (as->use_count)
-                               break;
-                       AST_LIST_APPEND_LIST(&dtmf_frames, &as->dtmf_frames, frame_list);
-                       orig_end_dtmf_flag = as->orig_end_dtmf_flag;
-                       ast_free(as);
-                       removed = 1;
-                       if (!ast_check_hangup(chan))
-                               res = 0;
+                       if (as->use_count < 1) {
+                               AST_LIST_REMOVE_CURRENT(list);
+                               removed = as;
+                       }
                        break;
                }
        }
        AST_LIST_TRAVERSE_SAFE_END;
 
                        break;
                }
        }
        AST_LIST_TRAVERSE_SAFE_END;
 
-       if (removed && asthread != AST_PTHREADT_NULL) 
+       if (removed && asthread != AST_PTHREADT_NULL) {
                pthread_kill(asthread, SIGURG);
                pthread_kill(asthread, SIGURG);
-       
+       }
+
        AST_LIST_UNLOCK(&aslist);
 
        AST_LIST_UNLOCK(&aslist);
 
-       if (!removed)
+       if (!removed) {
                return 0;
                return 0;
+       }
+
+       /* Wait while autoservice thread rebuilds its list. */
+       while (chan_list_state == as_chan_list_state) {
+               usleep(1000);
+       }
+
+       /* Now autoservice thread should have no references to our entry
+          and we can safely destroy it */
+
+       if (!chan->_softhangup) {
+               res = 0;
+       }
 
 
-       if (!orig_end_dtmf_flag)
+       if (!as->orig_end_dtmf_flag) {
                ast_clear_flag(chan, AST_FLAG_END_DTMF_ONLY);
                ast_clear_flag(chan, AST_FLAG_END_DTMF_ONLY);
+       }
 
 
-       while ((f = AST_LIST_REMOVE_HEAD(&dtmf_frames, frame_list))) {
+       while ((f = AST_LIST_REMOVE_HEAD(&as->deferred_frames, frame_list))) {
                ast_queue_frame(chan, f);
                ast_frfree(f);
        }
 
                ast_queue_frame(chan, f);
                ast_frfree(f);
        }
 
-       while (chan_list_state == as_chan_list_state)
-               usleep(1000);
+       free(as);
 
        return res;
 }
 
        return res;
 }