diff options
author | Richard Mudgett <rmudgett@digium.com> | 2011-10-11 19:28:23 +0000 |
---|---|---|
committer | Richard Mudgett <rmudgett@digium.com> | 2011-10-11 19:28:23 +0000 |
commit | b63c1cc5456fb9f0a28fc4d99c46075334102280 (patch) | |
tree | 1f76c1f09cab84ff18d3b1220d7cc0bf923154db /channels | |
parent | 1ec8a9d896e9ce2da8ab38d64c2a9b36707145d5 (diff) |
Fix some potential deadlocks pointed out by helgrind.
* Fixed deadlock potential calling dialog_unlink_all() in
__sip_autodestruct(). Found by helgrind.
* Fixed deadlock potential in handle_request_invite() after calling
sip_new(). Found by helgrind.
* The sip_new() function now returns with the created channel already
locked.
* Removed the dead code that starts a PBX in in sip_new(). No sip_new()
callers caused that code to be executed and it was a bad thing to do
anyway.
* Removed unused parameters and return value from dialog_unlink_all().
* Made dialog_unlink_all() and __sip_autodestruct() safely obtain the
owner and private channel locks without a deadlock avoidance loop.
........
Merged revisions 340284 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........
Merged revisions 340310 from http://svn.asterisk.org/svn/asterisk/branches/10
git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@340318 65c4cc65-6c06-0410-ace0-fbb531ad65f3
Diffstat (limited to 'channels')
-rw-r--r-- | channels/chan_sip.c | 121 | ||||
-rw-r--r-- | channels/sip/include/dialog.h | 9 |
2 files changed, 61 insertions, 69 deletions
diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 776421503..06b03d32c 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -1350,6 +1350,7 @@ static int restart_monitor(void); static void peer_mailboxes_to_str(struct ast_str **mailbox_str, struct sip_peer *peer); static struct ast_variable *copy_vars(struct ast_variable *src); static int dialog_find_multiple(void *obj, void *arg, int flags); +static struct ast_channel *sip_pvt_lock_full(struct sip_pvt *pvt); /* static int sip_addrcmp(char *name, struct sockaddr_in *sin); Support for peer matching */ static int sip_refer_allocate(struct sip_pvt *p); static int sip_notify_allocate(struct sip_pvt *p); @@ -2955,16 +2956,10 @@ static void *dialog_unlink_rtpcheck(struct sip_pvt *dialog) return NULL; } -/*! - * \brief Unlink a dialog from the dialogs container, as well as any other places - * that it may be currently stored. - * - * \note A reference to the dialog must be held before calling this function, and this - * function does not release that reference. - */ -void *dialog_unlink_all(struct sip_pvt *dialog, int lockowner, int lockdialoglist) +void dialog_unlink_all(struct sip_pvt *dialog) { struct sip_pkt *cp; + struct ast_channel *owner; dialog_ref(dialog, "Let's bump the count in the unlink so it doesn't accidentally become dead before we are done"); @@ -2973,16 +2968,16 @@ void *dialog_unlink_all(struct sip_pvt *dialog, int lockowner, int lockdialoglis ao2_t_unlink(dialogs_rtpcheck, dialog, "unlinking dialog_rtpcheck via ao2_unlink"); /* Unlink us from the owner (channel) if we have one */ - if (dialog->owner) { - if (lockowner) { - ast_channel_lock(dialog->owner); - } - ast_debug(1, "Detaching from channel %s\n", dialog->owner->name); - dialog->owner->tech_pvt = dialog_unref(dialog->owner->tech_pvt, "resetting channel dialog ptr in unlink_all"); - if (lockowner) { - ast_channel_unlock(dialog->owner); - } + owner = sip_pvt_lock_full(dialog); + if (owner) { + ast_debug(1, "Detaching from channel %s\n", owner->name); + owner->tech_pvt = dialog_unref(owner->tech_pvt, "resetting channel dialog ptr in unlink_all"); + ast_channel_unlock(owner); + ast_channel_unref(owner); + dialog->owner = NULL; } + sip_pvt_unlock(dialog); + if (dialog->registry) { if (dialog->registry->call == dialog) { dialog->registry->call = dialog_unref(dialog->registry->call, "nulling out the registry's call dialog field in unlink_all"); @@ -3036,7 +3031,6 @@ void *dialog_unlink_all(struct sip_pvt *dialog, int lockowner, int lockdialoglis } dialog_unref(dialog, "Let's unbump the count in the unlink so the poor pvt can disappear if it is time"); - return NULL; } void *registry_unref(struct sip_registry *reg, char *tag) @@ -3852,6 +3846,7 @@ static enum sip_result __sip_reliable_xmit(struct sip_pvt *p, int seqno, int res static int __sip_autodestruct(const void *data) { struct sip_pvt *p = (struct sip_pvt *)data; + struct ast_channel *owner; /* If this is a subscription, tell the phone that we got a timeout */ if (p->subscribed && p->subscribed != MWI_NOTIFICATION && p->subscribed != CALL_COMPLETION) { @@ -3887,17 +3882,12 @@ static int __sip_autodestruct(const void *data) /* * Lock both the pvt and the channel safely so that we can queue up a frame. */ - sip_pvt_lock(p); - while (p->owner && ast_channel_trylock(p->owner)) { - sip_pvt_unlock(p); - sched_yield(); - sip_pvt_lock(p); - } - - if (p->owner) { + owner = sip_pvt_lock_full(p); + if (owner) { ast_log(LOG_WARNING, "Autodestruct on dialog '%s' with owner in place (Method: %s)\n", p->callid, sip_methods[p->method].text); - ast_queue_hangup_with_cause(p->owner, AST_CAUSE_PROTOCOL_ERROR); - ast_channel_unlock(p->owner); + ast_queue_hangup_with_cause(owner, AST_CAUSE_PROTOCOL_ERROR); + ast_channel_unlock(owner); + ast_channel_unref(owner); } else if (p->refer && !p->alreadygone) { ast_debug(3, "Finally hanging up channel after transfer: %s\n", p->callid); transmit_request_with_auth(p, SIP_BYE, 0, XMIT_RELIABLE, 1); @@ -3906,7 +3896,9 @@ static int __sip_autodestruct(const void *data) } else { append_history(p, "AutoDestroy", "%s", p->callid); ast_debug(3, "Auto destroying SIP dialog '%s'\n", p->callid); - dialog_unlink_all(p, TRUE, TRUE); /* once it's unlinked and unrefd everywhere, it'll be freed automagically */ + sip_pvt_unlock(p); + dialog_unlink_all(p); /* once it's unlinked and unrefd everywhere, it'll be freed automagically */ + sip_pvt_lock(p); /* dialog_unref(p, "unref dialog-- no other matching conditions"); -- unlink all now should finish off the dialog's references and free it. */ /* sip_destroy(p); */ /* Go ahead and destroy dialog. All attempts to recover is done */ /* sip_destroy also absorbs the reference */ @@ -4603,12 +4595,12 @@ static void sip_destroy_peer(struct sip_peer *peer) /* Delete it, it needs to disappear */ if (peer->call) { - dialog_unlink_all(peer->call, TRUE, TRUE); + dialog_unlink_all(peer->call); peer->call = dialog_unref(peer->call, "peer->call is being unset"); } if (peer->mwipvt) { /* We have an active subscription, delete it */ - dialog_unlink_all(peer->mwipvt, TRUE, TRUE); + dialog_unlink_all(peer->mwipvt); peer->mwipvt = dialog_unref(peer->mwipvt, "unreffing peer->mwipvt"); } @@ -5590,7 +5582,7 @@ static void sip_registry_destroy(struct sip_registry *reg) we don't get reentered trying to grab the registry lock */ reg->call->registry = registry_unref(reg->call->registry, "destroy reg->call->registry"); ast_debug(3, "Destroying active SIP dialog for registry %s@%s\n", reg->username, reg->hostname); - dialog_unlink_all(reg->call, TRUE, TRUE); + dialog_unlink_all(reg->call); reg->call = dialog_unref(reg->call, "unref reg->call"); /* reg->call = sip_destroy(reg->call); */ } @@ -6842,11 +6834,17 @@ static int sip_indicate(struct ast_channel *ast, int condition, const void *data return res; } -/*! \brief Initiate a call in the SIP channel - called from sip_request_call (calls from the pbx ) for outbound channels - and from handle_request_invite for inbound channels - -*/ +/*! + * \brief Initiate a call in the SIP channel + * + * \note called from sip_request_call (calls from the pbx ) for + * outbound channels and from handle_request_invite for inbound + * channels + * + * \pre i is locked + * + * \return New ast_channel locked. + */ static struct ast_channel *sip_new(struct sip_pvt *i, int state, const char *title, const char *linkedid) { struct ast_channel *tmp; @@ -7050,15 +7048,6 @@ static struct ast_channel *sip_new(struct sip_pvt *i, int state, const char *tit pbx_builtin_setvar_helper(tmp, v->name, ast_get_encoded_str(v->value, valuebuf, sizeof(valuebuf))); } - ast_channel_unlock(tmp); /* ast_hangup requires the channel to be unlocked */ - - if (state != AST_STATE_DOWN && ast_pbx_start(tmp)) { - ast_log(LOG_WARNING, "Unable to start PBX on %s\n", tmp->name); - tmp->hangupcause = AST_CAUSE_SWITCH_CONGESTION; - ast_hangup(tmp); - tmp = NULL; - } - if (i->do_history) { append_history(i, "NewChan", "Channel %s - from %s", tmp->name, i->callid); } @@ -7886,7 +7875,7 @@ static void forked_invite_init(struct sip_request *req, const char *new_theirtag * \note This function will never let you down. * \note This function will run around and desert you. * - * \pre vpt is not locked + * \pre pvt is not locked * \post pvt is locked * \post pvt->owner is locked and its reference count is increased (if pvt->owner is not NULL) * @@ -12068,7 +12057,7 @@ static int transmit_publish(struct sip_epa_entry *epa_entry, enum sip_publish_ty if (create_addr(pvt, epa_entry->destination, NULL, TRUE, NULL)) { sip_pvt_unlock(pvt); - dialog_unlink_all(pvt, TRUE, TRUE); + dialog_unlink_all(pvt); dialog_unref(pvt, "create_addr failed in transmit_publish. Unref dialog"); return -1; } @@ -12310,7 +12299,7 @@ static int __sip_subscribe_mwi_do(struct sip_subscription_mwi *mwi) /* Setup the destination of our subscription */ if (create_addr(mwi->call, mwi->hostname, &mwi->us, 0, NULL)) { - dialog_unlink_all(mwi->call, TRUE, TRUE); + dialog_unlink_all(mwi->call); mwi->call = dialog_unref(mwi->call, "unref dialog after unlink_all"); return 0; } @@ -12765,7 +12754,7 @@ static int manager_sipnotify(struct mansession *s, const struct message *m) if (create_addr(p, channame, NULL, 0, NULL)) { /* Maybe they're not registered, etc. */ - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); dialog_unref(p, "unref dialog inside for loop" ); /* sip_destroy(p); */ astman_send_error(s, m, "Could not create address"); @@ -13104,7 +13093,7 @@ static int transmit_register(struct sip_registry *r, int sipmethod, const char * if (create_addr(p, S_OR(r->peername, r->hostname), &r->us, 0, NULL)) { /* we have what we hope is a temporary network error, * probably DNS. We need to reschedule a registration try */ - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); p = dialog_unref(p, "unref dialog after unlink_all"); if (r->timeout > -1) { AST_SCHED_REPLACE_UNREF(r->timeout, sched, global_reg_timeout * 1000, sip_reg_timeout, r, @@ -16993,7 +16982,7 @@ static int dialog_needdestroy(void *dialogobj, void *arg, int flags) sip_pvt_unlock(dialog); /* no, the unlink should handle this: dialog_unref(dialog, "needdestroy: one more refcount decrement to allow dialog to be destroyed"); */ /* the CMP_MATCH will unlink this dialog from the dialog hash table */ - dialog_unlink_all(dialog, TRUE, FALSE); + dialog_unlink_all(dialog); return 0; /* the unlink_all should unlink this from the table, so.... no need to return a match */ } @@ -19013,7 +19002,7 @@ static char *sip_cli_notify(struct ast_cli_entry *e, int cmd, struct ast_cli_arg if (create_addr(p, a->argv[i], NULL, 1, NULL)) { /* Maybe they're not registered, etc. */ - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); dialog_unref(p, "unref dialog inside for loop" ); /* sip_destroy(p); */ ast_cli(a->fd, "Could not create address for '%s'\n", a->argv[i]); @@ -22777,8 +22766,6 @@ static int handle_request_invite(struct sip_pvt *p, struct sip_request *req, int if (c) { ast_party_redirecting_init(&redirecting); memset(&update_redirecting, 0, sizeof(update_redirecting)); - /* Pre-lock the call */ - ast_channel_lock(c); change_redirecting_information(p, req, &redirecting, &update_redirecting, FALSE); /*Will return immediately if no Diversion header is present */ ast_channel_set_redirecting(c, &redirecting, &update_redirecting); @@ -24040,7 +24027,7 @@ static int sip_msg_send(const struct ast_msg *msg, const char *to, const char *f } if (ast_strlen_zero(peer)) { ast_log(LOG_WARNING, "MESSAGE(to) is invalid for SIP - '%s'\n", to); - dialog_unlink_all(pvt, TRUE, TRUE); + dialog_unlink_all(pvt); dialog_unref(pvt, "MESSAGE(to) is invalid for SIP"); return -1; } @@ -24073,7 +24060,7 @@ static int sip_msg_send(const struct ast_msg *msg, const char *to, const char *f if (create_addr(pvt, peer, NULL, TRUE, NULL)) { sip_pvt_unlock(pvt); - dialog_unlink_all(pvt, TRUE, TRUE); + dialog_unlink_all(pvt); dialog_unref(pvt, "create_addr failed sending a MESSAGE"); return -1; } @@ -24925,7 +24912,7 @@ static int handle_request_subscribe(struct sip_pvt *p, struct sip_request *req, } if (authpeer->mwipvt && authpeer->mwipvt != p) { /* Destroy old PVT if this is a new one */ /* We only allow one subscription per peer */ - dialog_unlink_all(authpeer->mwipvt, TRUE, TRUE); + dialog_unlink_all(authpeer->mwipvt); authpeer->mwipvt = dialog_unref(authpeer->mwipvt, "unref dialog authpeer->mwipvt"); /* sip_destroy(authpeer->mwipvt); */ } @@ -25780,7 +25767,7 @@ static int sip_send_mwi_to_peer(struct sip_peer *peer, int cache_only) set_socket_transport(&p->socket, 0); if (create_addr_from_peer(p, peer)) { /* Maybe they're not registered, etc. */ - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); dialog_unref(p, "unref dialog p just created via sip_alloc"); /* sip_destroy(p); */ ao2_unlock(peer); @@ -26343,7 +26330,7 @@ static int sip_poke_noanswer(const void *data) } if (peer->call) { - dialog_unlink_all(peer->call, TRUE, TRUE); + dialog_unlink_all(peer->call); peer->call = dialog_unref(peer->call, "unref dialog peer->call"); /* peer->call = sip_destroy(peer->call);*/ } @@ -26393,7 +26380,7 @@ static int sip_poke_peer(struct sip_peer *peer, int force) if (sipdebug) { ast_log(LOG_NOTICE, "Still have a QUALIFY dialog active, deleting\n"); } - dialog_unlink_all(peer->call, TRUE, TRUE); + dialog_unlink_all(peer->call); peer->call = dialog_unref(peer->call, "unref dialog peer->call"); /* peer->call = sip_destroy(peer->call); */ } @@ -26621,7 +26608,7 @@ static struct ast_channel *sip_request_call(const char *type, struct ast_format_ ast_string_field_set(p, dialstring, dialstring); if (!(p->options = ast_calloc(1, sizeof(*p->options)))) { - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); dialog_unref(p, "unref dialog p from mem fail"); /* sip_destroy(p); */ ast_log(LOG_ERROR, "Unable to build option SIP data structure - Out of memory\n"); @@ -26710,7 +26697,7 @@ static struct ast_channel *sip_request_call(const char *type, struct ast_format_ if (create_addr(p, host, NULL, 1, &remote_address_sa)) { *cause = AST_CAUSE_UNREGISTERED; ast_debug(3, "Cant create SIP call - target device not registered\n"); - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); dialog_unref(p, "unref dialog p UNREGISTERED"); /* sip_destroy(p); */ return NULL; @@ -26755,8 +26742,10 @@ static struct ast_channel *sip_request_call(const char *type, struct ast_format_ p->owner? p->owner->name : "", "SIP", p->callid, p->fullcontact, p->peername); sip_pvt_unlock(p); if (!tmpc) { - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); /* sip_destroy(p); */ + } else { + ast_channel_unlock(tmpc); } dialog_unref(p, "toss pvt ptr at end of sip_request_call"); ast_update_use_count(); @@ -28116,7 +28105,7 @@ static int reload_config(enum channelreloadreason reason) if (iterator->call) { ast_debug(3, "Destroying active SIP dialog for registry %s@%s\n", iterator->username, iterator->hostname); /* This will also remove references to the registry */ - dialog_unlink_all(iterator->call, TRUE, TRUE); + dialog_unlink_all(iterator->call); iterator->call = dialog_unref(iterator->call, "remove iterator->call from registry traversal"); } if (iterator->expire > -1) { @@ -30634,7 +30623,7 @@ static int unload_module(void) /* Destroy all the dialogs and free their memory */ i = ao2_iterator_init(dialogs, 0); while ((p = ao2_t_iterator_next(&i, "iterate thru dialogs"))) { - dialog_unlink_all(p, TRUE, TRUE); + dialog_unlink_all(p); ao2_t_ref(p, -1, "throw away iterator result"); } ao2_iterator_destroy(&i); diff --git a/channels/sip/include/dialog.h b/channels/sip/include/dialog.h index ed31b7774..d4024c068 100644 --- a/channels/sip/include/dialog.h +++ b/channels/sip/include/dialog.h @@ -57,10 +57,13 @@ void __sip_destroy(struct sip_pvt *p, int lockowner, int lockdialoglist); * \brief Unlink a dialog from the dialogs container, as well as any other places * that it may be currently stored. * - * \note A reference to the dialog must be held before calling this function, and this - * function does not release that reference. + * \note A reference to the dialog must be held before calling + * this function, and this function does not release that + * reference. + * + * \note The dialog must not be locked when called. */ -void *dialog_unlink_all(struct sip_pvt *dialog, int lockowner, int lockdialoglist); +void dialog_unlink_all(struct sip_pvt *dialog); /*! \brief Acknowledges receipt of a packet and stops retransmission * called with p locked*/ |