From 32bd7a64f98d5e9cf176cba1c701145201b1f987 Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Thu, 10 Mar 2016 12:17:09 -0600 Subject: chan_sip.c: Fix t38id deadlock potential. This patch is part of a series to resolve deadlocks in chan_sip.c. Stopping a scheduled event can result in a deadlock if the scheduled event is running when you try to stop the event. If you hold a lock needed by the scheduled event while trying to stop the scheduled event then a deadlock can happen. The general strategy for resolving the deadlock potential is to push the actual starting and stopping of the scheduled events off onto the scheduler/do_monitor() thread by scheduling an immediate one shot scheduled event. Some restructuring may be needed because the code may assume that the start/stop of the scheduled events is immediate. ASTERISK-25023 Change-Id: If595e4456cd059d7171880c7f354e844c21b5f5f --- channels/chan_sip.c | 103 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 81 insertions(+), 22 deletions(-) diff --git a/channels/chan_sip.c b/channels/chan_sip.c index f369df484..9f6ee21db 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -1512,6 +1512,7 @@ static void stop_provisional_keepalive(struct sip_pvt *pvt); static void do_stop_session_timer(struct sip_pvt *pvt); static void stop_reinvite_retry(struct sip_pvt *pvt); static void stop_retrans_pkt(struct sip_pkt *pkt); +static void stop_t38_abort_timer(struct sip_pvt *pvt); /*! \brief Definition of this channel for PBX channel registration */ struct ast_channel_tech sip_tech = { @@ -7644,13 +7645,13 @@ static int interpret_t38_parameters(struct sip_pvt *p, const struct ast_control_ /* Negotiation can not take place without a valid max_ifp value. */ if (!parameters->max_ifp) { if (p->t38.state == T38_PEER_REINVITE) { - AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr")); + stop_t38_abort_timer(p); transmit_response_reliable(p, "488 Not acceptable here", &p->initreq); } change_t38_state(p, T38_REJECTED); break; } else if (p->t38.state == T38_PEER_REINVITE) { - AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr")); + stop_t38_abort_timer(p); p->t38.our_parms = *parameters; /* modify our parameters to conform to the peer's parameters, * based on the rules in the ITU T.38 recommendation @@ -7684,7 +7685,7 @@ static int interpret_t38_parameters(struct sip_pvt *p, const struct ast_control_ case AST_T38_REFUSED: case AST_T38_REQUEST_TERMINATE: /* Shutdown T38 */ if (p->t38.state == T38_PEER_REINVITE) { - AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr")); + stop_t38_abort_timer(p); change_t38_state(p, T38_REJECTED); transmit_response_reliable(p, "488 Not acceptable here", &p->initreq); } else if (p->t38.state == T38_ENABLED) { @@ -7696,7 +7697,7 @@ static int interpret_t38_parameters(struct sip_pvt *p, const struct ast_control_ struct ast_control_t38_parameters parameters = p->t38.their_parms; if (p->t38.state == T38_PEER_REINVITE) { - AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr")); + stop_t38_abort_timer(p); parameters.max_ifp = ast_udptl_get_far_max_ifp(p->udptl); parameters.request_response = AST_T38_REQUEST_NEGOTIATE; if (p->owner) { @@ -25298,27 +25299,89 @@ static int do_magic_pickup(struct ast_channel *channel, const char *extension, c return 0; } -/*! \brief Called to deny a T38 reinvite if the core does not respond to our request */ +/*! + * \brief Called to deny a T38 reinvite if the core does not respond to our request + * + * \note Run by the sched thread. + */ static int sip_t38_abort(const void *data) { - struct sip_pvt *p = (struct sip_pvt *) data; + struct sip_pvt *pvt = (struct sip_pvt *) data; + struct ast_channel *owner; - sip_pvt_lock(p); - /* an application may have taken ownership of the T.38 negotiation on this - * channel while we were waiting to grab the lock... if it did, the scheduler - * id will have been reset to -1, which is our indication that we do *not* - * want to abort the negotiation process + owner = sip_pvt_lock_full(pvt); + pvt->t38id = -1; + + /* + * An application may have taken ownership of the T.38 negotiation + * on the channel while we were waiting to grab the lock. If it + * did, the T.38 state will have been changed. This is our + * indication that we do *not* want to abort the negotiation + * process. */ - if (p->t38id != -1) { - change_t38_state(p, T38_REJECTED); - transmit_response_reliable(p, "488 Not acceptable here", &p->initreq); - p->t38id = -1; - dialog_unref(p, "unref the dialog ptr from sip_t38_abort, because it held a dialog ptr"); + if (pvt->t38.state == T38_PEER_REINVITE) { + /* Still waiting for a response on timeout so reject the offer. */ + change_t38_state(pvt, T38_REJECTED); + transmit_response_reliable(pvt, "488 Not acceptable here", &pvt->initreq); } - sip_pvt_unlock(p); + + if (owner) { + ast_channel_unlock(owner); + ast_channel_unref(owner); + } + sip_pvt_unlock(pvt); + dialog_unref(pvt, "t38id complete"); return 0; } +/* Run by the sched thread. */ +static int __stop_t38_abort_timer(const void *data) +{ + struct sip_pvt *pvt = (void *) data; + + AST_SCHED_DEL_UNREF(sched, pvt->t38id, + dialog_unref(pvt, "Stop scheduled t38id")); + dialog_unref(pvt, "Stop t38id action"); + return 0; +} + +static void stop_t38_abort_timer(struct sip_pvt *pvt) +{ + dialog_ref(pvt, "Stop t38id action"); + if (ast_sched_add(sched, 0, __stop_t38_abort_timer, pvt) < 0) { + /* Uh Oh. Expect bad behavior. */ + dialog_unref(pvt, "Failed to schedule stop t38id action"); + } +} + +/* Run by the sched thread. */ +static int __start_t38_abort_timer(const void *data) +{ + struct sip_pvt *pvt = (void *) data; + + AST_SCHED_DEL_UNREF(sched, pvt->t38id, + dialog_unref(pvt, "Stop scheduled t38id")); + + dialog_ref(pvt, "Schedule t38id"); + pvt->t38id = ast_sched_add(sched, 5000, sip_t38_abort, pvt); + if (pvt->t38id < 0) { + /* Uh Oh. Expect bad behavior. */ + dialog_unref(pvt, "Failed to schedule t38id"); + } + + dialog_unref(pvt, "Start t38id action"); + return 0; +} + +static void start_t38_abort_timer(struct sip_pvt *pvt) +{ + dialog_ref(pvt, "Start t38id action"); + if (ast_sched_add(sched, 0, __start_t38_abort_timer, pvt) < 0) { + /* Uh Oh. Expect bad behavior. */ + dialog_unref(pvt, "Failed to schedule start t38id action"); + } +} + /*! * \brief bare-bones support for SIP UPDATE * @@ -26201,11 +26264,7 @@ static int handle_request_invite(struct sip_pvt *p, struct sip_request *req, str transmit_response(p, "100 Trying", req); if (p->t38.state == T38_PEER_REINVITE) { - if (p->t38id > -1) { - /* reset t38 abort timer */ - AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "remove ref for t38id")); - } - p->t38id = ast_sched_add(sched, 5000, sip_t38_abort, dialog_ref(p, "passing dialog ptr into sched structure based on t38id for sip_t38_abort.")); + start_t38_abort_timer(p); } else if (p->t38.state == T38_ENABLED) { ast_set_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED); transmit_response_with_t38_sdp(p, "200 OK", req, (reinvite ? XMIT_RELIABLE : (req->ignore ? XMIT_UNRELIABLE : XMIT_CRITICAL))); -- cgit v1.2.3