core: Cleanup some channel snapshot staging anomalies.

We shouldn't unlock the channel after starting a snapshot staging because
another thread may interfere and do its own snapshot staging.

* app_dial.c:dial_exec_full() made hold the channel lock while setting up
the outgoing channel staging.  Made hold the channel lock after the called
party answers while updating the caller channel staging.

* chan_sip.c:sip_new() completed the channel staging on off-nominal exit.
Also we need to use ast_hangup() instead of ast_channel_unref() at that
location.

* channel.c:__ast_channel_alloc_ap() added a comment about not needing to
complete the channel snapshot staging on off-nominal exit paths.

* rtp_engine.c:ast_rtp_instance_set_stats_vars() made hold the channel
locks while staging the channels for the stats channel variables.

Change-Id: Iefb6336893163f6447bad65568722ad5d5d8212a

diff --git a/apps/app_dial.c b/apps/app_dial.c
index ba3f3325212f770c57c1effb8eb1f8cf06e6b7c9..250340579f8edcaaa46c52d05c6feb4a550d0c17 100644 (file)
--- a/apps/app_dial.c
+++ b/apps/app_dial.c
@@ -2538,16 +2538,14 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast
                        continue;
                }
 
-               ast_channel_lock(tc);
-               ast_channel_stage_snapshot(tc);
-               ast_channel_unlock(tc);
-
                ast_channel_get_device_name(tc, device_name, sizeof(device_name));
                if (!ignore_cc) {
                        ast_cc_extension_monitor_add_dialstring(chan, tmp->interface, device_name);
                }
 
                ast_channel_lock_both(tc, chan);
+               ast_channel_stage_snapshot(tc);
+
                pbx_builtin_setvar_helper(tc, "DIALEDPEERNUMBER", tmp->number);
 
                /* Setup outgoing SDP to match incoming one */
@@ -2563,7 +2561,6 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast
 
                ast_channel_appl_set(tc, "AppDial");
                ast_channel_data_set(tc, "(Outgoing Line)");
-               ast_channel_publish_snapshot(tc);
 
                memset(ast_channel_whentohangup(tc), 0, sizeof(*ast_channel_whentohangup(tc)));
 
@@ -2788,15 +2785,14 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast
                }
        } else {
                const char *number;
+               const char *name;
                int dial_end_raised = 0;
                int cause = -1;
 
-               if (ast_test_flag64(&opts, OPT_CALLER_ANSWER))
+               if (ast_test_flag64(&opts, OPT_CALLER_ANSWER)) {
                        ast_answer(chan);
+               }
 
-               strcpy(pa.status, "ANSWER");
-               ast_channel_stage_snapshot(chan);
-               pbx_builtin_setvar_helper(chan, "DIALSTATUS", pa.status);
                /* Ah ha!  Someone answered within the desired timeframe.  Of course after this
                   we will always return with -1 so that it is hung up properly after the
                   conversation.  */
@@ -2818,10 +2814,10 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast
                hanguptree(&out_chans, peer, cause >= 0 ? cause : AST_CAUSE_ANSWERED_ELSEWHERE);
 
                /* If appropriate, log that we have a destination channel and set the answer time */
-               if (ast_channel_name(peer))
-                       pbx_builtin_setvar_helper(chan, "DIALEDPEERNAME", ast_channel_name(peer));
 
                ast_channel_lock(peer);
+               name = ast_strdupa(ast_channel_name(peer));
+
                number = pbx_builtin_getvar_helper(peer, "DIALEDPEERNUMBER");
                if (ast_strlen_zero(number)) {
                        number = NULL;
@@ -2829,8 +2825,16 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast
                        number = ast_strdupa(number);
                }
                ast_channel_unlock(peer);
+
                ast_channel_lock(chan);
+               ast_channel_stage_snapshot(chan);
+
+               strcpy(pa.status, "ANSWER");
+               pbx_builtin_setvar_helper(chan, "DIALSTATUS", pa.status);
+
+               pbx_builtin_setvar_helper(chan, "DIALEDPEERNAME", name);
                pbx_builtin_setvar_helper(chan, "DIALEDPEERNUMBER", number);
+
                ast_channel_stage_snapshot_done(chan);
                ast_channel_unlock(chan);
 

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 83f72b9ade5ba09fa3b34dc64c43e30ac3e3401d..15a3b2584a1fea1c9163690ad37d458f9d41d12f 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -8143,7 +8143,9 @@ static struct ast_channel *sip_new(struct sip_pvt *i, int state, const char *tit
                if (!fmt) {
                        ast_log(LOG_WARNING, "No compatible formats could be found for %s\n", ast_channel_name(tmp));
                        ao2_ref(caps, -1);
-                       tmp = ast_channel_unref(tmp);
+                       ast_channel_stage_snapshot_done(tmp);
+                       ast_channel_unlock(tmp);
+                       ast_hangup(tmp);
                        return NULL;
                }
        }

diff --git a/main/channel.c b/main/channel.c
index 28cfa79df9eb863bef449b6e0903610783a08668..3d51651fd5cceb51505a8572c3bde1fb1b2bc99f 100644 (file)
--- a/main/channel.c
+++ b/main/channel.c
@@ -820,7 +820,12 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char
        ast_channel_stage_snapshot(tmp);
 
        if (!(nativeformats = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT))) {
-               /* format capabilities structure allocation failure */
+               /*
+                * Aborting the channel creation.  We do not need to complete staging
+                * the channel snapshot because the channel has not been finalized or
+                * linked into the channels container yet.  Nobody else knows about
+                * this channel nor will anybody ever know about it.
+                */
                return ast_channel_unref(tmp);
        }
        ast_format_cap_append(nativeformats, ast_format_none, 0);
@@ -846,6 +851,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char
 
        if (!(schedctx = ast_sched_context_create())) {
                ast_log(LOG_WARNING, "Channel allocation failed: Unable to create schedule context\n");
+               /* See earlier channel creation abort comment above. */
                return ast_channel_unref(tmp);
        }
        ast_channel_sched_set(tmp, schedctx);
@@ -860,6 +866,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char
                ast_channel_caller(tmp)->id.name.valid = 1;
                ast_channel_caller(tmp)->id.name.str = ast_strdup(cid_name);
                if (!ast_channel_caller(tmp)->id.name.str) {
+                       /* See earlier channel creation abort comment above. */
                        return ast_channel_unref(tmp);
                }
        }
@@ -867,6 +874,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char
                ast_channel_caller(tmp)->id.number.valid = 1;
                ast_channel_caller(tmp)->id.number.str = ast_strdup(cid_num);
                if (!ast_channel_caller(tmp)->id.number.str) {
+                       /* See earlier channel creation abort comment above. */
                        return ast_channel_unref(tmp);
                }
        }
@@ -880,6 +888,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char
        }
 
        if (needqueue && ast_channel_internal_alertpipe_init(tmp)) {
+               /* See earlier channel creation abort comment above. */
                return ast_channel_unref(tmp);
        }
 
@@ -971,20 +980,14 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char
        if (assignedids && (does_id_conflict(assignedids->uniqueid) || does_id_conflict(assignedids->uniqueid2))) {
                ast_channel_internal_errno_set(AST_CHANNEL_ERROR_ID_EXISTS);
                ao2_unlock(channels);
-               /* This is a bit unorthodox, but we can't just call ast_channel_stage_snapshot_done()
-                * because that will result in attempting to publish the channel snapshot. That causes
-                * badness in some places, such as CDRs. So we need to manually clear the flag on the
-                * channel that says that a snapshot is being cleared.
-                */
-               ast_clear_flag(ast_channel_flags(tmp), AST_FLAG_SNAPSHOT_STAGE);
                ast_channel_unlock(tmp);
+               /* See earlier channel creation abort comment above. */
                return ast_channel_unref(tmp);
        }
 
+       /* Finalize and link into the channels container. */
        ast_channel_internal_finalize(tmp);
-
        ast_atomic_fetchadd_int(&chancount, +1);
-
        ao2_link_flags(channels, tmp, OBJ_NOLOCK);
 
        ao2_unlock(channels);

diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 5c7b7a07b58f52d0648eadba30ba53b27f5afe22..0f09e2f8ae093ed9c2a5d6aa3576bbe933b1e78f 100644 (file)
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -1394,16 +1394,16 @@ void ast_rtp_instance_set_stats_vars(struct ast_channel *chan, struct ast_rtp_in
 {
        char quality_buf[AST_MAX_USER_FIELD];
        char *quality;
-       struct ast_channel *bridge = ast_channel_bridge_peer(chan);
+       struct ast_channel *bridge;
 
-       ast_channel_lock(chan);
-       ast_channel_stage_snapshot(chan);
-       ast_channel_unlock(chan);
+       bridge = ast_channel_bridge_peer(chan);
        if (bridge) {
-               ast_channel_lock(bridge);
+               ast_channel_lock_both(chan, bridge);
                ast_channel_stage_snapshot(bridge);
-               ast_channel_unlock(bridge);
+       } else {
+               ast_channel_lock(chan);
        }
+       ast_channel_stage_snapshot(chan);
 
        quality = ast_rtp_instance_get_quality(instance, AST_RTP_INSTANCE_STAT_FIELD_QUALITY,
                quality_buf, sizeof(quality_buf));
@@ -1441,11 +1441,9 @@ void ast_rtp_instance_set_stats_vars(struct ast_channel *chan, struct ast_rtp_in
                }
        }
 
-       ast_channel_lock(chan);
        ast_channel_stage_snapshot_done(chan);
        ast_channel_unlock(chan);
        if (bridge) {
-               ast_channel_lock(bridge);
                ast_channel_stage_snapshot_done(bridge);
                ast_channel_unlock(bridge);
                ast_channel_unref(bridge);