From c7f2d02ef1d4e5c10987c807ecec377453bb425c Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Tue, 22 May 2012 17:29:12 +0000 Subject: [PATCH] Fix race condition for CEL LINKEDID_END event This patch fixes to situations that could cause the CEL LINKEDID_END event to be missed. 1) During a core stop gracefully, modules are unloaded when ast_active_channels == 0. The LINKDEDID_END event fires during the channel destructor. This means that occasionally, the cel_* module will be unloaded before the channel is destroyed. It seemed generally useful to wait until the refcount of all channels == 0 before unloading, so I added a channel counter and used it in the shutdown code. 2) During a masquerade, ast_channel_change_linkedid is called. It calls ast_cel_check_retire_linkedid which unrefs the linkedid in the linkedids container in cel.c. It didn't ref the new linkedid. Now it does. Review: https://reviewboard.asterisk.org/r/1900/ ........ Merged revisions 367292 from http://svn.asterisk.org/svn/asterisk/branches/1.8 ........ Merged revisions 367299 from http://svn.asterisk.org/svn/asterisk/branches/10 git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@367309 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- include/asterisk/cel.h | 9 ++++++++ include/asterisk/channel.h | 5 ++++- main/asterisk.c | 4 ++-- main/cel.c | 43 ++++++++++++++++++++++++------------- main/channel.c | 16 +++++++++++--- main/channel_internal_api.c | 7 +++++- 6 files changed, 62 insertions(+), 22 deletions(-) diff --git a/include/asterisk/cel.h b/include/asterisk/cel.h index b388ffd9b5..25a3a4af1d 100644 --- a/include/asterisk/cel.h +++ b/include/asterisk/cel.h @@ -187,6 +187,15 @@ const char *ast_cel_get_ama_flag_name(enum ast_cel_ama_flag flag); */ void ast_cel_check_retire_linkedid(struct ast_channel *chan); +/*! + * \brief Inform CEL that a new linkedid is being used + * \since 11 + * + * \retval -1 error + * \retval 0 success + */ +int ast_cel_linkedid_ref(const char *linkedid); + /*! * \brief Create a fake channel from data in a CEL event * diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index 1a1c998857..59008a6fe4 100644 --- a/include/asterisk/channel.h +++ b/include/asterisk/channel.h @@ -2104,9 +2104,12 @@ void ast_begin_shutdown(int hangup); /*! Cancels an existing shutdown and returns to normal operation */ void ast_cancel_shutdown(void); -/*! \return number of active/allocated channels */ +/*! \return number of channels available for lookup */ int ast_active_channels(void); +/*! \return the number of channels not yet destroyed */ +int ast_undestroyed_channels(void); + /*! \return non-zero if Asterisk is being shut down */ int ast_shutting_down(void); diff --git a/main/asterisk.c b/main/asterisk.c index 972c2fbb10..849cbdd808 100644 --- a/main/asterisk.c +++ b/main/asterisk.c @@ -1704,7 +1704,7 @@ static int can_safely_quit(shutdown_nice_t niceness, int restart) for (;;) { time(&e); /* Wait up to 15 seconds for all channels to go away */ - if ((e - s) > 15 || !ast_active_channels() || shuttingdown != niceness) { + if ((e - s) > 15 || !ast_undestroyed_channels() || shuttingdown != niceness) { break; } /* Sleep 1/10 of a second */ @@ -1718,7 +1718,7 @@ static int can_safely_quit(shutdown_nice_t niceness, int restart) ast_verb(0, "Waiting for inactivity to perform %s...\n", restart ? "restart" : "halt"); } for (;;) { - if (!ast_active_channels() || shuttingdown != niceness) { + if (!ast_undestroyed_channels() || shuttingdown != niceness) { break; } sleep(1); diff --git a/main/cel.c b/main/cel.c index 676fe101e6..613557ec79 100644 --- a/main/cel.c +++ b/main/cel.c @@ -470,6 +470,31 @@ struct ast_channel *ast_cel_fabricate_channel_from_event(const struct ast_event return tchan; } +int ast_cel_linkedid_ref(const char *linkedid) +{ + char *lid; + + if (ast_strlen_zero(linkedid)) { + ast_log(LOG_ERROR, "The linkedid should never be empty\n"); + return -1; + } + + if (!(lid = ao2_find(linkedids, (void *) linkedid, OBJ_POINTER))) { + if (!(lid = ao2_alloc(strlen(linkedid) + 1, NULL))) { + return -1; + } + strcpy(lid, linkedid); + if (!ao2_link(linkedids, lid)) { + ao2_ref(lid, -1); + return -1; + } + /* Leave both the link and the alloc refs to show a count of 1 + the link */ + } + /* If we've found, go ahead and keep the ref to increment count of how many channels + * have this linkedid. We'll clean it up in check_retire */ + return 0; +} + int ast_cel_report_event(struct ast_channel *chan, enum ast_cel_event_type event_type, const char *userdefevname, const char *extra, struct ast_channel *peer2) { @@ -487,22 +512,10 @@ int ast_cel_report_event(struct ast_channel *chan, enum ast_cel_event_type event /* Record the linkedid of new channels if we are tracking LINKEDID_END even if we aren't * reporting on CHANNEL_START so we can track when to send LINKEDID_END */ if (cel_enabled && ast_cel_track_event(AST_CEL_LINKEDID_END) && event_type == AST_CEL_CHANNEL_START && linkedid) { - char *lid; - if (!(lid = ao2_find(linkedids, (void *) linkedid, OBJ_POINTER))) { - if (!(lid = ao2_alloc(strlen(linkedid) + 1, NULL))) { - ast_mutex_unlock(&reload_lock); - return -1; - } - strcpy(lid, linkedid); - if (!ao2_link(linkedids, lid)) { - ao2_ref(lid, -1); - ast_mutex_unlock(&reload_lock); - return -1; - } - /* Leave both the link and the alloc refs to show a count of 1 + the link */ + if (ast_cel_linkedid_ref(linkedid)) { + ast_mutex_unlock(&reload_lock); + return -1; } - /* If we've found, go ahead and keep the ref to increment count of how many channels - * have this linkedid. We'll clean it up in check_retire */ } if (!cel_enabled || !ast_cel_track_event(event_type)) { diff --git a/main/channel.c b/main/channel.c index 2ccd18f3e2..98b2f44038 100644 --- a/main/channel.c +++ b/main/channel.c @@ -94,6 +94,7 @@ struct ast_epoll_data { static int shutting_down; static int uniqueint; +static int chancount; unsigned long global_fin, global_fout; @@ -645,6 +646,11 @@ int ast_active_channels(void) return channels ? ao2_container_count(channels) : 0; } +int ast_undestroyed_channels(void) +{ + return ast_atomic_fetchadd_int(&chancount, 0); +} + /*! \brief Cancel a shutdown in progress */ void ast_cancel_shutdown(void) { @@ -1095,6 +1101,7 @@ __ast_channel_alloc_ap(int needqueue, int state, const char *cid_num, const char ast_cdr_init(ast_channel_cdr(tmp), tmp); ast_cdr_start(ast_channel_cdr(tmp)); + ast_atomic_fetchadd_int(&chancount, +1); ast_cel_report_event(tmp, AST_CEL_CHANNEL_START, NULL, NULL, NULL); headp = ast_channel_varshead(tmp); @@ -2302,6 +2309,7 @@ static void ast_channel_destructor(void *obj) if (callid) { ast_callid_unref(callid); } + ast_atomic_fetchadd_int(&chancount, -1); } /*! \brief Free a dummy channel structure */ @@ -6357,15 +6365,17 @@ static const char *oldest_linkedid(const char *a, const char *b) * see if the channel's old linkedid is now being retired */ static void ast_channel_change_linkedid(struct ast_channel *chan, const char *linkedid) { + ast_assert(linkedid != NULL); /* if the linkedid for this channel is being changed from something, check... */ - if (!ast_strlen_zero(ast_channel_linkedid(chan)) && 0 != strcmp(ast_channel_linkedid(chan), linkedid)) { - ast_cel_check_retire_linkedid(chan); + if (ast_channel_linkedid(chan) && !strcmp(ast_channel_linkedid(chan), linkedid)) { + return; } + ast_cel_check_retire_linkedid(chan); ast_channel_linkedid_set(chan, linkedid); + ast_cel_linkedid_ref(linkedid); } - /*! \brief Propagate the oldest linkedid between associated channels diff --git a/main/channel_internal_api.c b/main/channel_internal_api.c index 5125c6f59d..3fc3277d02 100644 --- a/main/channel_internal_api.c +++ b/main/channel_internal_api.c @@ -423,7 +423,6 @@ DEFINE_STRINGFIELD_SETTERS_FOR(peeraccount); DEFINE_STRINGFIELD_SETTERS_FOR(userfield); DEFINE_STRINGFIELD_SETTERS_FOR(call_forward); DEFINE_STRINGFIELD_SETTERS_FOR(uniqueid); -DEFINE_STRINGFIELD_SETTERS_FOR(linkedid); DEFINE_STRINGFIELD_SETTERS_FOR(parkinglot); DEFINE_STRINGFIELD_SETTERS_FOR(hangupsource); DEFINE_STRINGFIELD_SETTERS_FOR(dialcontext); @@ -446,6 +445,12 @@ DEFINE_STRINGFIELD_GETTER_FOR(parkinglot); DEFINE_STRINGFIELD_GETTER_FOR(hangupsource); DEFINE_STRINGFIELD_GETTER_FOR(dialcontext); +void ast_channel_linkedid_set(struct ast_channel *chan, const char *value) +{ + ast_assert(!ast_strlen_zero(value)); + ast_string_field_set(chan, linkedid, value); +} + const char *ast_channel_appl(const struct ast_channel *chan) { return chan->appl;