From 0989b63047369fc93551f8ca96065bcfac118983 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 b892cda9e4..6f9a11e936 100644 --- a/include/asterisk/utils.h +++ b/include/asterisk/utils.h @@ -913,4 +913,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 cd7388b7dd..8ff2cb56e1 100644 --- a/main/autoservice.c +++ b/main/autoservice.c @@ -202,6 +202,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) { @@ -263,6 +270,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 f5557307e0..3ba52ff6a3 100644 --- a/main/tcptls.c +++ b/main/tcptls.c @@ -134,6 +134,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; + } + if (tcptls_session->parent->tls_cfg) { #ifdef DO_SSL if (ast_iostream_start_tls(&tcptls_session->stream, tcptls_session->parent->tls_cfg->ssl_ctx, tcptls_session->client) < 0) { diff --git a/main/utils.c b/main/utils.c index 6ecd2b82e5..2eaf1f9e25 100644 --- a/main/utils.c +++ b/main/utils.c @@ -2750,3 +2750,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 080508d2eb6acd70ed57187ffd2af9ac51c76948 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 | 40 ++++++++++++++++++++++---------------- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index 91aed34683..79a147745a 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -2710,15 +2710,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 647959792e..d92de81972 100644 --- a/main/channel.c +++ b/main/channel.c @@ -5033,11 +5033,9 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame /* 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) { /* @@ -5055,7 +5053,6 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame /* 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); @@ -5069,11 +5066,9 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame 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)) { @@ -5085,13 +5080,12 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame } } 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); @@ -5099,13 +5093,17 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame 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); if (ast_channel_tech(chan)->write_stream) { if (stream) { res = ast_channel_tech(chan)->write_stream(chan, ast_stream_get_position(stream), fr); @@ -5117,8 +5115,10 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame } else { res = 0; } + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_MODEM: + CHECK_BLOCKING(chan); if (ast_channel_tech(chan)->write_stream) { if (stream) { res = ast_channel_tech(chan)->write_stream(chan, ast_stream_get_position(stream), fr); @@ -5130,6 +5130,7 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame } else { res = 0; } + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_VOICE: if (ast_opt_generic_plc && ast_format_cmp(fr->subclass.format, ast_format_slin) == AST_FORMAT_CMP_EQUAL) { @@ -5179,15 +5180,15 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame if (ast_channel_writetrans(chan)) { struct ast_frame *trans_frame = ast_translate(ast_channel_writetrans(chan), f, 0); - if (trans_frame != f && f != fr) { - /* - * If translate gives us a new frame and so did the audio - * hook then we need to free the one from the audio hook. - */ - ast_frfree(f); - } - f = trans_frame; - } + if (trans_frame != f && f != fr) { + /* + * If translate gives us a new frame and so did the audio + * hook then we need to free the one from the audio hook. + */ + ast_frfree(f); + } + f = trans_frame; + } } if (!f) { @@ -5285,6 +5286,7 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame /* 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; @@ -5323,6 +5325,7 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame res = 0; } } + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; case AST_FRAME_NULL: case AST_FRAME_IAX: @@ -5331,23 +5334,26 @@ int ast_write_stream(struct ast_channel *chan, int stream_num, struct ast_frame break; case AST_FRAME_RTCP: /* RTCP information is on a per-stream basis and only available on multistream capable channels */ + CHECK_BLOCKING(chan); if (ast_channel_tech(chan)->write_stream && stream) { res = ast_channel_tech(chan)->write_stream(chan, ast_stream_get_position(stream), fr); } else { res = 0; } + ast_clear_flag(ast_channel_flags(chan), AST_FLAG_BLOCKING); break; default: /* 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 eb8bbe660eb3867d3fada24740dc8dc35b384c5d 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 79a147745a..3f22cdd9c5 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -2725,11 +2725,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); \ @@ -4369,6 +4370,9 @@ int ast_channel_fd_add(struct ast_channel *chan, int value); 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 d92de81972..c4b63c6b38 100644 --- a/main/channel.c +++ b/main/channel.c @@ -2618,10 +2618,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) { @@ -3661,7 +3661,6 @@ static struct ast_frame *__ast_read(struct ast_channel *chan, int dropaudio, int } } } 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 d31ce94d8b..b926514ca4 100644 --- a/main/channel_internal_api.c +++ b/main/channel_internal_api.c @@ -165,6 +165,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 */ AST_VECTOR(, int) fds; /*!< File descriptors for channel -- Drivers will poll on * these file descriptors, so at least one must be non -1. * See \arg \ref AstFileDesc */ @@ -1178,6 +1179,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;