From 5d34ca5b3314bfa93dc72a9003d9a81b9bbd180b Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Mon, 18 Jun 2018 18:04:54 -0500 Subject: [PATCH 1/3] autoservice: Don't start channel autoservice if the thread is a user interface. Executing dialplan functions from either AMI or ARI by getting a variable could place the channel into autoservice. However, these user interface threads do not handle the channel's media so we wind up with two threads attempting to handle the media. There can be one and only one thread handling a channel's media at a time. Otherwise, we don't know which thread is going to handle the media frames. ASTERISK-27625 Change-Id: If2dc94ce15ddabf923ed1e2a65ea0ef56e013e49 --- include/asterisk/utils.h | 18 ++++++++++++++++++ main/autoservice.c | 14 ++++++++++++++ main/tcptls.c | 13 +++++++++++++ main/utils.c | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+) diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h index f8f761d39f..379ae83196 100644 --- a/include/asterisk/utils.h +++ b/include/asterisk/utils.h @@ -1195,4 +1195,22 @@ enum ast_fd_flag_operation { int __ast_fd_set_flags(int fd, int flags, enum ast_fd_flag_operation op, const char *file, int lineno, const char *function); +/*! + * \brief Set the current thread's user interface status. + * + * \param is_user_interface Non-zero to mark the thread as a user interface. + * + * \return 0 if successfuly marked current thread. + * \return Non-zero if marking current thread failed. + */ +int ast_thread_user_interface_set(int is_user_interface); + +/*! + * \brief Indicates whether the current thread is a user interface + * + * \return True (non-zero) if thread is a user interface. + * \return False (zero) if thread is not a user interface. + */ +int ast_thread_is_user_interface(void); + #endif /* _ASTERISK_UTILS_H */ diff --git a/main/autoservice.c b/main/autoservice.c index f353f0e0cf..40e40454d3 100644 --- a/main/autoservice.c +++ b/main/autoservice.c @@ -195,6 +195,13 @@ int ast_autoservice_start(struct ast_channel *chan) int res = 0; struct asent *as; + if (ast_thread_is_user_interface()) { + /* User interface threads do not handle channel media. */ + ast_debug(1, "Thread is a user interface, not putting channel %s into autoservice\n", + ast_channel_name(chan)); + return 0; + } + AST_LIST_LOCK(&aslist); AST_LIST_TRAVERSE(&aslist, as, list) { if (as->chan == chan) { @@ -256,6 +263,13 @@ int ast_autoservice_stop(struct ast_channel *chan) struct ast_frame *f; int chan_list_state; + if (ast_thread_is_user_interface()) { + /* User interface threads do not handle channel media. */ + ast_debug(1, "Thread is a user interface, not removing channel %s from autoservice\n", + ast_channel_name(chan)); + return 0; + } + AST_LIST_LOCK(&aslist); /* Save the autoservice channel list state. We _must_ verify that the channel diff --git a/main/tcptls.c b/main/tcptls.c index 7c3efb4502..c5ab0b6036 100644 --- a/main/tcptls.c +++ b/main/tcptls.c @@ -685,6 +685,19 @@ static void *handle_tcptls_connection(void *data) return NULL; } + /* + * TCP/TLS connections are associated with external protocols which can + * be considered to be user interfaces (even for SIP messages), and + * will not handle channel media. This may need to be pushed down into + * the individual protocol handlers, but this seems like a good start. + */ + if (ast_thread_user_interface_set(1)) { + ast_log(LOG_ERROR, "Failed to set user interface status; killing connection\n"); + ast_tcptls_close_session_file(tcptls_session); + ao2_ref(tcptls_session, -1); + return NULL; + } + tcptls_session->stream_cookie = tcptls_stream_alloc(); if (!tcptls_session->stream_cookie) { ast_tcptls_close_session_file(tcptls_session); diff --git a/main/utils.c b/main/utils.c index 2e612c111f..ba16e41f18 100644 --- a/main/utils.c +++ b/main/utils.c @@ -2833,3 +2833,38 @@ int __ast_fd_set_flags(int fd, int flags, enum ast_fd_flag_operation op, return 0; } + +/*! + * \brief A thread local indicating whether the current thread is a user interface. + */ +AST_THREADSTORAGE(thread_user_interface_tl); + +int ast_thread_user_interface_set(int is_user_interface) +{ + int *thread_user_interface; + + thread_user_interface = ast_threadstorage_get( + &thread_user_interface_tl, sizeof(*thread_user_interface)); + if (thread_user_interface == NULL) { + ast_log(LOG_ERROR, "Error setting user interface status for current thread\n"); + return -1; + } + + *thread_user_interface = !!is_user_interface; + return 0; +} + +int ast_thread_is_user_interface(void) +{ + int *thread_user_interface; + + thread_user_interface = ast_threadstorage_get( + &thread_user_interface_tl, sizeof(*thread_user_interface)); + if (thread_user_interface == NULL) { + ast_log(LOG_ERROR, "Error checking thread's user interface status\n"); + /* On error, assume that we are not a user interface thread */ + return 0; + } + + return *thread_user_interface; +} From 1abcc41ffff95d5a5f4aae55055a53487c9c909c Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Tue, 12 Jun 2018 14:09:54 -0500 Subject: [PATCH 2/3] channel.c: Fix usage of CHECK_BLOCKING() The CHECK_BLOCKING() macro is used to indicate if a channel's handling thread is about to do a blocking operation (poll, read, or write) of media. A few operations such as ast_queue_frame(), soft hangup, and masquerades use the indication to wake up the blocked thread to reevaluate what is going on. ASTERISK-27625 Change-Id: I4dfc33e01e60627d962efa29d0a4244cf151a84d --- include/asterisk/channel.h | 27 +++++++++++++++++++++------ main/channel.c | 34 +++++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index 0fc236af4b..8c171b6f1b 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -2649,15 +2649,30 @@ static inline enum ast_t38_state ast_channel_get_t38_state(struct ast_channel *c return state; } -#define CHECK_BLOCKING(c) do { \ - if (ast_test_flag(ast_channel_flags(c), AST_FLAG_BLOCKING)) {\ - ast_debug(1, "Thread %p is blocking '%s', already blocked by thread %p in procedure %s\n", \ - (void *) pthread_self(), ast_channel_name(c), (void *) ast_channel_blocker(c), ast_channel_blockproc(c)); \ - } else { \ +/*! + * \brief Set the blocking indication on the channel. + * + * \details + * Indicate that the thread handling the channel is about to do a blocking + * operation to wait for media on the channel. (poll, read, or write) + * + * Masquerading and ast_queue_frame() use this indication to wake up the thread. + * + * \pre The channel needs to be locked + */ +#define CHECK_BLOCKING(c) \ + do { \ + if (ast_test_flag(ast_channel_flags(c), AST_FLAG_BLOCKING)) { \ + /* This should not happen as there should only be one thread handling a channel's media at a time. */ \ + ast_log(LOG_DEBUG, "Thread %p is blocking '%s', already blocked by thread %p in procedure %s\n", \ + (void *) pthread_self(), ast_channel_name(c), \ + (void *) ast_channel_blocker(c), ast_channel_blockproc(c)); \ + ast_assert(0); \ + } \ ast_channel_blocker_set((c), pthread_self()); \ ast_channel_blockproc_set((c), __PRETTY_FUNCTION__); \ ast_set_flag(ast_channel_flags(c), AST_FLAG_BLOCKING); \ - } } while (0) + } while (0) ast_group_t ast_get_group(const char *s); diff --git a/main/channel.c b/main/channel.c index 27a959a4b9..2a72aeed21 100644 --- a/main/channel.c +++ b/main/channel.c @@ -3266,11 +3266,11 @@ static struct ast_channel *ast_waitfor_nandfds_simple(struct ast_channel *chan, } } - ast_channel_unlock(chan); - /* Time to make this channel block... */ CHECK_BLOCKING(chan); + ast_channel_unlock(chan); + if (*ms > 0) { start = ast_tvnow(); } @@ -3279,7 +3279,9 @@ static struct ast_channel *ast_waitfor_nandfds_simple(struct ast_channel *chan, res = epoll_wait(ast_channel_epfd(chan), ev, 1, rms); /* Stop blocking */ + ast_channel_lock(chan); ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); + ast_channel_unlock(chan); /* Simulate a timeout if we were interrupted */ if (res < 0) { @@ -3305,12 +3307,14 @@ static struct ast_channel *ast_waitfor_nandfds_simple(struct ast_channel *chan, /* See what events are pending */ aed = ev[0].data.ptr; + ast_channel_lock(chan); ast_channel_fdno_set(chan, aed->which); if (ev[0].events & EPOLLPRI) { ast_set_flag(ast_channel_flags(chan), AST_FLAG_EXCEPTION); } else { ast_clear_flag(ast_channel_flags(chan), AST_FLAG_EXCEPTION); } + ast_channel_unlock(chan); if (*ms > 0) { *ms -= ast_tvdiff_ms(ast_tvnow(), start); @@ -3346,8 +3350,8 @@ static struct ast_channel *ast_waitfor_nandfds_complex(struct ast_channel **c, i whentohangup = diff; } } - ast_channel_unlock(c[i]); CHECK_BLOCKING(c[i]); + ast_channel_unlock(c[i]); } rms = *ms; @@ -3365,7 +3369,9 @@ static struct ast_channel *ast_waitfor_nandfds_complex(struct ast_channel **c, i res = epoll_wait(ast_channel_epfd(c[0]), ev, 25, rms); for (i = 0; i < n; i++) { + ast_channel_lock(c[i]); ast_clear_flag(ast_channel_flags(c[i]), AST_FLAG_BLOCKING); + ast_channel_unlock(c[i]); } if (res < 0) { @@ -3400,12 +3406,14 @@ static struct ast_channel *ast_waitfor_nandfds_complex(struct ast_channel **c, i } winner = aed->chan; + ast_channel_lock(winner); if (ev[i].events & EPOLLPRI) { ast_set_flag(ast_channel_flags(winner), AST_FLAG_EXCEPTION); } else { ast_clear_flag(ast_channel_flags(winner), AST_FLAG_EXCEPTION); } ast_channel_fdno_set(winner, aed->which); + ast_channel_unlock(winner); } if (*ms > 0) { @@ -5220,11 +5228,9 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) /* There is a generator running while we're in the middle of a digit. * It's probably inband DTMF, so go ahead and pass it so it can * stop the generator */ - ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); ast_channel_unlock(chan); res = ast_senddigit_end(chan, fr->subclass.integer, fr->len); ast_channel_lock(chan); - CHECK_BLOCKING(chan); } else if (fr->frametype == AST_FRAME_CONTROL && fr->subclass.integer == AST_CONTROL_UNHOLD) { /* @@ -5242,7 +5248,6 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) /* High bit prints debugging */ if (ast_channel_fout(chan) & DEBUGCHAN_FLAG) ast_frame_dump(ast_channel_name(chan), fr, ">>"); - CHECK_BLOCKING(chan); switch (fr->frametype) { case AST_FRAME_CONTROL: indicate_data_internal(chan, fr->subclass.integer, fr->data.ptr, fr->datalen); @@ -5256,11 +5261,9 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) f = fr; } send_dtmf_begin_event(chan, DTMF_SENT, fr->subclass.integer); - ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); ast_channel_unlock(chan); res = ast_senddigit_begin(chan, fr->subclass.integer); ast_channel_lock(chan); - CHECK_BLOCKING(chan); break; case AST_FRAME_DTMF_END: if (ast_channel_audiohooks(chan)) { @@ -5272,13 +5275,12 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) } } send_dtmf_end_event(chan, DTMF_SENT, fr->subclass.integer, fr->len); - ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); ast_channel_unlock(chan); res = ast_senddigit_end(chan, fr->subclass.integer, fr->len); ast_channel_lock(chan); - CHECK_BLOCKING(chan); break; case AST_FRAME_TEXT: + CHECK_BLOCKING(chan); if (ast_format_cmp(fr->subclass.format, ast_format_t140) == AST_FORMAT_CMP_EQUAL) { res = (ast_channel_tech(chan)->write_text == NULL) ? 0 : ast_channel_tech(chan)->write_text(chan, fr); @@ -5286,19 +5288,26 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) res = (ast_channel_tech(chan)->send_text == NULL) ? 0 : ast_channel_tech(chan)->send_text(chan, (char *) fr->data.ptr); } + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_HTML: + CHECK_BLOCKING(chan); res = (ast_channel_tech(chan)->send_html == NULL) ? 0 : ast_channel_tech(chan)->send_html(chan, fr->subclass.integer, (char *) fr->data.ptr, fr->datalen); + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_VIDEO: /* XXX Handle translation of video codecs one day XXX */ + CHECK_BLOCKING(chan); res = (ast_channel_tech(chan)->write_video == NULL) ? 0 : ast_channel_tech(chan)->write_video(chan, fr); + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_MODEM: + CHECK_BLOCKING(chan); res = (ast_channel_tech(chan)->write == NULL) ? 0 : ast_channel_tech(chan)->write(chan, fr); + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_VOICE: if (ast_channel_tech(chan)->write == NULL) @@ -5459,6 +5468,7 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) /* the translator on chan->writetrans may have returned multiple frames from the single frame we passed in; if so, feed each one of them to the channel, freeing each one after it has been written */ + CHECK_BLOCKING(chan); if ((f != fr) && AST_LIST_NEXT(f, frame_list)) { struct ast_frame *cur, *next = NULL; unsigned int skip = 0; @@ -5487,6 +5497,7 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) } else { res = ast_channel_tech(chan)->write(chan, f); } + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_NULL: case AST_FRAME_IAX: @@ -5497,13 +5508,14 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr) /* At this point, fr is the incoming frame and f is NULL. Channels do * not expect to get NULL as a frame pointer and will segfault. Hence, * we output the original frame passed in. */ + CHECK_BLOCKING(chan); res = ast_channel_tech(chan)->write(chan, fr); + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; } if (f && f != fr) ast_frfree(f); - ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); /* Consider a write failure to force a soft hangup */ if (res < 0) { From f878de44afa79e261fe483f6d0f21d3970c5c2cc Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Wed, 13 Jun 2018 11:33:44 -0500 Subject: [PATCH 3/3] channel.c: Make CHECK_BLOCKING() save thread LWP id for messages. * Removed an unnecessary call to ast_channel_blocker_set() in __ast_read(). ASTERISK-27625 Change-Id: I342168b999984666fb869cd519fe779583a73834 --- include/asterisk/channel.h | 10 +++++++--- main/channel.c | 9 ++++----- main/channel_internal_api.c | 10 ++++++++++ 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index 8c171b6f1b..d7091593b4 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -2664,11 +2664,12 @@ static inline enum ast_t38_state ast_channel_get_t38_state(struct ast_channel *c do { \ if (ast_test_flag(ast_channel_flags(c), AST_FLAG_BLOCKING)) { \ /* This should not happen as there should only be one thread handling a channel's media at a time. */ \ - ast_log(LOG_DEBUG, "Thread %p is blocking '%s', already blocked by thread %p in procedure %s\n", \ - (void *) pthread_self(), ast_channel_name(c), \ - (void *) ast_channel_blocker(c), ast_channel_blockproc(c)); \ + ast_log(LOG_DEBUG, "Thread LWP %d is blocking '%s', already blocked by thread LWP %d in procedure %s\n", \ + ast_get_tid(), ast_channel_name(c), \ + ast_channel_blocker_tid(c), ast_channel_blockproc(c)); \ ast_assert(0); \ } \ + ast_channel_blocker_tid_set((c), ast_get_tid()); \ ast_channel_blocker_set((c), pthread_self()); \ ast_channel_blockproc_set((c), __PRETTY_FUNCTION__); \ ast_set_flag(ast_channel_flags(c), AST_FLAG_BLOCKING); \ @@ -4316,6 +4317,9 @@ void ast_channel_internal_epfd_data_set(struct ast_channel *chan, int which , st pthread_t ast_channel_blocker(const struct ast_channel *chan); void ast_channel_blocker_set(struct ast_channel *chan, pthread_t value); +int ast_channel_blocker_tid(const struct ast_channel *chan); +void ast_channel_blocker_tid_set(struct ast_channel *chan, int tid); + ast_timing_func_t ast_channel_timingfunc(const struct ast_channel *chan); void ast_channel_timingfunc_set(struct ast_channel *chan, ast_timing_func_t value); diff --git a/main/channel.c b/main/channel.c index 2a72aeed21..753bb1a00c 100644 --- a/main/channel.c +++ b/main/channel.c @@ -2735,10 +2735,10 @@ void ast_hangup(struct ast_channel *chan) ast_channel_generator_set(chan, NULL); if (ast_test_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING)) { - ast_log(LOG_WARNING, "Hard hangup called by thread %ld on %s, while fd " - "is blocked by thread %ld in procedure %s! Expect a failure\n", - (long) pthread_self(), ast_channel_name(chan), (long)ast_channel_blocker(chan), ast_channel_blockproc(chan)); - ast_assert(ast_test_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING) == 0); + ast_log(LOG_WARNING, "Hard hangup called by thread LWP %d on %s, while blocked by thread LWP %d in procedure %s! Expect a failure\n", + ast_get_tid(), ast_channel_name(chan), ast_channel_blocker_tid(chan), + ast_channel_blockproc(chan)); + ast_assert(0); } if (ast_channel_tech(chan)->hangup) { @@ -3964,7 +3964,6 @@ static struct ast_frame *__ast_read(struct ast_channel *chan, int dropaudio) } } } else { - ast_channel_blocker_set(chan, pthread_self()); if (ast_test_flag(ast_channel_flags(chan), AST_FLAG_EXCEPTION)) { if (ast_channel_tech(chan)->exception) f = ast_channel_tech(chan)->exception(chan); diff --git a/main/channel_internal_api.c b/main/channel_internal_api.c index d181f52a2b..6e429b3db1 100644 --- a/main/channel_internal_api.c +++ b/main/channel_internal_api.c @@ -169,6 +169,7 @@ struct ast_channel { unsigned long insmpl; /*!< Track the read/written samples for monitor use */ unsigned long outsmpl; /*!< Track the read/written samples for monitor use */ + int blocker_tid; /*!< If anyone is blocking, this is their thread id */ int fds[AST_MAX_FDS]; /*!< File descriptors for channel -- Drivers will poll on * these file descriptors, so at least one must be non -1. * See \arg \ref AstFileDesc */ @@ -1331,6 +1332,15 @@ void ast_channel_blocker_set(struct ast_channel *chan, pthread_t value) chan->blocker = value; } +int ast_channel_blocker_tid(const struct ast_channel *chan) +{ + return chan->blocker_tid; +} +void ast_channel_blocker_tid_set(struct ast_channel *chan, int value) +{ + chan->blocker_tid = value; +} + ast_timing_func_t ast_channel_timingfunc(const struct ast_channel *chan) { return chan->timingfunc;