Allow AMI action callback to be reentrant.

Fix AMI module reload deadlock regression from ASTERISK-18479 when it
tried to fix the race between calling an AMI action callback and
unregistering that action.  Refixes ASTERISK-13784 broken by
ASTERISK-17785 change.

Locking the ao2 object guaranteed that there were no active callbacks that
mattered when ast_manager_unregister() was called.  Unfortunately, this
causes the deadlock situation.  The patch stops locking the ao2 object to
allow multiple threads to invoke the callback re-entrantly.  There is no
way to guarantee a module unload will not crash because of an active
callback.  The code attempts to minimize the chance with the registered
flag and the maximum 5 second delay before ast_manager_unregister()
returns.

The trunk version of the patch changes the API to fix the race condition
correctly to prevent the module code from unloading from memory while an
action callback is active.

* Don't hold the lock while calling the AMI action callback.

(closes issue ASTERISK-19487)
Reported by: Philippe Lindheimer

Review: https://reviewboard.asterisk.org/r/1818/
Review: https://reviewboard.asterisk.org/r/1820/
........

Merged revisions 359979 from http://svn.asterisk.org/svn/asterisk/branches/1.8

git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/10@359980 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/include/asterisk/manager.h b/include/asterisk/manager.h
index 534e43f90266bec9d100d0da9f6d0140028d6cf6..f30cc7329d6fc616506bf62a7b217e0fa8a5d169 100644 (file)
--- a/include/asterisk/manager.h
+++ b/include/asterisk/manager.h
@@ -160,6 +160,8 @@ struct manager_action {
         * function and unregestring the AMI action object.
         */
        unsigned int registered:1;
+       /*! Number of active func() calls in progress. */
+       unsigned int active_count;
 };
 
 /*! \brief External routines may register/unregister manager callbacks this way 

diff --git a/main/manager.c b/main/manager.c
index 85a524a39d14511acf7e213a9f901f8e675b52c7..a924b94e42b127b114e2a37dece1d6c7185ced87 100644 (file)
--- a/main/manager.c
+++ b/main/manager.c
@@ -1975,7 +1975,11 @@ int ast_hook_send_action(struct manager_custom_hook *hook, const char *msg)
 
                        ao2_lock(act_found);
                        if (act_found->registered && act_found->func) {
+                               ++act_found->active_count;
+                               ao2_unlock(act_found);
                                ret = act_found->func(&s, &m);
+                               ao2_lock(act_found);
+                               --act_found->active_count;
                        } else {
                                ret = -1;
                        }
@@ -4779,8 +4783,12 @@ static int process_message(struct mansession *s, const struct message *m)
                        ao2_lock(act_found);
                        if (act_found->registered && act_found->func) {
                                ast_debug(1, "Running action '%s'\n", act_found->action);
+                               ++act_found->active_count;
+                               ao2_unlock(act_found);
                                ret = act_found->func(s, m);
                                acted = 1;
+                               ao2_lock(act_found);
+                               --act_found->active_count;
                        }
                        ao2_unlock(act_found);
                }
@@ -5288,6 +5296,8 @@ int ast_manager_unregister(char *action)
        AST_RWLIST_UNLOCK(&actions);
 
        if (cur) {
+               time_t now;
+
                /*
                 * We have removed the action object from the container so we
                 * are no longer in a hurry.
@@ -5296,6 +5306,23 @@ int ast_manager_unregister(char *action)
                cur->registered = 0;
                ao2_unlock(cur);
 
+               /*
+                * Wait up to 5 seconds for any active invocations to complete
+                * before returning.  We have to wait instead of blocking
+                * because we may be waiting for ourself to complete.
+                */
+               now = time(NULL);
+               while (cur->active_count) {
+                       if (5 <= time(NULL) - now) {
+                               ast_debug(1,
+                                       "Unregister manager action %s timed out waiting for %d active instances to complete\n",
+                                       action, cur->active_count);
+                               break;
+                       }
+
+                       sched_yield();
+               }
+
                ao2_t_ref(cur, -1, "action object removed from list");
                ast_verb(2, "Manager unregistered action %s\n", action);
        }