[asterisk-commits] russell: trunk r104107 - in /trunk: ./ apps/app_chanspy.c
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Mon Feb 25 17:48:16 CST 2008
Author: russell
Date: Mon Feb 25 17:48:16 2008
New Revision: 104107
URL: http://svn.digium.com/view/asterisk?view=rev&rev=104107
Log:
Merged revisions 104106 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4
........
r104106 | russell | 2008-02-25 17:42:42 -0600 (Mon, 25 Feb 2008) | 10 lines
This patch fixes some pretty significant problems with how app_chanspy handles
pointers to channels that are being spied upon. It was very likely that a
crash would occur if the channel being spied upon hung up. This was because
the current ast_channel handling _requires_ that the object is locked or else
it could disappear at any time (except in the owning channel thread). So, this
patch uses some channel datastore magic on the spied upon channel to be able to
detect if and when the channel goes away.
(closes issue #11877)
(patch written by me, but thanks to kpfleming for the idea, and to file for review)
........
Modified:
trunk/ (props changed)
trunk/apps/app_chanspy.c
Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-1.4-merged' - no diff available.
Modified: trunk/apps/app_chanspy.c
URL: http://svn.digium.com/view/asterisk/trunk/apps/app_chanspy.c?view=diff&rev=104107&r1=104106&r2=104107
==============================================================================
--- trunk/apps/app_chanspy.c (original)
+++ trunk/apps/app_chanspy.c Mon Feb 25 17:48:16 2008
@@ -222,23 +222,31 @@
.generate = spy_generate,
};
-static int start_spying(struct ast_channel *chan, struct ast_channel *spychan, struct ast_audiohook *audiohook)
+static int start_spying(struct ast_channel *chan, const char *spychan_name, struct ast_audiohook *audiohook)
{
int res = 0;
struct ast_channel *peer = NULL;
- ast_log(LOG_NOTICE, "Attaching %s to %s\n", spychan->name, chan->name);
+ ast_log(LOG_NOTICE, "Attaching %s to %s\n", spychan_name, chan->name);
res = ast_audiohook_attach(chan, audiohook);
- if (!res && ast_test_flag(chan, AST_FLAG_NBRIDGE) && (peer = ast_bridged_channel(chan)))
+ if (!res && ast_test_flag(chan, AST_FLAG_NBRIDGE) && (peer = ast_bridged_channel(chan))) {
+ ast_channel_unlock(chan);
ast_softhangup(peer, AST_SOFTHANGUP_UNBRIDGE);
+ } else
+ ast_channel_unlock(chan);
return res;
}
-static int channel_spy(struct ast_channel *chan, struct ast_channel *spyee, int *volfactor, int fd,
- const struct ast_flags *flags, char *exitcontext)
+struct chanspy_ds {
+ struct ast_channel *chan;
+ ast_mutex_t lock;
+};
+
+static int channel_spy(struct ast_channel *chan, struct chanspy_ds *spyee_chanspy_ds,
+ int *volfactor, int fd, const struct ast_flags *flags, char *exitcontext)
{
struct chanspy_translation_helper csth;
int running = 0, res, x = 0;
@@ -246,6 +254,24 @@
char *name;
struct ast_frame *f;
struct ast_silence_generator *silgen = NULL;
+ struct ast_channel *spyee = NULL;
+ const char *spyer_name;
+
+ ast_channel_lock(chan);
+ spyer_name = ast_strdupa(chan->name);
+ ast_channel_unlock(chan);
+
+ ast_mutex_lock(&spyee_chanspy_ds->lock);
+ if (spyee_chanspy_ds->chan) {
+ spyee = spyee_chanspy_ds->chan;
+ ast_channel_lock(spyee);
+ }
+ ast_mutex_unlock(&spyee_chanspy_ds->lock);
+
+ if (!spyee)
+ return 0;
+
+ /* We now hold the channel lock on spyee */
if (ast_check_hangup(chan) || ast_check_hangup(spyee))
return 0;
@@ -257,15 +283,17 @@
ast_audiohook_init(&csth.spy_audiohook, AST_AUDIOHOOK_TYPE_SPY, "ChanSpy");
- if (start_spying(spyee, chan, &csth.spy_audiohook)) {
+ if (start_spying(spyee, spyer_name, &csth.spy_audiohook)) { /* Unlocks spyee */
ast_audiohook_destroy(&csth.spy_audiohook);
return 0;
}
if (ast_test_flag(flags, OPTION_WHISPER)) {
ast_audiohook_init(&csth.whisper_audiohook, AST_AUDIOHOOK_TYPE_WHISPER, "ChanSpy");
- start_spying(spyee, chan, &csth.whisper_audiohook);
- }
+ start_spying(spyee, spyer_name, &csth.whisper_audiohook); /* Unlocks spyee */
+ }
+
+ spyee = NULL;
csth.volfactor = *volfactor;
@@ -380,12 +408,82 @@
return running;
}
-static struct ast_channel *next_channel(const struct ast_channel *last, const char *spec,
- const char *exten, const char *context)
+/*!
+ * \note This relies on the embedded lock to be recursive, as it may be called
+ * due to a call to chanspy_ds_free with the lock held there.
+ */
+static void chanspy_ds_destroy(void *data)
+{
+ struct chanspy_ds *chanspy_ds = data;
+
+ /* Setting chan to be NULL is an atomic operation, but we don't want this
+ * value to change while this lock is held. The lock is held elsewhere
+ * while it performs non-atomic operations with this channel pointer */
+
+ ast_mutex_lock(&chanspy_ds->lock);
+ chanspy_ds->chan = NULL;
+ ast_mutex_unlock(&chanspy_ds->lock);
+}
+
+static const struct ast_datastore_info chanspy_ds_info = {
+ .type = "chanspy",
+ .destroy = chanspy_ds_destroy,
+};
+
+static struct chanspy_ds *chanspy_ds_free(struct chanspy_ds *chanspy_ds)
+{
+ if (!chanspy_ds)
+ return NULL;
+
+ ast_mutex_lock(&chanspy_ds->lock);
+ if (chanspy_ds->chan) {
+ struct ast_datastore *datastore;
+ struct ast_channel *chan;
+
+ chan = chanspy_ds->chan;
+
+ ast_channel_lock(chan);
+ if ((datastore = ast_channel_datastore_find(chan, &chanspy_ds_info, NULL))) {
+ ast_channel_datastore_remove(chan, datastore);
+ /* chanspy_ds->chan is NULL after this call */
+ ast_channel_datastore_free(datastore);
+ }
+ ast_channel_unlock(chan);
+ }
+ ast_mutex_unlock(&chanspy_ds->lock);
+
+ return NULL;
+}
+
+/*! \note Returns the channel in the chanspy_ds locked as well as the chanspy_ds locked */
+static struct chanspy_ds *setup_chanspy_ds(struct ast_channel *chan, struct chanspy_ds *chanspy_ds)
+{
+ struct ast_datastore *datastore = NULL;
+
+ ast_mutex_lock(&chanspy_ds->lock);
+
+ chanspy_ds->chan = chan;
+
+ if (!(datastore = ast_channel_datastore_alloc(&chanspy_ds_info, NULL))) {
+ chanspy_ds = chanspy_ds_free(chanspy_ds);
+ ast_channel_unlock(chan);
+ return NULL;
+ }
+
+ datastore->data = chanspy_ds;
+
+ ast_channel_datastore_add(chan, datastore);
+
+ return chanspy_ds;
+}
+
+static struct chanspy_ds *next_channel(struct ast_channel *chan,
+ const struct ast_channel *last, const char *spec,
+ const char *exten, const char *context, struct chanspy_ds *chanspy_ds)
{
struct ast_channel *this;
- redo:
+redo:
if (!ast_strlen_zero(spec))
this = ast_walk_channel_by_name_prefix_locked(last, spec, strlen(spec));
@@ -394,20 +492,21 @@
else
this = ast_channel_walk_locked(last);
- if (this) {
+ if (!this)
+ return NULL;
+
+ if (!strncmp(this->name, "Zap/pseudo", 10)) {
ast_channel_unlock(this);
- if (!strncmp(this->name, "Zap/pseudo", 10))
- goto redo;
- }
-
- return this;
+ goto redo;
+ }
+
+ return setup_chanspy_ds(this, chanspy_ds);
}
static int common_exec(struct ast_channel *chan, const struct ast_flags *flags,
int volfactor, const int fd, const char *mygroup, const char *myenforced,
const char *spec, const char *exten, const char *context)
{
- struct ast_channel *peer, *prev, *next;
char nameprefix[AST_NAME_STRLEN];
char peer_name[AST_NAME_STRLEN + 5];
char exitcontext[AST_MAX_CONTEXT] = "";
@@ -417,6 +516,7 @@
char *ptr;
int num;
int num_spyed_upon = 1;
+ struct chanspy_ds chanspy_ds;
if (ast_test_flag(flags, OPTION_EXIT)) {
const char *c;
@@ -428,6 +528,8 @@
ast_copy_string(exitcontext, chan->context, sizeof(exitcontext));
}
+ ast_mutex_init(&chanspy_ds.lock);
+
if (chan->_state != AST_STATE_UP)
ast_answer(chan);
@@ -436,6 +538,9 @@
waitms = 100;
for (;;) {
+ struct chanspy_ds *peer_chanspy_ds = NULL, *next_chanspy_ds = NULL;
+ struct ast_channel *prev = NULL, *peer = NULL;
+
if (!ast_test_flag(flags, OPTION_QUIET) && num_spyed_upon) {
res = ast_streamfile(chan, "beep", chan->language);
if (!res)
@@ -472,12 +577,13 @@
/* reset for the next loop around, unless overridden later */
waitms = 100;
- peer = prev = next = NULL;
num_spyed_upon = 0;
- for (peer = next_channel(peer, spec, exten, context);
- peer;
- prev = peer, peer = next ? next : next_channel(peer, spec, exten, context), next = NULL) {
+ for (peer_chanspy_ds = next_channel(chan, prev, spec, exten, context, &chanspy_ds);
+ peer_chanspy_ds;
+ chanspy_ds_free(peer_chanspy_ds), prev = peer,
+ peer_chanspy_ds = next_chanspy_ds ? next_chanspy_ds :
+ next_channel(chan, prev, spec, exten, context, &chanspy_ds), next_chanspy_ds = NULL) {
const char *group;
int igrp = !mygroup;
char *groups[25];
@@ -490,18 +596,32 @@
char *ext;
char *form_enforced;
int ienf = !myenforced;
-
- if (peer == prev)
+ struct ast_channel *peer;
+
+ peer = peer_chanspy_ds->chan;
+
+ ast_mutex_unlock(&peer_chanspy_ds->lock);
+
+ if (peer == prev) {
+ ast_channel_unlock(peer);
+ chanspy_ds_free(peer_chanspy_ds);
break;
-
- if (peer == chan)
+ }
+
+ if (peer == chan) {
+ ast_channel_unlock(peer);
continue;
-
- if (ast_test_flag(flags, OPTION_BRIDGED) && !ast_bridged_channel(peer))
+ }
+
+ if (ast_test_flag(flags, OPTION_BRIDGED) && !ast_bridged_channel(peer)) {
+ ast_channel_unlock(peer);
continue;
-
- if (ast_check_hangup(peer) || ast_test_flag(peer, AST_FLAG_SPYING))
+ }
+
+ if (ast_check_hangup(peer) || ast_test_flag(peer, AST_FLAG_SPYING)) {
+ ast_channel_unlock(peer);
continue;
+ }
if (mygroup) {
if ((group = pbx_builtin_getvar_helper(peer, "SPYGROUP"))) {
@@ -518,8 +638,10 @@
}
}
- if (!igrp)
+ if (!igrp) {
+ ast_channel_unlock(peer);
continue;
+ }
if (myenforced) {
@@ -559,6 +681,12 @@
for (s = peer_name; s < ptr; s++)
*s = tolower(*s);
+
+ /* We have to unlock the peer channel here to avoid a deadlock.
+ * So, when we need it again, we have to lock the datastore and get
+ * the pointer from there to see if the channel is still valid. */
+ ast_channel_unlock(peer);
+ peer = NULL;
if (!ast_test_flag(flags, OPTION_QUIET)) {
if (ast_fileexists(peer_name, NULL, NULL) != -1) {
@@ -574,8 +702,8 @@
}
waitms = 5000;
- res = channel_spy(chan, peer, &volfactor, fd, flags, exitcontext);
- num_spyed_upon++;
+ res = channel_spy(chan, peer_chanspy_ds, &volfactor, fd, flags, exitcontext);
+ num_spyed_upon++;
if (res == -1) {
goto exit;
@@ -583,13 +711,28 @@
res = 0;
goto exit;
} else if (res > 1 && spec) {
+ struct ast_channel *next;
+
snprintf(nameprefix, AST_NAME_STRLEN, "%s/%d", spec, res);
+
if ((next = ast_get_channel_by_name_prefix_locked(nameprefix, strlen(nameprefix)))) {
- ast_channel_unlock(next);
+ peer_chanspy_ds = chanspy_ds_free(peer_chanspy_ds);
+ next_chanspy_ds = setup_chanspy_ds(next, &chanspy_ds);
} else {
- /* stay on this channel */
- next = peer;
+ /* stay on this channel, if it is still valid */
+
+ ast_mutex_lock(&peer_chanspy_ds->lock);
+ if (peer_chanspy_ds->chan) {
+ ast_channel_lock(peer_chanspy_ds->chan);
+ next_chanspy_ds = peer_chanspy_ds;
+ peer_chanspy_ds = NULL;
+ } else {
+ /* the channel is gone */
+ ast_mutex_unlock(&peer_chanspy_ds->lock);
+ next_chanspy_ds = NULL;
+ }
}
+
peer = NULL;
}
}
@@ -599,6 +742,8 @@
ast_clear_flag(chan, AST_FLAG_SPYING);
ast_channel_setoption(chan, AST_OPTION_TXGAIN, &zero_volume, sizeof(zero_volume), 0);
+
+ ast_mutex_destroy(&chanspy_ds.lock);
return res;
}
More information about the asterisk-commits
mailing list