[Asterisk-code-review] core local: local channel data not being properly unref'ed a... (asterisk[certified/13.13])
Kevin Harwell
asteriskteam at digium.com
Tue Jun 20 16:08:01 CDT 2017
Kevin Harwell has uploaded this change for review. ( https://gerrit.asterisk.org/5891
Change subject: core_local: local channel data not being properly unref'ed and unlocked
......................................................................
core_local: local channel data not being properly unref'ed and unlocked
In an earlier version of Asterisk a local channel [un]lock all functions were
added in order to keep a crash from occurring when a channel hung up too early
during an attended transfer. Unfortunately, when a transfer failure occurs and
depending on the timing, the local channels sometime do not get properly
unlocked and deref'ed after being locked and ref'ed. This happens because the
underlying local channel structure gets NULLed out before unlocking.
This patch reworks those [un]lock functions and makes sure the values that get
locked and ref'ed later get unlocked and deref'ed.
ASTERISK-27074 #close
Change-Id: Ice96653e29bd9d6674ed5f95feb6b448ab148b09
---
M include/asterisk/core_local.h
M main/bridge.c
M main/core_local.c
3 files changed, 93 insertions(+), 22 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/91/5891/1
diff --git a/include/asterisk/core_local.h b/include/asterisk/core_local.h
index 8557072..0047577 100644
--- a/include/asterisk/core_local.h
+++ b/include/asterisk/core_local.h
@@ -45,6 +45,8 @@
* \brief Lock the "chan" and "owner" channels (and return them) on the base
* private structure as well as the base private structure itself.
*
+ * \deprecated - *DO NOT USE* Please use ast_local_lock_all2 instead.
+ *
* \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.
@@ -60,8 +62,28 @@
struct ast_channel **outowner);
/*!
+ * \brief Add a reference to the local channel's private tech, lock the local channel's
+ * private base, and add references and lock both sides of the local channel.
+ *
+ * \note None of these locks should be held prior to calling this function.
+ * \note To undo this process call ast_local_unlock_all2.
+ *
+ * \since 13.17.0, 14.6.0
+ *
+ * \param chan Must be a local channel
+ * \param tech_pvt [out] channel's private tech (ref added)
+ * \param base_pvt [out] channel's private base (lock added)
+ * \param base_chan [out] One side of the local channel (ref and lock added)
+ * \param base_owner [out] Other side of the local channel (ref and lock added)
+ */
+void ast_local_lock_all2(struct ast_channel *chan, void **tech_pvt, void **base_pvt,
+ struct ast_channel **base_chan, struct ast_channel **base_owner);
+
+/*!
* \brief Unlock the "chan" and "owner" channels on the base private structure
* as well as the base private structure itself.
+ *
+ * \deprecated - *DO NOT USE* Please use ast_local_unlock_all2 instead.
*
* \note This also removes references to each of the above mentioned elements and
* also the underlying private local structure.
@@ -74,6 +96,24 @@
void ast_local_unlock_all(struct ast_channel *chan);
/*!
+ * \brief Remove a reference to the given local channel's private tech, unlock the given
+ * local channel's private base, and remove references and unlock both sides of
+ * given the local channel.
+ *
+ * \note This function should be used in conjunction with ast_local_lock_all2.
+ *
+ * \since 13.17.0, 14.6.0
+ *
+ * \param chan Must be a local channel
+ * \param tech_pvt channel's private tech (ref removed)
+ * \param base_pvt channel's private base (lock removed)
+ * \param base_chan One side of the local channel (ref and lock removed)
+ * \param base_owner Other side of the local channel (ref and lock removed)
+ */
+void ast_local_unlock_all2(struct ast_channel *chan, void *tech_pvt, void *base_pvt,
+ struct ast_channel *base_chan, struct ast_channel *base_owner);
+
+/*!
* \brief Get the other local channel in the pair.
* \since 12.0.0
*
diff --git a/main/bridge.c b/main/bridge.c
index 6152b1b..e19a492 100644
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -4240,14 +4240,15 @@
BRIDGE_LOCK_ONE_OR_BOTH(bridge1, bridge2);
if (bridge2) {
+ void *tech, *base;
struct ast_channel *locals[2];
/* Have to lock everything just in case a hangup comes in early */
- ast_local_lock_all(local_chan, &locals[0], &locals[1]);
+ ast_local_lock_all2(local_chan, &tech, &base, &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);
+ ast_local_unlock_all2(local_chan, tech, base, locals[0], locals[1]);
ao2_cleanup(local_chan);
return AST_BRIDGE_TRANSFER_FAIL;
}
@@ -4258,7 +4259,7 @@
}
ast_attended_transfer_message_add_link(transfer_msg, locals);
- ast_local_unlock_all(local_chan);
+ ast_local_unlock_all2(local_chan, tech, base, locals[0], locals[1]);
} else {
ast_attended_transfer_message_add_app(transfer_msg, app, local_chan);
}
diff --git a/main/core_local.c b/main/core_local.c
index 1b8ebf6..5b274c6 100644
--- a/main/core_local.c
+++ b/main/core_local.c
@@ -235,18 +235,60 @@
char exten[AST_MAX_EXTENSION];
};
-void ast_local_lock_all(struct ast_channel *chan, struct ast_channel **outchan,
- struct ast_channel **outowner)
+void ast_local_lock_all2(struct ast_channel *chan, void **tech_pvt, void **base_pvt,
+ struct ast_channel **base_chan, struct ast_channel **base_owner)
{
struct local_pvt *p = ast_channel_tech_pvt(chan);
- *outchan = NULL;
- *outowner = NULL;
+ *tech_pvt = NULL;
+ *base_pvt = NULL;
+ *base_chan = NULL;
+ *base_owner = NULL;
if (p) {
- ao2_ref(p, 1);
- ast_unreal_lock_all(&p->base, outchan, outowner);
+ *tech_pvt = ao2_bump(p);
+ *base_pvt = &p->base;
+ ast_unreal_lock_all(*base_pvt, base_chan, base_owner);
}
+
+ ast_debug(5, "Local lock all - tech private = %p, base private = %p, base chan = %p,"
+ " base owner = %p\n", *tech_pvt, *base_pvt, *base_chan, *base_owner);
+}
+
+void ast_local_lock_all(struct ast_channel *chan, struct ast_channel **outchan,
+ struct ast_channel **outowner)
+{
+ void *tech_pvt, *base_pvt;
+ ast_local_lock_all2(chan, &tech_pvt, &base_pvt, outchan, outowner);
+}
+
+void ast_local_unlock_all2(struct ast_channel *chan, void *tech_pvt, void *base_pvt,
+ struct ast_channel *base_chan, struct ast_channel *base_owner)
+{
+ struct local_pvt *local = ast_channel_tech_pvt(chan);
+ struct ast_unreal_pvt *unreal = local ? &local->base : NULL;
+ struct ast_channel *unreal_chan = unreal ? unreal->chan : NULL;
+ struct ast_channel *unreal_owner = unreal ? unreal->owner : NULL;
+
+
+ ast_debug(5, "Local unlock all - local private = %p|%p, unreal private = %p|%p, "
+ "base chan = %p|%p, base owner = %p|%p\n", local, tech_pvt, unreal,
+ base_pvt, unreal_chan, base_chan, unreal_owner, base_owner);
+
+ if (base_chan) {
+ ast_channel_unlock(base_chan);
+ ast_channel_unref(base_chan);
+ }
+
+ if (base_owner) {
+ ast_channel_unlock(base_owner);
+ ast_channel_unref(base_owner);
+ }
+
+ if (base_pvt) {
+ ao2_unlock(base_pvt);
+ }
+ ao2_cleanup(tech_pvt);
}
void ast_local_unlock_all(struct ast_channel *chan)
@@ -259,19 +301,7 @@
}
base = &p->base;
-
- 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_ref(p, -1);
+ ast_local_unlock_all2(chan, p, base, base->chan, base->owner);
}
struct ast_channel *ast_local_get_peer(struct ast_channel *ast)
--
To view, visit https://gerrit.asterisk.org/5891
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.13
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ice96653e29bd9d6674ed5f95feb6b448ab148b09
Gerrit-Change-Number: 5891
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170620/86f7c263/attachment-0001.html>
More information about the asterisk-code-review
mailing list