From dcb25bb05792f5cbbb458b264030f68cfb59b682 Mon Sep 17 00:00:00 2001 From: Walter Doekes Date: Fri, 11 Mar 2016 23:03:08 +0100 Subject: app_chanspy: Fix occasional deadlock with ChanSpy and Local channels. Channel masquerading had a conflict with autochannel locking. When locking autochannel->channel, the channel is fetched from the autochannel and then locked. During the fetch, the autochannel -- which has no locks itself -- can be modified by someone who owns the channel lock. That means that the value of autochan->channel cannot be trusted until you hold the lock. In practice, this caused problems with Local channels getting masqueraded away while the ChanSpy attempted to get info from that channel. The old channel which was about to get removed got locked, but the new (replaced) channel got unlocked (no-op). Because the replaced channel was now locked (and would never get unlocked), it couldn't get removed from the channel list in a timely manner, and would now cause deadlocks when iterating over the channel list. This change checks the autochannel after locking the channel for changes to the autochannel. If the channel had been changed, the lock is reobtained on the new channel. In theory it seems possible that after this fix, the lock attempt on the old (wrong) channel can be on an already destroyed lock, maybe causing a crash. But that hasn't been observed in the wild and is harder induce than the current deadlock. Thanks go to Filip Frank for suggesting a fix similar to this and especially to IRC user hexanol for pointing out why this deadlock was possible and testing this fix. And to Richard for catching my rookie while loop mistake ;) ASTERISK-25321 #close Change-Id: I293ae0014e531cd0e675c3f02d1d118a98683def --- main/autochan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'main/autochan.c') diff --git a/main/autochan.c b/main/autochan.c index d41a8d821..38d778438 100644 --- a/main/autochan.c +++ b/main/autochan.c @@ -51,7 +51,7 @@ struct ast_autochan *ast_autochan_setup(struct ast_channel *chan) autochan->chan = ast_channel_ref(chan); - ast_channel_lock(autochan->chan); + ast_channel_lock(autochan->chan); /* autochan is still private, no need for ast_autochan_channel_lock() */ AST_LIST_INSERT_TAIL(ast_channel_autochans(autochan->chan), autochan, list); ast_channel_unlock(autochan->chan); @@ -64,7 +64,7 @@ void ast_autochan_destroy(struct ast_autochan *autochan) { struct ast_autochan *autochan_iter; - ast_channel_lock(autochan->chan); + ast_autochan_channel_lock(autochan); AST_LIST_TRAVERSE_SAFE_BEGIN(ast_channel_autochans(autochan->chan), autochan_iter, list) { if (autochan_iter == autochan) { AST_LIST_REMOVE_CURRENT(list); @@ -73,7 +73,7 @@ void ast_autochan_destroy(struct ast_autochan *autochan) } } AST_LIST_TRAVERSE_SAFE_END; - ast_channel_unlock(autochan->chan); + ast_autochan_channel_unlock(autochan); autochan->chan = ast_channel_unref(autochan->chan); -- cgit v1.2.3