[asterisk-commits] rmudgett: branch 1.4 r288192 - /branches/1.4/channels/chan_iax2.c
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Tue Sep 21 18:56:02 CDT 2010
Author: rmudgett
Date: Tue Sep 21 18:55:58 2010
New Revision: 288192
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=288192
Log:
In chan_iax2.c:schedule_delivery() calls ast_bridged_channel() on an unlocked channel.
Near the beginning of schedule_delivery(), ast_bridged_channel() is called
on iaxs[fr->callno]->owner. However, the channel is not locked, which can
result in ast_bridged_channel() crashing should owner->tech change to a
technology that doesn't implement bridged_channel.
I also fixed the other calls to ast_bridged_channel() in chan_iax2.c since
the owner lock was not held there either.
Converted the existing channel deadlock avoidance to use
iax2_lock_owner(). Using the new function simplified some awkward code.
In the process of fixing the locking on ast_bridged_channel(), I also
found a memory leak in socket_process() for v1.6.2 and v1.8. The local
struct variable ies.vars is not freed on early/abnormal function exits.
(closes issue #17919)
Reported by: rain
Patches:
issue17919_v1.4.patch uploaded by rmudgett (license 664)
issue17919_w_leak_v1.6.2.patch uploaded by rmudgett (license 664)
issue17919_w_leak_v1.8.patch uploaded by rmudgett (license 664)
Review: https://reviewboard.asterisk.org/r/926/
Modified:
branches/1.4/channels/chan_iax2.c
Modified: branches/1.4/channels/chan_iax2.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/channels/chan_iax2.c?view=diff&rev=288192&r1=288191&r2=288192
==============================================================================
--- branches/1.4/channels/chan_iax2.c (original)
+++ branches/1.4/channels/chan_iax2.c Tue Sep 21 18:55:58 2010
@@ -1008,6 +1008,39 @@
.fixup = iax2_fixup,
};
+/*!
+ * \internal
+ * \brief Obtain the owner channel lock if the owner exists.
+ *
+ * \param callno IAX2 call id.
+ *
+ * \note Assumes the iaxsl[callno] lock is already obtained.
+ *
+ * \note
+ * IMPORTANT NOTE!!! Any time this function is used, even if
+ * iaxs[callno] was valid before calling it, it may no longer be
+ * valid after calling it. This function may unlock and lock
+ * the mutex associated with this callno, meaning that another
+ * thread may grab it and destroy the call.
+ *
+ * \return Nothing
+ */
+static void iax2_lock_owner(int callno)
+{
+ for (;;) {
+ if (!iaxs[callno] || !iaxs[callno]->owner) {
+ /* There is no owner lock to get. */
+ break;
+ }
+ if (!ast_mutex_trylock(&iaxs[callno]->owner->lock)) {
+ /* We got the lock */
+ break;
+ }
+ /* Avoid deadlock by pausing and trying again */
+ DEADLOCK_AVOIDANCE(&iaxsl[callno]);
+ }
+}
+
/* WARNING: insert_idle_thread should only ever be called within the
* context of an iax2_process_thread() thread.
*/
@@ -2564,18 +2597,10 @@
*/
static int iax2_queue_frame(int callno, struct ast_frame *f)
{
- for (;;) {
- if (iaxs[callno] && iaxs[callno]->owner) {
- if (ast_mutex_trylock(&iaxs[callno]->owner->lock)) {
- /* Avoid deadlock by pausing and trying again */
- DEADLOCK_AVOIDANCE(&iaxsl[callno]);
- } else {
- ast_queue_frame(iaxs[callno]->owner, f);
- ast_mutex_unlock(&iaxs[callno]->owner->lock);
- break;
- }
- } else
- break;
+ iax2_lock_owner(callno);
+ if (iaxs[callno] && iaxs[callno]->owner) {
+ ast_queue_frame(iaxs[callno]->owner, f);
+ ast_mutex_unlock(&iaxs[callno]->owner->lock);
}
return 0;
}
@@ -2595,18 +2620,10 @@
*/
static int iax2_queue_hangup(int callno)
{
- for (;;) {
- if (iaxs[callno] && iaxs[callno]->owner) {
- if (ast_mutex_trylock(&iaxs[callno]->owner->lock)) {
- /* Avoid deadlock by pausing and trying again */
- DEADLOCK_AVOIDANCE(&iaxsl[callno]);
- } else {
- ast_queue_hangup(iaxs[callno]->owner);
- ast_mutex_unlock(&iaxs[callno]->owner->lock);
- break;
- }
- } else
- break;
+ iax2_lock_owner(callno);
+ if (iaxs[callno] && iaxs[callno]->owner) {
+ ast_queue_hangup(iaxs[callno]->owner);
+ ast_mutex_unlock(&iaxs[callno]->owner->lock);
}
return 0;
}
@@ -2627,18 +2644,10 @@
static int iax2_queue_control_data(int callno,
enum ast_control_frame_type control, const void *data, size_t datalen)
{
- for (;;) {
- if (iaxs[callno] && iaxs[callno]->owner) {
- if (ast_mutex_trylock(&iaxs[callno]->owner->lock)) {
- /* Avoid deadlock by pausing and trying again */
- DEADLOCK_AVOIDANCE(&iaxsl[callno]);
- } else {
- ast_queue_control_data(iaxs[callno]->owner, control, data, datalen);
- ast_mutex_unlock(&iaxs[callno]->owner->lock);
- break;
- }
- } else
- break;
+ iax2_lock_owner(callno);
+ if (iaxs[callno] && iaxs[callno]->owner) {
+ ast_queue_control_data(iaxs[callno]->owner, control, data, datalen);
+ ast_mutex_unlock(&iaxs[callno]->owner->lock);
}
return 0;
}
@@ -3620,6 +3629,12 @@
return -1;
}
+ iax2_lock_owner(fr->callno);
+ if (!iaxs[fr->callno]) {
+ /* The call dissappeared so discard this frame that we could not send. */
+ iax2_frame_free(fr);
+ return -1;
+ }
if ((owner = iaxs[fr->callno]->owner))
bridge = ast_bridged_channel(owner);
@@ -3627,6 +3642,8 @@
* a channel that can accept jitter, then flush and suspend the jb, and send this frame straight through */
if ( (!ast_test_flag(iaxs[fr->callno], IAX_FORCEJITTERBUF)) && owner && bridge && (bridge->tech->properties & AST_CHAN_TP_WANTSJITTER) ) {
jb_frame frame;
+
+ ast_mutex_unlock(&owner->lock);
/* deliver any frames in the jb */
while (jb_getall(iaxs[fr->callno]->jb, &frame) == JB_OK) {
@@ -3645,6 +3662,9 @@
*tsout = fr->ts;
__do_deliver(fr);
return -1;
+ }
+ if (owner) {
+ ast_mutex_unlock(&owner->lock);
}
/* insert into jitterbuffer */
@@ -8832,14 +8852,11 @@
if (option_debug)
ast_log(LOG_DEBUG, "Ooh, voice format changed to %d\n", f.subclass);
if (iaxs[fr->callno]->owner) {
- int orignative;
-retryowner:
- if (ast_mutex_trylock(&iaxs[fr->callno]->owner->lock)) {
- DEADLOCK_AVOIDANCE(&iaxsl[fr->callno]);
- if (iaxs[fr->callno] && iaxs[fr->callno]->owner) goto retryowner;
- }
+ iax2_lock_owner(fr->callno);
if (iaxs[fr->callno]) {
if (iaxs[fr->callno]->owner) {
+ int orignative;
+
orignative = iaxs[fr->callno]->owner->nativeformats;
iaxs[fr->callno]->owner->nativeformats = f.subclass;
if (iaxs[fr->callno]->owner->readformat)
@@ -8908,22 +8925,32 @@
ast_set_flag(iaxs[fr->callno], IAX_QUELCH);
if (ies.musiconhold) {
- if (iaxs[fr->callno]->owner && ast_bridged_channel(iaxs[fr->callno]->owner)) {
+ iax2_lock_owner(fr->callno);
+ if (!iaxs[fr->callno] || !iaxs[fr->callno]->owner) {
+ break;
+ }
+ if (ast_bridged_channel(iaxs[fr->callno]->owner)) {
const char *mohsuggest = iaxs[fr->callno]->mohsuggest;
+
+ /*
+ * We already hold the owner lock so we do not
+ * need to check iaxs[fr->callno] after it returns.
+ */
iax2_queue_control_data(fr->callno, AST_CONTROL_HOLD,
S_OR(mohsuggest, NULL),
!ast_strlen_zero(mohsuggest) ? strlen(mohsuggest) + 1 : 0);
- if (!iaxs[fr->callno]) {
- ast_mutex_unlock(&iaxsl[fr->callno]);
- return 1;
- }
}
+ ast_mutex_unlock(&iaxs[fr->callno]->owner->lock);
}
}
break;
case IAX_COMMAND_UNQUELCH:
if (ast_test_flag(&iaxs[fr->callno]->state, IAX_STATE_STARTED)) {
- /* Generate Manager Unhold event, if necessary*/
+ iax2_lock_owner(fr->callno);
+ if (!iaxs[fr->callno]) {
+ break;
+ }
+ /* Generate Manager Unhold event, if necessary */
if (iaxs[fr->callno]->owner && ast_test_flag(iaxs[fr->callno], IAX_QUELCH)) {
manager_event(EVENT_FLAG_CALL, "Unhold",
"Channel: %s\r\n"
@@ -8933,13 +8960,17 @@
}
ast_clear_flag(iaxs[fr->callno], IAX_QUELCH);
- if (iaxs[fr->callno]->owner && ast_bridged_channel(iaxs[fr->callno]->owner)) {
+ if (!iaxs[fr->callno]->owner) {
+ break;
+ }
+ if (ast_bridged_channel(iaxs[fr->callno]->owner)) {
+ /*
+ * We already hold the owner lock so we do not
+ * need to check iaxs[fr->callno] after it returns.
+ */
iax2_queue_control_data(fr->callno, AST_CONTROL_UNHOLD, NULL, 0);
- if (!iaxs[fr->callno]) {
- ast_mutex_unlock(&iaxsl[fr->callno]);
- return 1;
- }
}
+ ast_mutex_unlock(&iaxs[fr->callno]->owner->lock);
}
break;
case IAX_COMMAND_TXACC:
@@ -9211,38 +9242,53 @@
case IAX_COMMAND_TRANSFER:
{
struct ast_channel *bridged_chan;
-
- if (iaxs[fr->callno]->owner && (bridged_chan = ast_bridged_channel(iaxs[fr->callno]->owner)) && ies.called_number) {
+ struct ast_channel *owner;
+
+ iax2_lock_owner(fr->callno);
+ if (!iaxs[fr->callno]) {
+ /* Initiating call went away before we could transfer. */
+ break;
+ }
+ owner = iaxs[fr->callno]->owner;
+ bridged_chan = owner ? ast_bridged_channel(owner) : NULL;
+ if (bridged_chan && ies.called_number) {
+ ast_mutex_unlock(&iaxsl[fr->callno]);
+
/* Set BLINDTRANSFER channel variables */
-
- ast_mutex_unlock(&iaxsl[fr->callno]);
- pbx_builtin_setvar_helper(iaxs[fr->callno]->owner, "BLINDTRANSFER", bridged_chan->name);
- ast_mutex_lock(&iaxsl[fr->callno]);
- if (!iaxs[fr->callno]) {
- ast_mutex_unlock(&iaxsl[fr->callno]);
- return 1;
- }
-
- pbx_builtin_setvar_helper(bridged_chan, "BLINDTRANSFER", iaxs[fr->callno]->owner->name);
+ pbx_builtin_setvar_helper(owner, "BLINDTRANSFER", bridged_chan->name);
+ pbx_builtin_setvar_helper(bridged_chan, "BLINDTRANSFER", owner->name);
+
if (!strcmp(ies.called_number, ast_parking_ext())) {
- struct ast_channel *saved_channel = iaxs[fr->callno]->owner;
- ast_mutex_unlock(&iaxsl[fr->callno]);
- if (iax_park(bridged_chan, saved_channel)) {
- ast_log(LOG_WARNING, "Failed to park call on '%s'\n", bridged_chan->name);
- } else {
- ast_log(LOG_DEBUG, "Parked call on '%s'\n", bridged_chan->name);
+ ast_log(LOG_DEBUG, "Parking call '%s'\n", bridged_chan->name);
+ if (iax_park(bridged_chan, owner)) {
+ ast_log(LOG_WARNING, "Failed to park call '%s'\n",
+ bridged_chan->name);
}
ast_mutex_lock(&iaxsl[fr->callno]);
} else {
- if (ast_async_goto(bridged_chan, iaxs[fr->callno]->context, ies.called_number, 1))
- ast_log(LOG_WARNING, "Async goto of '%s' to '%s@%s' failed\n", bridged_chan->name,
- ies.called_number, iaxs[fr->callno]->context);
- else
- ast_log(LOG_DEBUG, "Async goto of '%s' to '%s@%s' started\n", bridged_chan->name,
- ies.called_number, iaxs[fr->callno]->context);
+ ast_mutex_lock(&iaxsl[fr->callno]);
+
+ if (iaxs[fr->callno]) {
+ if (ast_async_goto(bridged_chan, iaxs[fr->callno]->context,
+ ies.called_number, 1)) {
+ ast_log(LOG_WARNING,
+ "Async goto of '%s' to '%s@%s' failed\n",
+ bridged_chan->name, ies.called_number,
+ iaxs[fr->callno]->context);
+ } else {
+ ast_log(LOG_DEBUG, "Async goto of '%s' to '%s@%s' started\n",
+ bridged_chan->name, ies.called_number,
+ iaxs[fr->callno]->context);
+ }
+ } else {
+ /* Initiating call went away before we could transfer. */
+ }
}
} else
ast_log(LOG_DEBUG, "Async goto not applicable on call %d\n", fr->callno);
+ if (owner) {
+ ast_mutex_unlock(&owner->lock);
+ }
break;
}
@@ -9279,25 +9325,19 @@
ast_log(LOG_NOTICE, "Rejected call to %s, format 0x%x incompatible with our capability 0x%x.\n", ast_inet_ntoa(sin.sin_addr), iaxs[fr->callno]->peerformat, iaxs[fr->callno]->capability);
} else {
ast_set_flag(&iaxs[fr->callno]->state, IAX_STATE_STARTED);
- if (iaxs[fr->callno]->owner) {
+ iax2_lock_owner(fr->callno);
+ if (iaxs[fr->callno] && iaxs[fr->callno]->owner) {
/* Switch us to use a compatible format */
iaxs[fr->callno]->owner->nativeformats = iaxs[fr->callno]->peerformat;
if (option_verbose > 2)
ast_verbose(VERBOSE_PREFIX_3 "Format for call is %s\n", ast_getformatname(iaxs[fr->callno]->owner->nativeformats));
-retryowner2:
- if (ast_mutex_trylock(&iaxs[fr->callno]->owner->lock)) {
- DEADLOCK_AVOIDANCE(&iaxsl[fr->callno]);
- if (iaxs[fr->callno] && iaxs[fr->callno]->owner) goto retryowner2;
- }
-
- if (iaxs[fr->callno] && iaxs[fr->callno]->owner) {
- /* Setup read/write formats properly. */
- if (iaxs[fr->callno]->owner->writeformat)
- ast_set_write_format(iaxs[fr->callno]->owner, iaxs[fr->callno]->owner->writeformat);
- if (iaxs[fr->callno]->owner->readformat)
- ast_set_read_format(iaxs[fr->callno]->owner, iaxs[fr->callno]->owner->readformat);
- ast_mutex_unlock(&iaxs[fr->callno]->owner->lock);
- }
+
+ /* Setup read/write formats properly. */
+ if (iaxs[fr->callno]->owner->writeformat)
+ ast_set_write_format(iaxs[fr->callno]->owner, iaxs[fr->callno]->owner->writeformat);
+ if (iaxs[fr->callno]->owner->readformat)
+ ast_set_read_format(iaxs[fr->callno]->owner, iaxs[fr->callno]->owner->readformat);
+ ast_mutex_unlock(&iaxs[fr->callno]->owner->lock);
}
}
if (iaxs[fr->callno]) {
More information about the asterisk-commits
mailing list