[Asterisk-code-review] app.c: make sure that no non-async-signal-safe syscalls are used afte... (asterisk[13])

Friendly Automation asteriskteam at digium.com
Fri May 8 09:24:53 CDT 2020


Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/14391 )

Change subject: app.c: make sure that no non-async-signal-safe syscalls are used after fork before exec
......................................................................

app.c: make sure that no non-async-signal-safe syscalls are used after
fork before exec

Posix does only allow async-signal-safe syscalls after fork before exec.
As asterisk ignores this, functions like TrySystem or System sometimes
end up in a deadlocked child process. The patch prevents the use of
non-async-signal-safe syscalls.

ASTERISK-28776

Change-Id: Idc76365c0592ee3f3b3bd72a4f48f7a098978e8e
---
M main/app.c
M main/asterisk.c
M main/strcompat.c
3 files changed, 75 insertions(+), 58 deletions(-)

Approvals:
  George Joseph: Looks good to me, approved
  Friendly Automation: Approved for Submit



diff --git a/main/app.c b/main/app.c
index 4cd5934..120dabf 100644
--- a/main/app.c
+++ b/main/app.c
@@ -83,6 +83,9 @@
 
 static AST_LIST_HEAD_STATIC(zombies, zombie);
 
+#ifdef HAVE_CAP
+static cap_t child_cap;
+#endif
 /*
  * @{ \brief Define \ref stasis topic objects
  */
@@ -3014,12 +3017,7 @@
 	} else {
 		/* Child */
 #ifdef HAVE_CAP
-		cap_t cap = cap_from_text("cap_net_admin-eip");
-
-		if (cap_set_proc(cap)) {
-			ast_log(LOG_WARNING, "Unable to remove capabilities.\n");
-		}
-		cap_free(cap);
+		cap_set_proc(child_cap);
 #endif
 
 		/* Before we unblock our signals, return our trapped signals back to the defaults */
@@ -3129,6 +3127,9 @@
 
 static void app_cleanup(void)
 {
+#ifdef HAS_CAP
+	cap_free(child_cap);
+#endif
 	ao2_cleanup(queue_topic_pool);
 	queue_topic_pool = NULL;
 	ao2_cleanup(queue_topic_all);
@@ -3138,7 +3139,9 @@
 int app_init(void)
 {
 	ast_register_cleanup(app_cleanup);
-
+#ifdef HAVE_CAP
+	child_cap = cap_from_text("cap_net_admin-eip");
+#endif
 	queue_topic_all = stasis_topic_create("queue:all");
 	if (!queue_topic_all) {
 		return -1;
diff --git a/main/asterisk.c b/main/asterisk.c
index 8af5588..5572c9a 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -387,6 +387,10 @@
 
 static char randompool[256];
 
+#ifdef HAVE_CAP
+static cap_t child_cap;
+#endif
+
 static int sig_alert_pipe[2] = { -1, -1 };
 static struct {
 	 unsigned int need_reload:1;
@@ -1220,13 +1224,7 @@
 
 	if (pid == 0) {
 #ifdef HAVE_CAP
-		cap_t cap = cap_from_text("cap_net_admin-eip");
-
-		if (cap_set_proc(cap)) {
-			/* Careful with order! Logging cannot happen after we close FDs */
-			ast_log(LOG_WARNING, "Unable to remove capabilities.\n");
-		}
-		cap_free(cap);
+		cap_set_proc(child_cap);
 #endif
 #ifdef HAVE_WORKING_FORK
 		if (ast_opt_high_priority) {
@@ -1922,10 +1920,8 @@
 	if (pri) {
 		sched.sched_priority = 10;
 		if (sched_setscheduler(0, SCHED_RR, &sched)) {
-			ast_log(LOG_WARNING, "Unable to set high priority\n");
 			return -1;
-		} else
-			ast_verb(1, "Set to realtime thread\n");
+		}
 	} else {
 		sched.sched_priority = 0;
 		/* According to the manpage, these parameters can never fail. */
@@ -4037,8 +4033,14 @@
 		exit(1);
 	}
 
+#ifdef HAVE_CAP
+	child_cap = cap_from_text("cap_net_admin-eip");
+#endif
 	/* Not a remote console? Start the daemon. */
 	asterisk_daemon(isroot, runuser, rungroup);
+#ifdef HAS_CAP
+	cap_free(child_cap);
+#endif
 	return 0;
 }
 
diff --git a/main/strcompat.c b/main/strcompat.c
index c3b4ff1..1276de1 100644
--- a/main/strcompat.c
+++ b/main/strcompat.c
@@ -37,6 +37,8 @@
 
 #include "asterisk/utils.h"
 
+#define POLL_SIZE 1024
+
 #ifndef HAVE_STRSEP
 char *strsep(char **str, const char *delims)
 {
@@ -425,59 +427,69 @@
 #ifndef HAVE_CLOSEFROM
 void closefrom(int n)
 {
-	long x;
+	int maxfd;
+#ifndef _SC_OPEN_MAX
 	struct rlimit rl;
-	DIR *dir;
-	char path[16], *result;
-	struct dirent *entry;
+#endif
+	struct pollfd fds[POLL_SIZE];
+	int fd=n, loopmax, i;
+#ifndef STRICT_COMPAT
+	long flags;
+#endif
 
-	snprintf(path, sizeof(path), "/proc/%d/fd", (int) getpid());
-	if ((dir = opendir(path))) {
-		while ((entry = readdir(dir))) {
-			/* Skip . and .. */
-			if (entry->d_name[0] == '.') {
+#ifndef _SC_OPEN_MAX
+	if (getrlimit(RLIMIT_NOFILE, &rl) == -1) {
+		maxfd = -1;
+	} else {
+		maxfd = rl.rlim_cur;
+	}
+#else
+	maxfd = sysconf (_SC_OPEN_MAX);
+#endif
+
+	if (maxfd == -1 || maxfd > 65536) {
+		/* A more reasonable value.  Consider that the primary source of
+		 * file descriptors in Asterisk are UDP sockets, of which we are
+		 * limited to 65,535 per address.  We additionally limit that down
+		 * to about 10,000 sockets per protocol.  While the kernel will
+		 * allow us to set the fileno limit higher (up to 4.2 billion),
+		 * there really is no practical reason for it to be that high.
+		 *
+		 * sysconf as well as getrlimit can return -1 on error. Let's set
+		 * maxfd to the mentioned reasonable value of 65,535 in this case.
+		 */
+		maxfd = 65536;
+	}
+
+	while (fd < maxfd) {
+		loopmax = maxfd - fd;
+		if (loopmax > POLL_SIZE) {
+			loopmax = POLL_SIZE;
+		}
+		for (i = 0; i < loopmax; i++) {
+			fds[i].fd = fd+i;
+			fds[i].events = 0;
+		}
+		poll(fds, loopmax, 0);
+		for (i = 0; i < loopmax; i++) {
+			if (fds[i].revents == POLLNVAL) {
 				continue;
 			}
-			if ((x = strtol(entry->d_name, &result, 10)) && x >= n) {
 #ifdef STRICT_COMPAT
-				close(x);
+			close(fds[i].fd);
 #else
-				/* This isn't strictly compatible, but it's actually faster
-				 * for our purposes to set the CLOEXEC flag than to close
-				 * file descriptors.
-				 */
-				long flags = fcntl(x, F_GETFD);
-				if (flags == -1 && errno == EBADF) {
-					continue;
-				}
-				fcntl(x, F_SETFD, flags | FD_CLOEXEC);
-#endif
-			}
-		}
-		closedir(dir);
-	} else {
-		getrlimit(RLIMIT_NOFILE, &rl);
-		if (rl.rlim_cur > 65535) {
-			/* A more reasonable value.  Consider that the primary source of
-			 * file descriptors in Asterisk are UDP sockets, of which we are
-			 * limited to 65,535 per address.  We additionally limit that down
-			 * to about 10,000 sockets per protocol.  While the kernel will
-			 * allow us to set the fileno limit higher (up to 4.2 billion),
-			 * there really is no practical reason for it to be that high.
+			/* This isn't strictly compatible, but it's actually faster
+			 * for our purposes to set the CLOEXEC flag than to close
+			 * file descriptors.
 			 */
-			rl.rlim_cur = 65535;
-		}
-		for (x = n; x < rl.rlim_cur; x++) {
-#ifdef STRICT_COMPAT
-			close(x);
-#else
-			long flags = fcntl(x, F_GETFD);
+			flags = fcntl(fds[i].fd, F_GETFD);
 			if (flags == -1 && errno == EBADF) {
 				continue;
 			}
-			fcntl(x, F_SETFD, flags | FD_CLOEXEC);
+			fcntl(fds[i].fd, F_SETFD, flags | FD_CLOEXEC);
 #endif
 		}
+		fd += loopmax;
 	}
 }
 #endif

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/14391
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Idc76365c0592ee3f3b3bd72a4f48f7a098978e8e
Gerrit-Change-Number: 14391
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: nappsoft <infos at nappsoft.ch>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200508/2a0b2614/attachment-0001.html>


More information about the asterisk-code-review mailing list