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

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

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);

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 */

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: