Fix deadlock potential with ast_set_hangupsource() calls.

Calling ast_set_hangupsource() with the channel lock held can result in a
deadlock because the function also locks the bridged channel.

(issue ASTERISK-19537)

(closes issue ASTERISK-19801)
Reported by: Alec Davis

(closes issue AST-891)
Reported by: Guenther Kelleter
Tested by: Guenther Kelleter

........

Merged revisions 368759 from http://svn.asterisk.org/svn/asterisk/branches/1.8

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

diff --git a/channels/chan_dahdi.c b/channels/chan_dahdi.c
index 37ed75db7694ce2d07071a46e45139c83a0078a7..9a464248f01412aa1ed6594d9655f451a23b846d 100644 (file)
--- a/channels/chan_dahdi.c
+++ b/channels/chan_dahdi.c
@@ -8953,13 +8953,18 @@ static struct ast_frame *__dahdi_exception(struct ast_channel *ast)
                f = &p->subs[idx].f;
                return f;
        }
+
        f = dahdi_handle_event(ast);
+       if (!f) {
+               const char *name = ast_strdupa(ast->name);
 
-       /* tell the cdr this zap device hung up */
-       if (f == NULL) {
-               ast_set_hangupsource(ast, ast->name, 0);
+               /* Tell the CDR this DAHDI device hung up */
+               ast_mutex_unlock(&p->lock);
+               ast_channel_unlock(ast);
+               ast_set_hangupsource(ast, name, 0);
+               ast_channel_lock(ast);
+               ast_mutex_lock(&p->lock);
        }
-
        return f;
 }
 

diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index 36369e58c2f9b0972ccb2cd30cfc4ffd93674483..8cd425e0c7010aed069f4fc3969809b1331ee140 100644 (file)
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -9993,11 +9993,20 @@ static void set_hangup_source_and_cause(int callno, unsigned char causecode)
 {
        iax2_lock_owner(callno);
        if (iaxs[callno] && iaxs[callno]->owner) {
+               struct ast_channel *owner;
+               const char *name;
+
+               owner = iaxs[callno]->owner;
                if (causecode) {
-                       iaxs[callno]->owner->hangupcause = causecode;
+                       owner->hangupcause = causecode;
                }
-               ast_set_hangupsource(iaxs[callno]->owner, iaxs[callno]->owner->name, 0);
-               ast_channel_unlock(iaxs[callno]->owner);
+               name = ast_strdupa(owner->name);
+               ast_channel_ref(owner);
+               ast_channel_unlock(owner);
+               ast_mutex_unlock(&iaxsl[callno]);
+               ast_set_hangupsource(owner, name, 0);
+               ast_channel_unref(owner);
+               ast_mutex_lock(&iaxsl[callno]);
        }
 }
 

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 54a3590b8373306bc045887806ca785bd863e717..b4a49b9b9ae53880533ee80006840dec040bb75c 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -20641,6 +20641,41 @@ static void handle_response_publish(struct sip_pvt *p, int resp, const char *res
        }
 }
 
+/*!
+ * \internal
+ * \brief Set hangup source and cause.
+ *
+ * \param p SIP private.
+ * \param cause Hangup cause to queue.  Zero if no cause.
+ *
+ * \pre p and p->owner are locked.
+ *
+ * \return Nothing
+ */
+static void sip_queue_hangup_cause(struct sip_pvt *p, int cause)
+{
+       struct ast_channel *owner = p->owner;
+       const char *name = ast_strdupa(owner->name);
+
+       /* Cannot hold any channel/private locks when calling. */
+       ast_channel_ref(owner);
+       ast_channel_unlock(owner);
+       sip_pvt_unlock(p);
+       ast_set_hangupsource(owner, name, 0);
+       if (cause) {
+               ast_queue_hangup_with_cause(owner, cause);
+       } else {
+               ast_queue_hangup(owner);
+       }
+       ast_channel_unref(owner);
+
+       /* Relock things. */
+       owner = sip_pvt_lock_full(p);
+       if (owner) {
+               ast_channel_unref(owner);
+       }
+}
+
 /*! \brief Handle SIP response to INVITE dialogue */
 static void handle_response_invite(struct sip_pvt *p, int resp, const char *rest, struct sip_request *req, uint32_t seqno)
 {
@@ -20990,16 +21025,14 @@ static void handle_response_invite(struct sip_pvt *p, int resp, const char *rest
                xmitres = transmit_request(p, SIP_ACK, seqno, XMIT_UNRELIABLE, FALSE);
                ast_log(LOG_WARNING, "Received response: \"Forbidden\" from '%s'\n", sip_get_header(&p->initreq, "From"));
                if (!req->ignore && p->owner) {
-                       ast_set_hangupsource(p->owner, p->owner->name, 0);
-                       ast_queue_hangup_with_cause(p->owner, hangup_sip2cause(resp));
+                       sip_queue_hangup_cause(p, hangup_sip2cause(resp));
                }
                break;
 
        case 404: /* Not found */
                xmitres = transmit_request(p, SIP_ACK, seqno, XMIT_UNRELIABLE, FALSE);
                if (p->owner && !req->ignore) {
-                       ast_set_hangupsource(p->owner, p->owner->name, 0);
-                       ast_queue_hangup_with_cause(p->owner, hangup_sip2cause(resp));
+                       sip_queue_hangup_cause(p, hangup_sip2cause(resp));
                }
                break;
 
@@ -24511,11 +24544,10 @@ static int handle_request_cancel(struct sip_pvt *p, struct sip_request *req)
 
        stop_media_flows(p); /* Immediately stop RTP, VRTP and UDPTL as applicable */
        if (p->owner) {
-               ast_set_hangupsource(p->owner, p->owner->name, 0);
-               ast_queue_hangup(p->owner);
-       }
-       else
+               sip_queue_hangup_cause(p, 0);
+       } else {
                sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
+       }
        if (ast_str_strlen(p->initreq.data) > 0) {
                struct sip_pkt *pkt, *prev_pkt;
                /* If the CANCEL we are receiving is a retransmission, and we already have scheduled
@@ -24669,8 +24701,7 @@ static int handle_request_bye(struct sip_pvt *p, struct sip_request *req)
                                ast_queue_hangup_with_cause(p->owner, AST_CAUSE_PROTOCOL_ERROR);
                }
        } else if (p->owner) {
-               ast_set_hangupsource(p->owner, p->owner->name, 0);
-               ast_queue_hangup(p->owner);
+               sip_queue_hangup_cause(p, 0);
                sip_scheddestroy_final(p, DEFAULT_TRANS_TIMEOUT);
                ast_debug(3, "Received bye, issuing owner hangup\n");
        } else {

diff --git a/channels/sig_analog.c b/channels/sig_analog.c
index b3e75b18b8243ba7b44d95bbb335f4b0ba207b70..fc027303cb0663a09ce20b1b4eba288f98d5a6f6 100644 (file)
--- a/channels/sig_analog.c
+++ b/channels/sig_analog.c
@@ -3623,7 +3623,18 @@ struct ast_frame *analog_exception(struct analog_pvt *p, struct ast_channel *ast
                f = &p->subs[idx].f;
                return f;
        }
+
        f = __analog_handle_event(p, ast);
+       if (!f) {
+               const char *name = ast_strdupa(ast->name);
+
+               /* Tell the CDR this DAHDI device hung up */
+               analog_unlock_private(p);
+               ast_channel_unlock(ast);
+               ast_set_hangupsource(ast, name, 0);
+               ast_channel_lock(ast);
+               analog_lock_private(p);
+       }
        return f;
 }
 

diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index 4e16b5ae7d8f0da77c9cc2fa667db74b5b7ee1cd..1ff14220fb5d2d74c12c7d316e19e1bf16309dc4 100644 (file)
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -1474,6 +1474,8 @@ void ast_channel_clear_softhangup(struct ast_channel *chan, int flag);
  * \param source a string describing the source of the hangup for this channel
  * \param force
  *
+ * \note Absolutely _NO_ channel locks should be held before calling this function.
+ *
  * \since 1.8
  *
  * Hangupsource is generally the channel name that caused the bridge to be

diff --git a/main/channel.c b/main/channel.c
index cf634707bd287c267009b5565d37549f45c84758..0a5b4ca4191745366ab017793d8eb218fa55ae9c 100644 (file)
--- a/main/channel.c
+++ b/main/channel.c
@@ -2773,12 +2773,18 @@ void ast_set_hangupsource(struct ast_channel *chan, const char *source, int forc
                ast_string_field_set(chan, hangupsource, source);
        }
        bridge = ast_bridged_channel(chan);
+       if (bridge) {
+               ast_channel_ref(bridge);
+       }
        ast_channel_unlock(chan);
 
-       if (bridge && (force || ast_strlen_zero(bridge->hangupsource))) {
+       if (bridge) {
                ast_channel_lock(bridge);
-               ast_string_field_set(chan, hangupsource, source);
+               if (force || ast_strlen_zero(bridge->hangupsource)) {
+                       ast_string_field_set(bridge, hangupsource, source);
+               }
                ast_channel_unlock(bridge);
+               ast_channel_unref(bridge);
        }
 }