From: Kinsey Moore Date: Mon, 7 Nov 2011 20:27:38 +0000 (+0000) Subject: Prevent BLF subscriptions from causing deadlocks X-Git-Tag: 1.8.9.0-rc1~101 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dc3ca68670e0af9979ac8dcb9241c4b4acbb43ea;p=thirdparty%2Fasterisk.git Prevent BLF subscriptions from causing deadlocks Fix a locking inversion in sip_send_mwi_to_peer that was causing deadlocks. This function now requires that both the peer and associated pvt be unlocked before it is called for cases where peer and peer->mwipvt form a circular reference. (closes issue ASTERISK-18663) Review: https://reviewboard.asterisk.org/r/1563/ git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8@343621 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 29c7aaff4c..c84131d5d2 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -24345,7 +24345,9 @@ static int handle_request_subscribe(struct sip_pvt *p, struct sip_request *req, p->subscribed = MWI_NOTIFICATION; if (ast_test_flag(&authpeer->flags[1], SIP_PAGE2_SUBSCRIBEMWIONLY)) { + ao2_unlock(p); add_peer_mwi_subs(authpeer); + ao2_lock(p); } if (authpeer->mwipvt && authpeer->mwipvt != p) { /* Destroy old PVT if this is a new one */ /* We only allow one subscription per peer */ @@ -24421,7 +24423,12 @@ static int handle_request_subscribe(struct sip_pvt *p, struct sip_request *req, ast_set_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED); transmit_response(p, "200 OK", req); if (p->relatedpeer) { /* Send first notification */ - sip_send_mwi_to_peer(p->relatedpeer, 0); + struct sip_peer *peer = p->relatedpeer; + ref_peer(peer, "ensure a peer ref is held during MWI sending"); + ao2_unlock(p); + sip_send_mwi_to_peer(peer, 0); + ao2_lock(p); + unref_peer(peer, "release a peer ref now that MWI is sent"); } } else if (p->subscribed != CALL_COMPLETION) { if ((firststate = ast_extension_state(NULL, p->context, p->exten)) < 0) { @@ -25132,36 +25139,49 @@ static int get_cached_mwi(struct sip_peer *peer, int *new, int *old) return in_cache; } -/*! \brief Send message waiting indication to alert peer that they've got voicemail */ +/*! \brief Send message waiting indication to alert peer that they've got voicemail + * \note Both peer and associated sip_pvt must be unlocked prior to calling this function +*/ static int sip_send_mwi_to_peer(struct sip_peer *peer, int cache_only) { /* Called with peerl lock, but releases it */ struct sip_pvt *p; int newmsgs = 0, oldmsgs = 0; + const char *vmexten; + + ao2_lock(peer); + + vmexten = ast_strdupa(peer->vmexten); - if (ast_test_flag((&peer->flags[1]), SIP_PAGE2_SUBSCRIBEMWIONLY) && !peer->mwipvt) + if (ast_test_flag((&peer->flags[1]), SIP_PAGE2_SUBSCRIBEMWIONLY) && !peer->mwipvt) { + ao2_unlock(peer); return 0; + } /* Do we have an IP address? If not, skip this peer */ - if (ast_sockaddr_isnull(&peer->addr) && ast_sockaddr_isnull(&peer->defaddr)) + if (ast_sockaddr_isnull(&peer->addr) && ast_sockaddr_isnull(&peer->defaddr)) { + ao2_unlock(peer); return 0; + } /* Attempt to use cached mwi to get message counts. */ if (!get_cached_mwi(peer, &newmsgs, &oldmsgs) && !cache_only) { /* Fall back to manually checking the mailbox if not cache_only and get_cached_mwi failed */ struct ast_str *mailbox_str = ast_str_alloca(512); peer_mailboxes_to_str(&mailbox_str, peer); + ao2_unlock(peer); ast_app_inboxcount(mailbox_str->str, &newmsgs, &oldmsgs); + ao2_lock(peer); } - ao2_lock(peer); if (peer->mwipvt) { /* Base message on subscription */ - p = dialog_ref(peer->mwipvt, "sip_send_mwi_to_peer: Setting dialog ptr p from peer->mwipvt-- should this be done?"); + p = dialog_ref(peer->mwipvt, "sip_send_mwi_to_peer: Setting dialog ptr p from peer->mwipvt"); + ao2_unlock(peer); } else { + ao2_unlock(peer); /* Build temporary dialog for this message */ if (!(p = sip_alloc(NULL, NULL, 0, SIP_NOTIFY, NULL))) { - ao2_unlock(peer); return -1; } @@ -25175,7 +25195,6 @@ static int sip_send_mwi_to_peer(struct sip_peer *peer, int cache_only) dialog_unlink_all(p); dialog_unref(p, "unref dialog p just created via sip_alloc"); /* sip_destroy(p); */ - ao2_unlock(peer); return 0; } /* Recalculate our side, and recalculate Call ID */ @@ -25183,11 +25202,15 @@ static int sip_send_mwi_to_peer(struct sip_peer *peer, int cache_only) build_via(p); ao2_t_unlink(dialogs, p, "About to change the callid -- remove the old name"); build_callid_pvt(p); + + ao2_lock(peer); if (!ast_strlen_zero(peer->mwi_from)) { ast_string_field_set(p, mwi_from, peer->mwi_from); } else if (!ast_strlen_zero(default_mwi_from)) { ast_string_field_set(p, mwi_from, default_mwi_from); } + ao2_unlock(peer); + ao2_t_link(dialogs, p, "Linking in under new name"); /* Destroy this session after 32 secs */ sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT); @@ -25200,10 +25223,10 @@ static int sip_send_mwi_to_peer(struct sip_peer *peer, int cache_only) /* Send MWI */ ast_set_flag(&p->flags[0], SIP_OUTGOING); /* the following will decrement the refcount on p as it finishes */ - transmit_notify_with_mwi(p, newmsgs, oldmsgs, peer->vmexten); + transmit_notify_with_mwi(p, newmsgs, oldmsgs, vmexten); sip_pvt_unlock(p); dialog_unref(p, "unref dialog ptr p just before it goes out of scope at the end of sip_send_mwi_to_peer."); - ao2_unlock(peer); + return 0; }