diff options
author | Richard Mudgett <rmudgett@digium.com> | 2014-10-03 17:39:50 +0000 |
---|---|---|
committer | Richard Mudgett <rmudgett@digium.com> | 2014-10-03 17:39:50 +0000 |
commit | 6a844be566478d6fed1ec93c0836fbf5d5d46ff3 (patch) | |
tree | ca7a344f77695f1ecdeaf68cb5c9a6b899562b96 /main | |
parent | b67094624d33b6586177435017cd4f6fdedd3f3f (diff) |
chan_pjsip: Fix deadlock when masquerading PJSIP channels.
Performing a directed call pickup resulted in a deadlock when PJSIP
channels were involved.
A masquerade needs to hold onto the channel locks while it swaps channel
information between the two channels involved in the masquerade. With
PJSIP channels, the fixup routine needed to push a fixup task onto the
PJSIP channel's serializer. Unfortunately, if the serializer was also
processing a task that needed to lock the channel, you get deadlock.
* Added a new control frame that is used to notify the channels that a
masquerade is about to start and when it has completed.
* Added the ability to query taskprocessors if the current thread is the
taskprocessor thread.
* Added the ability to suspend/unsuspend the PJSIP serializer thread so a
masquerade could fixup the PJSIP channel without using the serializer.
ASTERISK-24356 #close
Reported by: rmudgett
Review: https://reviewboard.asterisk.org/r/4034/
........
Merged revisions 424471 from http://svn.asterisk.org/svn/asterisk/branches/12
git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/13@424472 65c4cc65-6c06-0410-ace0-fbb531ad65f3
Diffstat (limited to 'main')
-rw-r--r-- | main/bridge_channel.c | 4 | ||||
-rw-r--r-- | main/channel.c | 29 | ||||
-rw-r--r-- | main/core_unreal.c | 6 | ||||
-rw-r--r-- | main/taskprocessor.c | 18 |
4 files changed, 52 insertions, 5 deletions
diff --git a/main/bridge_channel.c b/main/bridge_channel.c index 5bf85126a..d94b7387f 100644 --- a/main/bridge_channel.c +++ b/main/bridge_channel.c @@ -2065,6 +2065,10 @@ static void bridge_channel_handle_control(struct ast_bridge_channel *bridge_chan ast_indicate(chan, -1); } break; + case AST_CONTROL_MASQUERADE_NOTIFY: + /* Should never happen. */ + ast_assert(0); + break; default: ast_indicate_data(chan, fr->subclass.integer, fr->data.ptr, fr->datalen); break; diff --git a/main/channel.c b/main/channel.c index 9100f2671..47d47b6b0 100644 --- a/main/channel.c +++ b/main/channel.c @@ -4285,6 +4285,7 @@ static int attribute_const is_visible_indication(enum ast_control_frame_type con case AST_CONTROL_MCID: case AST_CONTROL_UPDATE_RTP_PEER: case AST_CONTROL_PVT_CAUSE_CODE: + case AST_CONTROL_MASQUERADE_NOTIFY: case AST_CONTROL_STREAM_STOP: case AST_CONTROL_STREAM_SUSPEND: case AST_CONTROL_STREAM_REVERSE: @@ -4451,7 +4452,9 @@ int ast_indicate_data(struct ast_channel *chan, int _condition, ast_channel_lock(chan); /* Don't bother if the channel is about to go away, anyway. */ - if (ast_test_flag(ast_channel_flags(chan), AST_FLAG_ZOMBIE) || ast_check_hangup(chan)) { + if ((ast_test_flag(ast_channel_flags(chan), AST_FLAG_ZOMBIE) + || ast_check_hangup(chan)) + && condition != AST_CONTROL_MASQUERADE_NOTIFY) { res = -1; goto indicate_cleanup; } @@ -4599,6 +4602,7 @@ int ast_indicate_data(struct ast_channel *chan, int _condition, case AST_CONTROL_AOC: case AST_CONTROL_END_OF_Q: case AST_CONTROL_MCID: + case AST_CONTROL_MASQUERADE_NOTIFY: case AST_CONTROL_UPDATE_RTP_PEER: case AST_CONTROL_STREAM_STOP: case AST_CONTROL_STREAM_SUSPEND: @@ -6445,6 +6449,11 @@ static void channel_do_masquerade(struct ast_channel *original, struct ast_chann * original channel's backend. While the features are nice, which is the * reason we're keeping it, it's still awesomely weird. XXX */ + /* Indicate to each channel that a masquerade is about to begin. */ + x = 1; + ast_indicate_data(original, AST_CONTROL_MASQUERADE_NOTIFY, &x, sizeof(x)); + ast_indicate_data(clonechan, AST_CONTROL_MASQUERADE_NOTIFY, &x, sizeof(x)); + /* * The container lock is necessary for proper locking order * because the channels must be unlinked to change their @@ -6485,8 +6494,9 @@ static void channel_do_masquerade(struct ast_channel *original, struct ast_chann /* Start the masquerade channel contents rearangement. */ ast_channel_lock_both(original, clonechan); - ast_debug(4, "Actually Masquerading %s(%u) into the structure of %s(%u)\n", - ast_channel_name(clonechan), ast_channel_state(clonechan), ast_channel_name(original), ast_channel_state(original)); + ast_debug(1, "Actually Masquerading %s(%u) into the structure of %s(%u)\n", + ast_channel_name(clonechan), ast_channel_state(clonechan), + ast_channel_name(original), ast_channel_state(original)); /* * Remember the original read/write formats. We turn off any @@ -6759,6 +6769,19 @@ static void channel_do_masquerade(struct ast_channel *original, struct ast_chann ast_channel_unlock(original); ast_channel_unlock(clonechan); + /* + * Indicate to each channel that a masquerade is complete. + * + * We can still do this to clonechan even though it is a + * zombie because ast_indicate_data() will explicitly pass + * this control and ast_hangup() is held off until the + * ast_channel_masq() and ast_channel_masqr() pointers are + * cleared. + */ + x = 0; + ast_indicate_data(original, AST_CONTROL_MASQUERADE_NOTIFY, &x, sizeof(x)); + ast_indicate_data(clonechan, AST_CONTROL_MASQUERADE_NOTIFY, &x, sizeof(x)); + ast_bridge_notify_masquerade(original); if (clone_hold_state == AST_CONTROL_HOLD) { diff --git a/main/core_unreal.c b/main/core_unreal.c index b3dc5281c..90ed3fb43 100644 --- a/main/core_unreal.c +++ b/main/core_unreal.c @@ -530,6 +530,12 @@ int ast_unreal_indicate(struct ast_channel *ast, int condition, const void *data ao2_ref(p, 1); /* ref for unreal_queue_frame */ switch (condition) { + case AST_CONTROL_MASQUERADE_NOTIFY: + /* + * Always block this because this is the channel being + * masqueraded; not anything down the chain. + */ + break; case AST_CONTROL_CONNECTED_LINE: case AST_CONTROL_REDIRECTING: res = unreal_colp_redirect_indicate(p, ast, condition); diff --git a/main/taskprocessor.c b/main/taskprocessor.c index 0582f76fa..5fbbca571 100644 --- a/main/taskprocessor.c +++ b/main/taskprocessor.c @@ -78,9 +78,9 @@ struct ast_taskprocessor { long tps_queue_size; /*! \brief Taskprocessor queue */ AST_LIST_HEAD_NOLOCK(tps_queue, tps_task) tps_queue; - /*! \brief Taskprocessor singleton list entry */ - AST_LIST_ENTRY(ast_taskprocessor) list; struct ast_taskprocessor_listener *listener; + /*! Current thread executing the tasks */ + pthread_t thread; /*! Indicates if the taskprocessor is currently executing a task */ unsigned int executing:1; }; @@ -600,6 +600,8 @@ static struct ast_taskprocessor *__allocate_taskprocessor(const char *name, stru ao2_ref(listener, +1); p->listener = listener; + p->thread = AST_PTHREADT_NULL; + ao2_ref(p, +1); listener->tps = p; @@ -752,6 +754,7 @@ int ast_taskprocessor_execute(struct ast_taskprocessor *tps) return 0; } + tps->thread = pthread_self(); tps->executing = 1; if (t->wants_local) { @@ -768,6 +771,7 @@ int ast_taskprocessor_execute(struct ast_taskprocessor *tps) tps_task_free(t); ao2_lock(tps); + tps->thread = AST_PTHREADT_NULL; /* We need to check size in the same critical section where we reset the * executing bit. Avoids a race condition where a task is pushed right * after we pop an empty stack. @@ -789,3 +793,13 @@ int ast_taskprocessor_execute(struct ast_taskprocessor *tps) } return size > 0; } + +int ast_taskprocessor_is_task(struct ast_taskprocessor *tps) +{ + int is_task; + + ao2_lock(tps); + is_task = pthread_equal(tps->thread, pthread_self()); + ao2_unlock(tps); + return is_task; +} |