res_agi.c: Ensure SIGCHLD handler functions are properly balanced.

Calls to `ast_replace_sigchld()` and `ast_unreplace_sigchld()` must be
balanced to ensure that we can capture the exit status of child
processes when we need to. This extends to functions that call
`ast_replace_sigchld()` and `ast_unreplace_sigchld()` such as
`ast_safe_fork()` and `ast_safe_fork_cleanup()`.

The primary change here is ensuring that we do not call
`ast_safe_fork_cleanup()` in `res_agi.c` if we have not previously
called `ast_safe_fork()`.

Additionally we reinforce some of the documentation and add an
assertion to, ideally, catch this sooner were this to happen again.

Fixes #922

(cherry picked from commit 243f20a78d373d0154dd33488c91fd40a5b0cfa1)

diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c
index f54007c5fdf5e97224ac103235af0ada5ff8a35f..714e4d95cf8b841e04c7008aa6d3e7902137a561 100644 (file)
--- a/apps/app_voicemail.c
+++ b/apps/app_voicemail.c
@@ -15803,7 +15803,6 @@ AST_TEST_DEFINE(test_voicemail_msgcount)
        if (!(vmu = find_user(&svm, testcontext, testmailbox)) &&
                !(vmu = find_or_create(testcontext, testmailbox))) {
                ast_test_status_update(test, "Cannot create vmu structure\n");
-               ast_unreplace_sigchld();
 #ifdef IMAP_STORAGE
                chan = ast_channel_unref(chan);
 #endif
@@ -15825,7 +15824,6 @@ AST_TEST_DEFINE(test_voicemail_msgcount)
                        if ((syserr = ast_safe_system(syscmd))) {
                                ast_test_status_update(test, "Unable to create test voicemail: %s\n",
                                        syserr > 0 ? strerror(syserr) : "unable to fork()");
-                               ast_unreplace_sigchld();
 #ifdef IMAP_STORAGE
                                chan = ast_channel_unref(chan);
 #endif

diff --git a/include/asterisk/app.h b/include/asterisk/app.h
index e4314509097825bd9b438b0f31525562da9b6a9f..587b6b04f9de802055f266526de2299d8d28409d 100644 (file)
--- a/include/asterisk/app.h
+++ b/include/asterisk/app.h
@@ -1491,6 +1491,10 @@ int ast_safe_fork(int stop_reaper);
 
 /*!
  * \brief Common routine to cleanup after fork'ed process is complete (if reaping was stopped)
+ *
+ * \note This must <b>not</b> be called unless ast_safe_fork(1) has been called
+ * previously.
+ *
  * \since 1.6.1
  */
 void ast_safe_fork_cleanup(void);

diff --git a/main/asterisk.c b/main/asterisk.c
index 527ed4445c5686f69f09b3d5e0ebf83a76c1309f..132ae80e7f3fce8a994f3ea4d2821a0aae5adf9d 100644 (file)
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -1118,6 +1118,10 @@ void ast_unreplace_sigchld(void)
        unsigned int level;
 
        ast_mutex_lock(&safe_system_lock);
+
+       /* Wrapping around here is an error */
+       ast_assert(safe_system_level > 0);
+
        level = --safe_system_level;
 
        /* only restore the handler if we are the last one */

diff --git a/res/res_agi.c b/res/res_agi.c
index f09329bb6fc06bc89574ef2d1c192a536a7044bd..5789f63fb0f4b8c2044275d32523d6e933e29a04 100644 (file)
--- a/res/res_agi.c
+++ b/res/res_agi.c
@@ -2192,12 +2192,15 @@ static enum agi_result launch_ha_netscript(char *agiurl, char *argv[], int *fds)
        return AGI_RESULT_FAILURE;
 }
 
-static enum agi_result launch_script(struct ast_channel *chan, char *script, int argc, char *argv[], int *fds, int *efd, int *opid)
+static enum agi_result launch_script(struct ast_channel *chan, char *script, int argc, char *argv[], int *fds, int *efd, int *opid, int *safe_fork_called)
 {
        char tmp[256];
        int pid, toast[2], fromast[2], audio[2], res;
        struct stat st;
 
+       /* We should not call ast_safe_fork_cleanup() if we never call ast_safe_fork(1) */
+       *safe_fork_called = 0;
+
        if (!strncasecmp(script, "agi://", 6)) {
                return (efd == NULL) ? launch_netscript(script, argv, fds) : AGI_RESULT_FAILURE;
        }
@@ -2252,6 +2255,8 @@ static enum agi_result launch_script(struct ast_channel *chan, char *script, int
                }
        }
 
+       *safe_fork_called = 1;
+
        if ((pid = ast_safe_fork(1)) < 0) {
                ast_log(LOG_WARNING, "Failed to fork(): %s\n", strerror(errno));
                return AGI_RESULT_FAILURE;
@@ -4531,6 +4536,7 @@ static int agi_exec_full(struct ast_channel *chan, const char *data, int enhance
        enum agi_result res;
        char *buf;
        int fds[2], efd = -1, pid = -1;
+       int safe_fork_called = 0;
        AST_DECLARE_APP_ARGS(args,
                AST_APP_ARG(arg)[MAX_ARGS];
        );
@@ -4553,7 +4559,7 @@ static int agi_exec_full(struct ast_channel *chan, const char *data, int enhance
                        return -1;
        }
 #endif
-       res = launch_script(chan, args.arg[0], args.argc, args.arg, fds, enhanced ? &efd : NULL, &pid);
+       res = launch_script(chan, args.arg[0], args.argc, args.arg, fds, enhanced ? &efd : NULL, &pid, &safe_fork_called);
        /* Async AGI do not require run_agi(), so just proceed if normal AGI
           or Fast AGI are setup with success. */
        if (res == AGI_RESULT_SUCCESS || res == AGI_RESULT_SUCCESS_FAST) {
@@ -4571,7 +4577,9 @@ static int agi_exec_full(struct ast_channel *chan, const char *data, int enhance
                if (efd > -1)
                        close(efd);
        }
-       ast_safe_fork_cleanup();
+       if (safe_fork_called) {
+               ast_safe_fork_cleanup();
+       }
 
        switch (res) {
        case AGI_RESULT_SUCCESS: