Merged revisions 260050 via svnmerge from

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

................
  r260050 | dvossel | 2010-04-29 10:33:27 -0500 (Thu, 29 Apr 2010) | 21 lines

  Merged revisions 260049 via svnmerge from
  https://origsvn.digium.com/svn/asterisk/branches/1.4

  ........
    r260049 | dvossel | 2010-04-29 10:31:02 -0500 (Thu, 29 Apr 2010) | 14 lines

    Fixes crash in audiohook_write_list

    The middle_frame in the audiohook_write_list function was
    being freed if a audiohook manipulator returned a failure.
    This is incorrect logic.  This patch resolves this and
    adds detailed descriptions of how this function should work
    and why manipulator failures must be ignored.

    (closes issue #17052)
    Reported by: dvossel
    Tested by: dvossel

    (closes issue #16196)
    Reported by: atis

    Review: https://reviewboard.asterisk.org/r/623/
  ........
................

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

diff --git a/include/asterisk/audiohook.h b/include/asterisk/audiohook.h
index 0a36bb934ebcd4f57924f0c8176f719b6c4b53b5..dc6c3e1f3b7e8a9fc88a30aad09bbcbe4aea6f5c 100644 (file)
--- a/include/asterisk/audiohook.h
+++ b/include/asterisk/audiohook.h
@@ -73,9 +73,16 @@ struct ast_audiohook;
  * \param chan Channel
  * \param frame Frame of audio to manipulate
  * \param direction Direction frame came from
- * \return Returns 0 on success, -1 on failure
- * \note An audiohook does not have any reference to a private data structure for manipulate types. It is up to the manipulate callback to store this data
- *       via it's own method. An example would be datastores.
+ * \return Returns 0 on success, -1 on failure.
+ * \note An audiohook does not have any reference to a private data structure for manipulate
+ *       types. It is up to the manipulate callback to store this data via it's own method.
+ *       An example would be datastores.
+ * \note The input frame should never be freed or corrupted during a manipulate callback.
+ *       If the callback has the potential to corrupt the frame's data during manipulation,
+ *       local data should be used for the manipulation and only copied to the frame on
+ *       success.
+ * \note A failure return value indicates that the frame was not manipulated and that
+ *       is being returned in its original state.
  */
 typedef int (*ast_audiohook_manipulate_callback)(struct ast_audiohook *audiohook, struct ast_channel *chan, struct ast_frame *frame, enum ast_audiohook_direction direction);
 

diff --git a/main/audiohook.c b/main/audiohook.c
index 7e7f87785bbdb625b10c0eac1300d931b2ccf9db..5f4f025209795c7ff6c9a82e25d5bc929554ffa2 100644 (file)
--- a/main/audiohook.c
+++ b/main/audiohook.c
@@ -572,7 +572,29 @@ static struct ast_frame *dtmf_audiohook_write_list(struct ast_channel *chan, str
        return frame;
 }
 
-/*! \brief Pass an AUDIO frame off to be handled by the audiohook core
+/*!
+ * \brief Pass an AUDIO frame off to be handled by the audiohook core
+ *
+ * \details
+ * This function has 3 ast_frames and 3 parts to handle each.  At the beginning of this
+ * function all 3 frames, start_frame, middle_frame, and end_frame point to the initial
+ * input frame.
+ *
+ * Part_1: Translate the start_frame into SLINEAR audio if it is not already in that
+ *         format.  The result of this part is middle_frame is guaranteed to be in
+ *         SLINEAR format for Part_2.
+ * Part_2: Send middle_frame off to spies and manipulators.  At this point middle_frame is
+ *         either a new frame as result of the translation, or points directly to the start_frame
+ *         because no translation to SLINEAR audio was required.  The result of this part
+ *         is end_frame will be updated to point to middle_frame if any audiohook manipulation
+ *         took place.
+ * Part_3: Translate end_frame's audio back into the format of start frame if necessary.
+ *         At this point if middle_frame != end_frame, we are guaranteed that no manipulation
+ *         took place and middle_frame can be freed as it was translated... If middle_frame was
+ *         not translated and still pointed to start_frame, it would be equal to end_frame as well
+ *         regardless if manipulation took place which would not result in this free.  The result
+ *         of this part is end_frame is guaranteed to be the format of start_frame for the return.
+ *         
  * \param chan Channel that the list is coming off of
  * \param audiohook_list List of audiohooks
  * \param direction Direction frame is coming in from
@@ -587,6 +609,7 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
        struct ast_audiohook *audiohook = NULL;
        int samples = frame->samples;
 
+       /* ---Part_1. translate start_frame to SLINEAR if necessary. */
        /* If the frame coming in is not signed linear we have to send it through the in_translate path */
        if (frame->subclass != AST_FORMAT_SLINEAR) {
                if (in_translate->format != frame->subclass) {
@@ -601,6 +624,7 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
                samples = middle_frame->samples;
        }
 
+       /* ---Part_2: Send middle_frame to spy and manipulator lists.  middle_frame is guaranteed to be SLINEAR here.*/
        /* Queue up signed linear frame to each spy */
        AST_LIST_TRAVERSE_SAFE_BEGIN(&audiohook_list->spy_list, audiohook, list) {
                ast_audiohook_lock(audiohook);
@@ -654,20 +678,21 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
                                audiohook->manipulate_callback(audiohook, chan, NULL, direction);
                                continue;
                        }
-                       /* Feed in frame to manipulation */
+                       /* Feed in frame to manipulation. */
                        if (audiohook->manipulate_callback(audiohook, chan, middle_frame, direction)) {
-                               ast_frfree(middle_frame);
-                               middle_frame = NULL;
+                               /* XXX IGNORE FAILURE */
+
+                               /* If the manipulation fails then the frame will be returned in its original state.
+                                * Since there are potentially more manipulator callbacks in the list, no action should
+                                * be taken here to exit early. */
                        }
                        ast_audiohook_unlock(audiohook);
                }
                AST_LIST_TRAVERSE_SAFE_END
-               if (middle_frame) {
-                       end_frame = middle_frame;
-               }
+               end_frame = middle_frame;
        }
 
-       /* Now we figure out what to do with our end frame (whether to transcode or not) */
+       /* ---Part_3: Decide what to do with the end_frame (whether to transcode or not) */
        if (middle_frame == end_frame) {
                /* Middle frame was modified and became the end frame... let's see if we need to transcode */
                if (end_frame->subclass != start_frame->subclass) {
@@ -692,9 +717,7 @@ static struct ast_frame *audio_audiohook_write_list(struct ast_channel *chan, st
                }
        } else {
                /* No frame was modified, we can just drop our middle frame and pass the frame we got in out */
-               if (middle_frame) {
-                       ast_frfree(middle_frame);
-               }
+               ast_frfree(middle_frame);
        }
 
        return end_frame;