Add channel lock protection around translation path setup.
authorRichard Mudgett <rmudgett@digium.com>
Fri, 18 Oct 2013 16:59:09 +0000 (16:59 +0000)
committerRichard Mudgett <rmudgett@digium.com>
Fri, 18 Oct 2013 16:59:09 +0000 (16:59 +0000)
Most callers of ast_channel_make_compatible() happen before the channels
enter a two party bridge.  With the new bridging framework, two party
bridging technologies may also call ast_channel_make_compatible() when
there is more than one thread involved with the two channels.

* Added channel lock protection in set_format() and
ast_channel_make_compatible_helper() when dealing with the channel's
native formats while setting up a translation path.

* Fixed best_src_fmt and best_dst_fmt usage consistency in
ast_channel_make_compatible_helper().  The call to
ast_translator_best_choice() got them backwards.

* Updated some callers of ast_channel_make_compatible() and the function
documentation.  There is actually a difference between the two channels
passed in.

* Fixed the deadlock potential in res_fax.c dealing with
ast_channel_make_compatible().  The deadlock potential was already there
anyway because res_fax called ast_channel_make_compatible() with chan
locked.

(closes issue ASTERISK-22542)
Reported by: Matt Jordan

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

Merged revisions 401239 from http://svn.asterisk.org/svn/asterisk/branches/12

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

apps/app_dial.c
include/asterisk/channel.h
main/channel.c
res/res_fax.c

index 2fb7fc9..b995b55 100644 (file)
@@ -870,7 +870,7 @@ static void do_forward(struct chanlist *o, struct cause_args *num,
                c = o->chan = ast_request(tech, ast_channel_nativeformats(in), in, stuff, &cause);
                if (c) {
                        if (single && !caller_entertained) {
-                               ast_channel_make_compatible(o->chan, in);
+                               ast_channel_make_compatible(in, o->chan);
                        }
                        ast_channel_lock_both(in, o->chan);
                        ast_channel_inherit_variables(in, o->chan);
@@ -1070,7 +1070,7 @@ static struct ast_channel *wait_for_answer(struct ast_channel *in,
                        ast_deactivate_generator(in);
                        /* If we are calling a single channel, and not providing ringback or music, */
                        /* then, make them compatible for in-band tone purpose */
-                       if (ast_channel_make_compatible(outgoing->chan, in) < 0) {
+                       if (ast_channel_make_compatible(in, outgoing->chan) < 0) {
                                /* If these channels can not be made compatible,
                                 * there is no point in continuing.  The bridge
                                 * will just fail if it gets that far.
index 56c5baa..7db2b3c 100644 (file)
@@ -1984,14 +1984,23 @@ int ast_readstring_full(struct ast_channel *c, char *s, int len, int timeout, in
 
 
 /*!
- * \brief Makes two channel formats compatible
- * \param c0 first channel to make compatible
- * \param c1 other channel to make compatible
+ * \brief Make the frame formats of two channels compatible.
+ *
+ * \param chan First channel to make compatible.  Should be the calling party.
+ * \param peer Other channel to make compatible.  Should be the called party.
+ *
+ * \note Absolutely _NO_ channel locks should be held before calling this function.
+ *
  * \details
- * Set two channels to compatible formats -- call before ast_channel_bridge in general.
- * \return Returns 0 on success and -1 if it could not be done
+ * Set two channels to compatible frame formats in both
+ * directions.  The path from peer to chan is made compatible
+ * first to allow for in-band audio in case the other direction
+ * cannot be made compatible.
+ *
+ * \retval 0 on success.
+ * \retval -1 on error.
  */
-int ast_channel_make_compatible(struct ast_channel *c0, struct ast_channel *c1);
+int ast_channel_make_compatible(struct ast_channel *chan, struct ast_channel *peer);
 
 /*!
  * \brief Bridge two channels together (early)
index 6846b84..a386154 100644 (file)
@@ -5288,11 +5288,10 @@ static int set_format(struct ast_channel *chan,
        const int direction)
 {
        struct ast_trans_pvt *trans_pvt;
-       struct ast_format_cap *cap_native = ast_channel_nativeformats(chan);
+       struct ast_format_cap *cap_native;
        struct ast_format best_set_fmt;
        struct ast_format best_native_fmt;
        int res;
-       char from[200], to[200];
 
        ast_best_codec(cap_set, &best_set_fmt);
 
@@ -5324,6 +5323,9 @@ static int set_format(struct ast_channel *chan,
                return 0;
        }
 
+       ast_channel_lock(chan);
+       cap_native = ast_channel_nativeformats(chan);
+
        /* Find a translation path from the native format to one of the desired formats */
        if (!direction) {
                /* reading */
@@ -5332,17 +5334,20 @@ static int set_format(struct ast_channel *chan,
                /* writing */
                res = ast_translator_best_choice(cap_native, cap_set, &best_native_fmt, &best_set_fmt);
        }
-
        if (res < 0) {
+               char from[200];
+               char to[200];
+
+               ast_getformatname_multiple(from, sizeof(from), cap_native);
+               ast_channel_unlock(chan);
+               ast_getformatname_multiple(to, sizeof(to), cap_set);
+
                ast_log(LOG_WARNING, "Unable to find a codec translation path from %s to %s\n",
-                       ast_getformatname_multiple(from, sizeof(from), cap_native),
-                       ast_getformatname_multiple(to, sizeof(to), cap_set));
+                       from, to);
                return -1;
        }
 
        /* Now we have a good choice for both. */
-       ast_channel_lock(chan);
-
        if ((ast_format_cmp(rawformat, &best_native_fmt) != AST_FORMAT_CMP_NOT_EQUAL) &&
                (ast_format_cmp(format, &best_set_fmt) != AST_FORMAT_CMP_NOT_EQUAL) &&
                ((ast_format_cmp(rawformat, format) != AST_FORMAT_CMP_NOT_EQUAL) || trans->get(chan))) {
@@ -6144,24 +6149,41 @@ int ast_channel_sendurl(struct ast_channel *chan, const char *url)
 /*! \brief Set up translation from one channel to another */
 static int ast_channel_make_compatible_helper(struct ast_channel *from, struct ast_channel *to)
 {
-       struct ast_format_cap *src_cap = ast_channel_nativeformats(from); /* shallow copy, do not destroy */
-       struct ast_format_cap *dst_cap = ast_channel_nativeformats(to);   /* shallow copy, do not destroy */
+       struct ast_format_cap *src_cap;
+       struct ast_format_cap *dst_cap;
        struct ast_format best_src_fmt;
        struct ast_format best_dst_fmt;
-       int use_slin;
+       int no_path;
+
+       ast_channel_lock_both(from, to);
 
        if ((ast_format_cmp(ast_channel_readformat(from), ast_channel_writeformat(to)) != AST_FORMAT_CMP_NOT_EQUAL) &&
                (ast_format_cmp(ast_channel_readformat(to), ast_channel_writeformat(from)) != AST_FORMAT_CMP_NOT_EQUAL)) {
                /* Already compatible!  Moving on ... */
+               ast_channel_unlock(to);
+               ast_channel_unlock(from);
                return 0;
        }
 
+       src_cap = ast_channel_nativeformats(from); /* shallow copy, do not destroy */
+       dst_cap = ast_channel_nativeformats(to);   /* shallow copy, do not destroy */
+
        /* If there's no audio in this call, don't bother with trying to find a translation path */
-       if (!ast_format_cap_has_type(src_cap, AST_FORMAT_TYPE_AUDIO) || !ast_format_cap_has_type(dst_cap, AST_FORMAT_TYPE_AUDIO))
+       if (!ast_format_cap_has_type(src_cap, AST_FORMAT_TYPE_AUDIO)
+               || !ast_format_cap_has_type(dst_cap, AST_FORMAT_TYPE_AUDIO)) {
+               ast_channel_unlock(to);
+               ast_channel_unlock(from);
                return 0;
+       }
+
+       no_path = ast_translator_best_choice(dst_cap, src_cap, &best_dst_fmt, &best_src_fmt);
 
-       if (ast_translator_best_choice(dst_cap, src_cap, &best_src_fmt, &best_dst_fmt) < 0) {
-               ast_log(LOG_WARNING, "No path to translate from %s to %s\n", ast_channel_name(from), ast_channel_name(to));
+       ast_channel_unlock(to);
+       ast_channel_unlock(from);
+
+       if (no_path) {
+               ast_log(LOG_WARNING, "No path to translate from %s to %s\n",
+                       ast_channel_name(from), ast_channel_name(to));
                return -1;
        }
 
@@ -6171,24 +6193,28 @@ static int ast_channel_make_compatible_helper(struct ast_channel *from, struct a
         * no direct conversion available. If generic PLC is
         * desired, then transcoding via SLINEAR is a requirement
         */
-       use_slin = ast_format_is_slinear(&best_src_fmt) || ast_format_is_slinear(&best_dst_fmt) ? 1 : 0;
-       if ((ast_format_cmp(&best_src_fmt, &best_dst_fmt) == AST_FORMAT_CMP_NOT_EQUAL) &&
-               (ast_opt_generic_plc || ast_opt_transcode_via_slin) &&
-           (ast_translate_path_steps(&best_dst_fmt, &best_src_fmt) != 1 || use_slin)) {
+       if (ast_format_cmp(&best_dst_fmt, &best_src_fmt) == AST_FORMAT_CMP_NOT_EQUAL
+               && (ast_opt_generic_plc || ast_opt_transcode_via_slin)) {
+               int use_slin = (ast_format_is_slinear(&best_src_fmt)
+                       || ast_format_is_slinear(&best_dst_fmt)) ? 1 : 0;
 
-               int best_sample_rate = ast_format_rate(&best_src_fmt) > ast_format_rate(&best_dst_fmt) ?
-                       ast_format_rate(&best_src_fmt) : ast_format_rate(&best_dst_fmt);
+               if (use_slin || ast_translate_path_steps(&best_dst_fmt, &best_src_fmt) != 1) {
+                       int best_sample_rate = (ast_format_rate(&best_src_fmt) > ast_format_rate(&best_dst_fmt)) ?
+                               ast_format_rate(&best_src_fmt) : ast_format_rate(&best_dst_fmt);
 
-               /* pick the best signed linear format based upon what preserves the sample rate the best. */
-               ast_format_set(&best_dst_fmt, ast_format_slin_by_rate(best_sample_rate), 0);
+                       /* pick the best signed linear format based upon what preserves the sample rate the best. */
+                       ast_format_set(&best_src_fmt, ast_format_slin_by_rate(best_sample_rate), 0);
+               }
        }
 
-       if (ast_set_read_format(from, &best_dst_fmt) < 0) {
-               ast_log(LOG_WARNING, "Unable to set read format on channel %s to %s\n", ast_channel_name(from), ast_getformatname(&best_dst_fmt));
+       if (ast_set_read_format(from, &best_src_fmt)) {
+               ast_log(LOG_WARNING, "Unable to set read format on channel %s to %s\n",
+                       ast_channel_name(from), ast_getformatname(&best_src_fmt));
                return -1;
        }
-       if (ast_set_write_format(to, &best_dst_fmt) < 0) {
-               ast_log(LOG_WARNING, "Unable to set write format on channel %s to %s\n", ast_channel_name(to), ast_getformatname(&best_dst_fmt));
+       if (ast_set_write_format(to, &best_src_fmt)) {
+               ast_log(LOG_WARNING, "Unable to set write format on channel %s to %s\n",
+                       ast_channel_name(to), ast_getformatname(&best_src_fmt));
                return -1;
        }
        return 0;
@@ -6196,19 +6222,20 @@ static int ast_channel_make_compatible_helper(struct ast_channel *from, struct a
 
 int ast_channel_make_compatible(struct ast_channel *chan, struct ast_channel *peer)
 {
-       /* Some callers do not check return code, and we must try to set all call legs correctly */
-       int rc = 0;
+       /*
+        * Set up translation from the peer to the chan first in case we
+        * need to hear any in-band tones and the other direction fails.
+        */
+       if (ast_channel_make_compatible_helper(peer, chan)) {
+               return -1;
+       }
 
        /* Set up translation from the chan to the peer */
-       rc = ast_channel_make_compatible_helper(chan, peer);
-
-       if (rc < 0)
-               return rc;
-
-       /* Set up translation from the peer to the chan */
-       rc = ast_channel_make_compatible_helper(peer, chan);
+       if (ast_channel_make_compatible_helper(chan, peer)) {
+               return -1;
+       }
 
-       return rc;
+       return 0;
 }
 
 int ast_channel_masquerade(struct ast_channel *original, struct ast_channel *clonechan)
index 6341a55..55bd3c6 100644 (file)
@@ -3057,12 +3057,12 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct
 
                        ast_channel_unlock(chan);
                        peer = ast_channel_bridge_peer(chan);
-                       ast_channel_lock(chan);
                        if (peer) {
                                ast_set_read_format(peer, &gateway->peer_read_format);
                                ast_set_read_format(peer, &gateway->peer_write_format);
                                ast_channel_make_compatible(chan, peer);
                        }
+                       ast_channel_lock(chan);
                }
                return NULL;
        }
@@ -3084,7 +3084,7 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct
                return f;
        }
 
-       if (!gateway->bridged && peer) {
+       if (!gateway->bridged) {
                /* don't start a gateway if neither channel can handle T.38 */
                if (ast_channel_get_t38_state(chan) == T38_STATE_UNAVAILABLE && ast_channel_get_t38_state(peer) == T38_STATE_UNAVAILABLE) {
                        ast_debug(1, "not starting gateway for %s and %s; neither channel supports T.38\n", ast_channel_name(chan), ast_channel_name(peer));
@@ -3113,10 +3113,12 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct
                ast_set_read_format_by_id(chan, AST_FORMAT_SLINEAR);
                ast_set_write_format_by_id(chan, AST_FORMAT_SLINEAR);
 
+               ast_channel_unlock(chan);
                ast_set_read_format_by_id(peer, AST_FORMAT_SLINEAR);
                ast_set_write_format_by_id(peer, AST_FORMAT_SLINEAR);
 
                ast_channel_make_compatible(chan, peer);
+               ast_channel_lock(chan);
                gateway->bridged = 1;
        }
 
@@ -3381,10 +3383,10 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
                ast_set_read_format(chan, &faxdetect->orig_format);
                ast_channel_unlock(chan);
                peer = ast_channel_bridge_peer(chan);
-               ast_channel_lock(chan);
                if (peer) {
                        ast_channel_make_compatible(chan, peer);
                }
+               ast_channel_lock(chan);
                return NULL;
        case AST_FRAMEHOOK_EVENT_READ:
                if (f) {