[Asterisk-code-review] bridge.c: Crash during attended transfer when missing a loca... (asterisk[13])
Kevin Harwell
asteriskteam at digium.com
Tue Mar 1 17:03:08 CST 2016
Kevin Harwell has uploaded a new change for review.
https://gerrit.asterisk.org/2319
Change subject: bridge.c: Crash during attended transfer when missing a local channel half
......................................................................
bridge.c: Crash during attended transfer when missing a local channel half
It's possible for the transferer channel to get hung up early during the
attended transfer process. For instance, a phone may send a "bye" immediately
upon receiving a sip notify that contains a sip frag 100 (I'm looking at you
Jitsi). When this occurs a race begins between the transferer being hung up
and completion of the transfer code.
If the channel hangs up too early during a transfer involving stasis bridging
for instance, then when the created local channel goes to look up its swap
channel (and associated datastore) it can't find it (since it is no longer in
the bridge) thus if fails to enter the stasis application. Consequently, the
created local channel(s) hang up as well. If the timing is just right then the
bridging code attempts to add the message link with missing local channel(s).
Hence the crash.
Unfortunately, there is no great way to solve the problem of the unexpected
"bye". While we can't guarantee we won't receive an early hangup, and in this
case still fail to enter the stasis application, we can make it so asterisk
does not crash.
This patch does just that by locking the local channel structure, checking
that the local channel's peer has not been lost, and then continuing. This
keeps the local channel's peer from being ripped out from underneath it by
the local/unreal hangup code while attempting to set the stasis message link.
ASTERISK-25771
Change-Id: Ie6d6061e34c7c95f07116fffac9a09e5d225c880
---
M include/asterisk/core_local.h
M main/bridge.c
M main/core_local.c
3 files changed, 85 insertions(+), 12 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/19/2319/1
diff --git a/include/asterisk/core_local.h b/include/asterisk/core_local.h
index 491112d..8557072 100644
--- a/include/asterisk/core_local.h
+++ b/include/asterisk/core_local.h
@@ -42,6 +42,38 @@
/* ------------------------------------------------------------------- */
/*!
+ * \brief Lock the "chan" and "owner" channels (and return them) on the base
+ * private structure as well as the base private structure itself.
+ *
+ * \note This also adds references to each of the above mentioned elements and
+ * also the underlying private local structure.
+ * \note None of these locks should be held prior to calling this function.
+ * \note To undo this process call ast_local_unlock_all.
+ *
+ * \since 13.8.0
+ *
+ * \param chan Must be a local channel
+ * \param outchan The local channel's "chan" channel
+ * \param outowner The local channel's "owner" channel
+ */
+void ast_local_lock_all(struct ast_channel *chan, struct ast_channel **outchan,
+ struct ast_channel **outowner);
+
+/*!
+ * \brief Unlock the "chan" and "owner" channels on the base private structure
+ * as well as the base private structure itself.
+ *
+ * \note This also removes references to each of the above mentioned elements and
+ * also the underlying private local structure.
+ * \note This function should be used in conjunction with ast_local_lock_all.
+ *
+ * \since 13.8.0
+ *
+ * \param chan Must be a local channel
+ */
+void ast_local_unlock_all(struct ast_channel *chan);
+
+/*!
* \brief Get the other local channel in the pair.
* \since 12.0.0
*
diff --git a/main/bridge.c b/main/bridge.c
index 9d8d807..04dbb8a 100644
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -3979,8 +3979,8 @@
} \
} while (0)
+ RAII_VAR(struct ast_channel *, local_chan, NULL, ao2_cleanup);
static const char *dest = "_attended at transfer/m";
- struct ast_channel *local_chan;
int cause;
int res;
const char *app = NULL;
@@ -4005,7 +4005,6 @@
}
if (res) {
- ast_hangup(local_chan);
return AST_BRIDGE_TRANSFER_FAIL;
}
@@ -4029,31 +4028,36 @@
if (ast_bridge_impart(bridge1, local_chan, chan1, NULL,
AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) {
ast_hangup(local_chan);
- ao2_cleanup(local_chan);
BRIDGE_LOCK_ONE_OR_BOTH(bridge1, bridge2);
return AST_BRIDGE_TRANSFER_FAIL;
}
BRIDGE_LOCK_ONE_OR_BOTH(bridge1, bridge2);
if (bridge2) {
- RAII_VAR(struct ast_channel *, local_chan2, NULL, ao2_cleanup);
struct ast_channel *locals[2];
- ast_channel_lock(local_chan);
- local_chan2 = ast_local_get_peer(local_chan);
- ast_channel_unlock(local_chan);
+ /* Have to lock everything just in case a hangup comes in early */
+ ast_local_lock_all(local_chan, &locals[0], &locals[1]);
+ if (!locals[0] || !locals[1]) {
+ ast_log(LOG_ERROR, "Transfer failed probably due to an early hangup - "
+ "missing other half of '%s'\n", ast_channel_name(local_chan));
+ ast_local_unlock_all(local_chan);
+ return AST_BRIDGE_TRANSFER_FAIL;
+ }
- ast_assert(local_chan2 != NULL);
-
- locals[0] = local_chan;
- locals[1] = local_chan2;
+ /* Make sure the peer is properly set */
+ if (local_chan != locals[0]) {
+ struct ast_channel *tmp = locals[0];
+ locals[0] = locals[1];
+ locals[1] = tmp;
+ }
ast_attended_transfer_message_add_link(transfer_msg, locals);
+ ast_local_unlock_all(local_chan);
} else {
ast_attended_transfer_message_add_app(transfer_msg, app, local_chan);
}
- ao2_cleanup(local_chan);
return AST_BRIDGE_TRANSFER_SUCCESS;
}
diff --git a/main/core_local.c b/main/core_local.c
index 10bd839..4108dd8 100644
--- a/main/core_local.c
+++ b/main/core_local.c
@@ -235,6 +235,43 @@
char exten[AST_MAX_EXTENSION];
};
+void ast_local_lock_all(struct ast_channel *chan, struct ast_channel **outchan,
+ struct ast_channel **outowner)
+{
+ struct local_pvt *p = ast_channel_tech_pvt(chan);
+
+ *outchan = NULL;
+ *outowner = NULL;
+
+ if (p) {
+ ao2_ref(p, 1);
+ ast_unreal_lock_all(&p->base, outchan, outowner);
+ }
+}
+
+void ast_local_unlock_all(struct ast_channel *chan)
+{
+ struct local_pvt *p = ast_channel_tech_pvt(chan);
+ struct ast_unreal_pvt *base = &p->base;
+
+ if (!p) {
+ return;
+ }
+
+ if (base->owner) {
+ ast_channel_unlock(base->owner);
+ ast_channel_unref(base->owner);
+ }
+
+ if (base->chan) {
+ ast_channel_unlock(base->chan);
+ ast_channel_unref(base->chan);
+ }
+
+ ao2_unlock(base);
+ ao2_unlock(p);
+}
+
struct ast_channel *ast_local_get_peer(struct ast_channel *ast)
{
struct local_pvt *p = ast_channel_tech_pvt(ast);
--
To view, visit https://gerrit.asterisk.org/2319
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie6d6061e34c7c95f07116fffac9a09e5d225c880
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
More information about the asterisk-code-review
mailing list